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

src/proc/namer.rs

@@ -1,7 +1,8 @@
 use crate::{arena::Handle, FastHashMap, FastHashSet};
-use std::collections::hash_map::Entry;
+use std::borrow::Cow;
 
 pub type EntryPointIndex = u16;
+const SEPARATOR: char = '_';
 
 #[derive(Debug, Eq, Hash, PartialEq)]
 pub enum NameKey {
@@ -30,26 +31,40 @@ 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. Ensure
-    /// that the string does not end with a digit, so we can attach numeric
-    /// suffixes without merging. Avoid prefixes in
-    /// [`Namer::reserved_prefixes`].
-    fn sanitize(&self, string: &str) -> String {
-        let mut base = string
-            .chars()
-            .skip_while(|&c| c.is_numeric() || c == '_')
-            .filter(|&c| c.is_ascii_alphanumeric() || c == '_')
-            .collect::<String>();
-        // close the name by '_' if the re is a number, so that
-        // we can have our own number!
-        match base.chars().next_back() {
-            Some(c) if !c.is_numeric() => {}
-            _ => base.push('_'),
+    /// - 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())
+            .trim_end_matches(SEPARATOR);
+
+        let base = if !string.is_empty()
+            && string
+                .chars()
+                .all(|c: char| c.is_ascii_alphanumeric() || c == '_')
+        {
+            Cow::Borrowed(string)
+        } else {
+            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 {
             if base.starts_with(prefix) {
-                return format!("gen_{}", base);
+                return Cow::Owned(format!("gen_{}", base));
             }
         }
 
@@ -66,36 +81,41 @@ impl Namer {
     ///
     /// Guarantee uniqueness by applying a numeric suffix when necessary.
     pub fn call(&mut self, label_raw: &str) -> String {
+        use std::fmt::Write as _; // for write!-ing to Strings
+
         let base = self.sanitize(label_raw);
-        match self.unique.entry(base) {
-            Entry::Occupied(mut e) => {
-                *e.get_mut() += 1;
-                format!("{}{}", e.key(), e.get())
+        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
+        // just going to throw away. The approach below double-hashes only when we create
+        // a new entry, in which case the heap allocation of the owned key was more
+        // expensive anyway.
+        match self.unique.get_mut(base.as_ref()) {
+            Some(count) => {
+                *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();
+                suffixed
             }
-            Entry::Vacant(e) => {
-                let name = e.key();
-                if self.keywords.contains(e.key()) {
-                    let name = format!("{}1", name);
-                    e.insert(1);
-                    name
-                } else {
-                    let name = name.to_string();
-                    e.insert(0);
-                    name
+            None => {
+                let mut suffixed = base.to_string();
+                if base.ends_with(char::is_numeric) || self.keywords.contains(base.as_ref()) {
+                    suffixed.push(SEPARATOR);
                 }
+                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(), 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,
         })
     }
@@ -230,3 +250,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/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;

tests/out/glsl/operators.main.Compute.glsl

@@ -50,8 +50,8 @@ float constructors() {
 }
 
 void modulo() {
-    int a1 = (1 % 1);
-    float b1 = (1.0 - 1.0 * trunc(1.0 / 1.0));
+    int a_1 = (1 % 1);
+    float b_1 = (1.0 - 1.0 * trunc(1.0 / 1.0));
     ivec3 c = (ivec3(1) % ivec3(1));
     vec3 d = (vec3(1.0) - vec3(1.0) * trunc(vec3(1.0) / vec3(1.0)));
 }

tests/out/glsl/quad-vert.main.Vertex.glsl

@@ -3,14 +3,14 @@
 precision highp float;
 precision highp int;
 
-struct type9 {
+struct type_9 {
     vec2 member;
     vec4 gen_gl_Position;
 };
 
 vec2 v_uv = vec2(0.0, 0.0);
 
-vec2 a_uv1 = vec2(0.0, 0.0);
+vec2 a_uv_1 = vec2(0.0, 0.0);
 
 struct gen_gl_PerVertex_block_0Vs {
     vec4 gen_gl_Position;
@@ -19,29 +19,29 @@ struct gen_gl_PerVertex_block_0Vs {
     float gen_gl_CullDistance[1];
 } perVertexStruct;
 
-vec2 a_pos1 = vec2(0.0, 0.0);
+vec2 a_pos_1 = vec2(0.0, 0.0);
 
 layout(location = 1) in vec2 _p2vs_location1;
 layout(location = 0) in vec2 _p2vs_location0;
 layout(location = 0) smooth out vec2 _vs2fs_location0;
 
-void main2() {
-    vec2 _e12 = a_uv1;
+void main_1() {
+    vec2 _e12 = a_uv_1;
     v_uv = _e12;
-    vec2 _e13 = a_pos1;
+    vec2 _e13 = a_pos_1;
     perVertexStruct.gen_gl_Position = vec4(_e13.x, _e13.y, 0.0, 1.0);
     return;
 }
 
 void main() {
     vec2 a_uv = _p2vs_location1;
     vec2 a_pos = _p2vs_location0;
-    a_uv1 = a_uv;
-    a_pos1 = a_pos;
-    main2();
+    a_uv_1 = a_uv;
+    a_pos_1 = a_pos;
+    main_1();
     vec2 _e7 = v_uv;
     vec4 _e8 = perVertexStruct.gen_gl_Position;
-    type9 _tmp_return = type9(_e7, _e8);
+    type_9 _tmp_return = type_9(_e7, _e8);
     _vs2fs_location0 = _tmp_return.member;
     gl_Position = _tmp_return.gen_gl_Position;
     gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);

tests/out/glsl/quad.main.Fragment.glsl

@@ -14,8 +14,8 @@ smooth in vec2 _vs2fs_location0;
 layout(location = 0) out vec4 _fs2p_location0;
 
 void main() {
-    vec2 uv1 = _vs2fs_location0;
-    vec4 color = texture(_group_0_binding_0, vec2(uv1));
+    vec2 uv_1 = _vs2fs_location0;
+    vec4 color = texture(_group_0_binding_0, vec2(uv_1));
     if ((color.w == 0.0)) {
         discard;
     }

tests/out/glsl/skybox.fs_main.Fragment.glsl

@@ -14,8 +14,8 @@ layout(location = 0) smooth in vec3 _vs2fs_location0;
 layout(location = 0) out vec4 _fs2p_location0;
 
 void main() {
-    VertexOutput in1 = VertexOutput(gl_FragCoord, _vs2fs_location0);
-    vec4 _e5 = texture(_group_0_binding_1, vec3(in1.uv));
+    VertexOutput in_ = VertexOutput(gl_FragCoord, _vs2fs_location0);
+    vec4 _e5 = texture(_group_0_binding_1, vec3(in_.uv));
     _fs2p_location0 = _e5;
     return;
 }

tests/out/hlsl/access.hlsl

@@ -1,9 +1,9 @@
 
 RWByteAddressBuffer bar : register(u0);
 
-float read_from_private(inout float foo2)
+float read_from_private(inout float foo_2)
 {
-    float _expr2 = foo2;
+    float _expr2 = foo_2;
     return _expr2;
 }
 
@@ -16,16 +16,16 @@ uint NagaBufferLengthRW(RWByteAddressBuffer buffer)
 
 float4 foo(uint vi : SV_VertexID) : SV_Position
 {
-    float foo1 = 0.0;
+    float foo_1 = 0.0;
     int c[5] = {(int)0,(int)0,(int)0,(int)0,(int)0};
 
-    float baz = foo1;
-    foo1 = 1.0;
-    float4x4 matrix1 = float4x4(asfloat(bar.Load4(0+0)), asfloat(bar.Load4(0+16)), asfloat(bar.Load4(0+32)), asfloat(bar.Load4(0+48)));
+    float baz = foo_1;
+    foo_1 = 1.0;
+    float4x4 matrix_ = float4x4(asfloat(bar.Load4(0+0)), asfloat(bar.Load4(0+16)), asfloat(bar.Load4(0+32)), asfloat(bar.Load4(0+48)));
     uint2 arr[2] = {asuint(bar.Load2(72+0)), asuint(bar.Load2(72+8))};
     float b = asfloat(bar.Load(0+48+0));
     int a = asint(bar.Load((((NagaBufferLengthRW(bar) - 88) / 8) - 2u)*8+88));
-    const float _e25 = read_from_private(foo1);
+    const float _e25 = read_from_private(foo_1);
     bar.Store(8+16+0, asuint(1.0));
     {
         float4x4 _value2 = float4x4(float4(0.0.xxxx), float4(1.0.xxxx), float4(2.0.xxxx), float4(3.0.xxxx));
@@ -46,15 +46,15 @@ float4 foo(uint vi : SV_VertexID) : SV_Position
     }
     c[(vi + 1u)] = 42;
     int value = c[vi];
-    return mul(float4(int4(value.xxxx)), matrix1);
+    return mul(float4(int4(value.xxxx)), matrix_);
 }
 
 [numthreads(1, 1, 1)]
 void atomics()
 {
     int tmp = (int)0;
 
-    int value1 = asint(bar.Load(64));
+    int value_1 = asint(bar.Load(64));
     int _e6; bar.InterlockedAdd(64, 5, _e6);
     tmp = _e6;
     int _e9; bar.InterlockedAdd(64, -5, _e9);
@@ -71,6 +71,6 @@ void atomics()
     tmp = _e24;
     int _e27; bar.InterlockedExchange(64, 5, _e27);
     tmp = _e27;
-    bar.Store(64, asuint(value1));
+    bar.Store(64, asuint(value_1));
     return;
 }

tests/out/hlsl/empty-global-name.hlsl

@@ -0,0 +1,19 @@
+
+struct type_1 {
+    int member;
+};
+
+RWByteAddressBuffer unnamed : register(u0);
+
+void function()
+{
+    int _expr8 = asint(unnamed.Load(0));
+    unnamed.Store(0, asuint((_expr8 + 1)));
+    return;
+}
+
+[numthreads(64, 1, 1)]
+void main()
+{
+    function();
+}