Refactor namer scheme, fix trailing digits (gfx-rs#1510)

src/proc/namer.rs

@@ -2,6 +2,7 @@ use crate::{arena::Handle, FastHashMap, FastHashSet};
 use std::borrow::Cow;
 
 pub type EntryPointIndex = u16;
+const SEPARATOR: char = '_';
 
 #[derive(Debug, Eq, Hash, PartialEq)]
 pub enum NameKey {
@@ -30,23 +31,35 @@ pub struct Namer {
 impl Namer {
     /// Return a form of `string` suitable for use as the base of an identifier.
     ///
-    /// Retain only alphanumeric and `_` characters. Drop leading digits. Avoid
-    /// prefixes in [`Namer::reserved_prefixes`].
+    /// - Drop leading digits.
+    /// - Retain only alphanumeric and `_` characters.
+    /// - Avoid prefixes in [`Namer::reserved_prefixes`].
+    ///
+    /// The return value is a valid identifier prefix in all of Naga's output languages,
+    /// and it never ends with a `SEPARATOR` character.
+    /// It is used as a key into the unique table.
     fn sanitize<'s>(&self, string: &'s str) -> Cow<'s, str> {
-        let string = string.trim_start_matches(|c: char| c.is_numeric() || c == '_');
+        let string = string
+            .trim_start_matches(|c: char| c.is_numeric())
+            .trim_end_matches(SEPARATOR);
 
-        let base = if string
-            .chars()
-            .all(|c: char| c.is_ascii_alphanumeric() || c == '_')
+        let base = if !string.is_empty()
+            && string
+                .chars()
+                .all(|c: char| c.is_ascii_alphanumeric() || c == '_')
         {
             Cow::Borrowed(string)
         } else {
-            Cow::Owned(
-                string
-                    .chars()
-                    .filter(|&c| c.is_ascii_alphanumeric() || c == '_')
-                    .collect::<String>(),
-            )
+            let mut filtered = string
+                .chars()
+                .filter(|&c| c.is_ascii_alphanumeric() || c == '_')
+                .collect::<String>();
+            let stripped_len = filtered.trim_end_matches(SEPARATOR).len();
+            filtered.truncate(stripped_len);
+            if filtered.is_empty() {
+                filtered.push_str("unnamed");
+            }
+            Cow::Owned(filtered)
         };
 
         for prefix in &self.reserved_prefixes {
@@ -69,14 +82,10 @@ impl Namer {
     /// Guarantee uniqueness by applying a numeric suffix when necessary. If `label_raw`
     /// itself ends with digits, separate them from the suffix with an underscore.
     pub fn call(&mut self, label_raw: &str) -> String {
-        use std::fmt::Write; // for write!-ing to Strings
+        use std::fmt::Write as _; // for write!-ing to Strings
 
         let base = self.sanitize(label_raw);
-        let separator = if base.ends_with(char::is_numeric) {
-            "_"
-        } else {
-            ""
-        };
+        debug_assert!(!base.is_empty() && !base.ends_with(SEPARATOR));
 
         // This would seem to be a natural place to use `HashMap::entry`. However, `entry`
         // requires an owned key, and we'd like to avoid heap-allocating strings we're
@@ -88,42 +97,26 @@ impl Namer {
                 *count += 1;
                 // Add the suffix. This may fit in base's existing allocation.
                 let mut suffixed = base.into_owned();
-                write!(&mut suffixed, "{}{}", separator, *count).unwrap();
+                write!(&mut suffixed, "{}{}", SEPARATOR, *count).unwrap();
                 suffixed
             }
             None => {
-                let mut count = 0;
-                let mut suffixed = Cow::Borrowed(base.as_ref());
-                while self.keywords.contains(suffixed.as_ref()) {
-                    count += 1;
-                    // Try to reuse suffixed's allocation.
-                    let mut buf = suffixed.into_owned();
-                    buf.clear();
-                    write!(&mut buf, "{}{}{}", base, separator, count).unwrap();
-                    suffixed = Cow::Owned(buf);
+                let mut suffixed = base.to_string();
+                if base.ends_with(char::is_numeric) || self.keywords.contains(base.as_ref()) {
+                    suffixed.push(SEPARATOR);
                 }
-                // Produce our return value, which must be an owned string. This allocates
-                // only if we haven't already done so earlier.
-                let suffixed = suffixed.into_owned();
-
+                debug_assert!(!self.keywords.contains(&suffixed));
                 // `self.unique` wants to own its keys. This allocates only if we haven't
                 // already done so earlier.
-                self.unique.insert(base.into_owned(), count);
-
+                self.unique.insert(base.into_owned(), 0);
                 suffixed
             }
         }
     }
 
     pub fn call_or(&mut self, label: &Option<String>, fallback: &str) -> String {
         self.call(match *label {
-            Some(ref name) => {
-                if name.trim().is_empty() {
-                    fallback
-                } else {
-                    name
-                }
-            }
+            Some(ref name) => name,
             None => fallback,
         })
     }
@@ -258,3 +251,11 @@ impl Namer {
         }
     }
 }
+
+#[test]
+fn test() {
+    let mut namer = Namer::default();
+    assert_eq!(namer.call("x"), "x");
+    assert_eq!(namer.call("x"), "x_1");
+    assert_eq!(namer.call("x1"), "x1_");
+}

tests/out/glsl/access.atomics.Compute.glsl

@@ -13,8 +13,8 @@ layout(std430) buffer Bar_block_0Cs {
 } _group_0_binding_0;
 
 
-float read_from_private(inout float foo2) {
-    float _e2 = foo2;
+float read_from_private(inout float foo_2) {
+    float _e2 = foo_2;
     return _e2;
 }
 

tests/out/glsl/access.foo.Vertex.glsl

@@ -11,22 +11,22 @@ layout(std430) buffer Bar_block_0Vs {
 } _group_0_binding_0;
 
 
-float read_from_private(inout float foo2) {
-    float _e2 = foo2;
+float read_from_private(inout float foo_2) {
+    float _e2 = foo_2;
     return _e2;
 }
 
 void main() {
     uint vi = uint(gl_VertexID);
-    float foo1 = 0.0;
+    float foo_1 = 0.0;
     int c[5];
-    float baz = foo1;
-    foo1 = 1.0;
+    float baz = foo_1;
+    foo_1 = 1.0;
     mat4x4 matrix = _group_0_binding_0.matrix;
     uvec2 arr[2] = _group_0_binding_0.arr;
     float b = _group_0_binding_0.matrix[3][0];
     int a = _group_0_binding_0.data[(uint(_group_0_binding_0.data.length()) - 2u)];
-    float _e25 = read_from_private(foo1);
+    float _e25 = read_from_private(foo_1);
     _group_0_binding_0.matrix[1][2] = 1.0;
     _group_0_binding_0.matrix = mat4x4(vec4(0.0), vec4(1.0), vec4(2.0), vec4(3.0));
     _group_0_binding_0.arr = uvec2[2](uvec2(0u), uvec2(1u));

tests/out/glsl/bits.main.Compute.glsl

@@ -8,83 +8,83 @@ layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
 
 void main() {
     int i = 0;
-    ivec2 i2 = ivec2(0, 0);
-    ivec3 i3 = ivec3(0, 0, 0);
-    ivec4 i4 = ivec4(0, 0, 0, 0);
+    ivec2 i2_ = ivec2(0, 0);
+    ivec3 i3_ = ivec3(0, 0, 0);
+    ivec4 i4_ = ivec4(0, 0, 0, 0);
     uint u = 0u;
-    uvec2 u2 = uvec2(0u, 0u);
-    uvec3 u3 = uvec3(0u, 0u, 0u);
-    uvec4 u4 = uvec4(0u, 0u, 0u, 0u);
-    vec2 f2 = vec2(0.0, 0.0);
-    vec4 f4 = vec4(0.0, 0.0, 0.0, 0.0);
-    i2 = ivec2(0);
-    i3 = ivec3(0);
-    i4 = ivec4(0);
-    u2 = uvec2(0u);
-    u3 = uvec3(0u);
-    u4 = uvec4(0u);
-    f2 = vec2(0.0);
-    f4 = vec4(0.0);
-    vec4 _e28 = f4;
+    uvec2 u2_ = uvec2(0u, 0u);
+    uvec3 u3_ = uvec3(0u, 0u, 0u);
+    uvec4 u4_ = uvec4(0u, 0u, 0u, 0u);
+    vec2 f2_ = vec2(0.0, 0.0);
+    vec4 f4_ = vec4(0.0, 0.0, 0.0, 0.0);
+    i2_ = ivec2(0);
+    i3_ = ivec3(0);
+    i4_ = ivec4(0);
+    u2_ = uvec2(0u);
+    u3_ = uvec3(0u);
+    u4_ = uvec4(0u);
+    f2_ = vec2(0.0);
+    f4_ = vec4(0.0);
+    vec4 _e28 = f4_;
     u = packSnorm4x8(_e28);
-    vec4 _e30 = f4;
+    vec4 _e30 = f4_;
     u = packUnorm4x8(_e30);
-    vec2 _e32 = f2;
+    vec2 _e32 = f2_;
     u = packSnorm2x16(_e32);
-    vec2 _e34 = f2;
+    vec2 _e34 = f2_;
     u = packUnorm2x16(_e34);
-    vec2 _e36 = f2;
+    vec2 _e36 = f2_;
     u = packHalf2x16(_e36);
     uint _e38 = u;
-    f4 = unpackSnorm4x8(_e38);
+    f4_ = unpackSnorm4x8(_e38);
     uint _e40 = u;
-    f4 = unpackUnorm4x8(_e40);
+    f4_ = unpackUnorm4x8(_e40);
     uint _e42 = u;
-    f2 = unpackSnorm2x16(_e42);
+    f2_ = unpackSnorm2x16(_e42);
     uint _e44 = u;
-    f2 = unpackUnorm2x16(_e44);
+    f2_ = unpackUnorm2x16(_e44);
     uint _e46 = u;
-    f2 = unpackHalf2x16(_e46);
+    f2_ = unpackHalf2x16(_e46);
     int _e48 = i;
     int _e49 = i;
     i = bitfieldInsert(_e48, _e49, int(5u), int(10u));
-    ivec2 _e53 = i2;
-    ivec2 _e54 = i2;
-    i2 = bitfieldInsert(_e53, _e54, int(5u), int(10u));
-    ivec3 _e58 = i3;
-    ivec3 _e59 = i3;
-    i3 = bitfieldInsert(_e58, _e59, int(5u), int(10u));
-    ivec4 _e63 = i4;
-    ivec4 _e64 = i4;
-    i4 = bitfieldInsert(_e63, _e64, int(5u), int(10u));
+    ivec2 _e53 = i2_;
+    ivec2 _e54 = i2_;
+    i2_ = bitfieldInsert(_e53, _e54, int(5u), int(10u));
+    ivec3 _e58 = i3_;
+    ivec3 _e59 = i3_;
+    i3_ = bitfieldInsert(_e58, _e59, int(5u), int(10u));
+    ivec4 _e63 = i4_;
+    ivec4 _e64 = i4_;
+    i4_ = bitfieldInsert(_e63, _e64, int(5u), int(10u));
     uint _e68 = u;
     uint _e69 = u;
     u = bitfieldInsert(_e68, _e69, int(5u), int(10u));
-    uvec2 _e73 = u2;
-    uvec2 _e74 = u2;
-    u2 = bitfieldInsert(_e73, _e74, int(5u), int(10u));
-    uvec3 _e78 = u3;
-    uvec3 _e79 = u3;
-    u3 = bitfieldInsert(_e78, _e79, int(5u), int(10u));
-    uvec4 _e83 = u4;
-    uvec4 _e84 = u4;
-    u4 = bitfieldInsert(_e83, _e84, int(5u), int(10u));
+    uvec2 _e73 = u2_;
+    uvec2 _e74 = u2_;
+    u2_ = bitfieldInsert(_e73, _e74, int(5u), int(10u));
+    uvec3 _e78 = u3_;
+    uvec3 _e79 = u3_;
+    u3_ = bitfieldInsert(_e78, _e79, int(5u), int(10u));
+    uvec4 _e83 = u4_;
+    uvec4 _e84 = u4_;
+    u4_ = bitfieldInsert(_e83, _e84, int(5u), int(10u));
     int _e88 = i;
     i = bitfieldExtract(_e88, int(5u), int(10u));
-    ivec2 _e92 = i2;
-    i2 = bitfieldExtract(_e92, int(5u), int(10u));
-    ivec3 _e96 = i3;
-    i3 = bitfieldExtract(_e96, int(5u), int(10u));
-    ivec4 _e100 = i4;
-    i4 = bitfieldExtract(_e100, int(5u), int(10u));
+    ivec2 _e92 = i2_;
+    i2_ = bitfieldExtract(_e92, int(5u), int(10u));
+    ivec3 _e96 = i3_;
+    i3_ = bitfieldExtract(_e96, int(5u), int(10u));
+    ivec4 _e100 = i4_;
+    i4_ = bitfieldExtract(_e100, int(5u), int(10u));
     uint _e104 = u;
     u = bitfieldExtract(_e104, int(5u), int(10u));
-    uvec2 _e108 = u2;
-    u2 = bitfieldExtract(_e108, int(5u), int(10u));
-    uvec3 _e112 = u3;
-    u3 = bitfieldExtract(_e112, int(5u), int(10u));
-    uvec4 _e116 = u4;
-    u4 = bitfieldExtract(_e116, int(5u), int(10u));
+    uvec2 _e108 = u2_;
+    u2_ = bitfieldExtract(_e108, int(5u), int(10u));
+    uvec3 _e112 = u3_;
+    u3_ = bitfieldExtract(_e112, int(5u), int(10u));
+    uvec4 _e116 = u4_;
+    u4_ = bitfieldExtract(_e116, int(5u), int(10u));
     return;
 }
 

tests/out/glsl/image.main.Compute.glsl

@@ -21,11 +21,11 @@ void main() {
     uvec3 local_id = gl_LocalInvocationID;
     ivec2 dim = imageSize(_group_0_binding_1).xy;
     ivec2 itc = ((dim * ivec2(local_id.xy)) % ivec2(10, 20));
-    uvec4 value1 = texelFetch(_group_0_binding_0, itc, int(local_id.z));
-    uvec4 value2 = texelFetch(_group_0_binding_3, itc, int(local_id.z));
-    uvec4 value4 = imageLoad(_group_0_binding_1, itc);
-    uvec4 value5 = texelFetch(_group_0_binding_5, ivec3(itc, int(local_id.z)), (int(local_id.z) + 1));
-    imageStore(_group_0_binding_2, ivec2(itc.x, 0.0), (((value1 + value2) + value4) + value5));
+    uvec4 value1_ = texelFetch(_group_0_binding_0, itc, int(local_id.z));
+    uvec4 value2_ = texelFetch(_group_0_binding_3, itc, int(local_id.z));
+    uvec4 value4_ = imageLoad(_group_0_binding_1, itc);
+    uvec4 value5_ = texelFetch(_group_0_binding_5, ivec3(itc, int(local_id.z)), (int(local_id.z) + 1));
+    imageStore(_group_0_binding_2, ivec2(itc.x, 0.0), (((value1_ + value2_) + value4_) + value5_));
     return;
 }
 

tests/out/glsl/interpolate.main.Fragment.glsl

@@ -1,7 +1,7 @@
 #version 400 core
 struct FragmentInput {
     vec4 position;
-    uint flat1;
+    uint flat_;
     float linear;
     vec2 linear_centroid;
     vec3 linear_sample;

tests/out/glsl/interpolate.main.Vertex.glsl

@@ -1,7 +1,7 @@
 #version 400 core
 struct FragmentInput {
     vec4 position;
-    uint flat1;
+    uint flat_;
     float linear;
     vec2 linear_centroid;
     vec3 linear_sample;
@@ -19,18 +19,18 @@ smooth centroid out float _vs2fs_location5;
 smooth sample out float _vs2fs_location6;
 
 void main() {
-    FragmentInput out1;
-    out1.position = vec4(2.0, 4.0, 5.0, 6.0);
-    out1.flat1 = 8u;
-    out1.linear = 27.0;
-    out1.linear_centroid = vec2(64.0, 125.0);
-    out1.linear_sample = vec3(216.0, 343.0, 512.0);
-    out1.perspective = vec4(729.0, 1000.0, 1331.0, 1728.0);
-    out1.perspective_centroid = 2197.0;
-    out1.perspective_sample = 2744.0;
-    FragmentInput _e30 = out1;
+    FragmentInput out_;
+    out_.position = vec4(2.0, 4.0, 5.0, 6.0);
+    out_.flat_ = 8u;
+    out_.linear = 27.0;
+    out_.linear_centroid = vec2(64.0, 125.0);
+    out_.linear_sample = vec3(216.0, 343.0, 512.0);
+    out_.perspective = vec4(729.0, 1000.0, 1331.0, 1728.0);
+    out_.perspective_centroid = 2197.0;
+    out_.perspective_sample = 2744.0;
+    FragmentInput _e30 = out_;
     gl_Position = _e30.position;
-    _vs2fs_location0 = _e30.flat1;
+    _vs2fs_location0 = _e30.flat_;
     _vs2fs_location1 = _e30.linear;
     _vs2fs_location2 = _e30.linear_centroid;
     _vs2fs_location3 = _e30.linear_sample;