Forbid local variable shadowing in WGSL

[zesterer]

This is actually generated by wgpu, but the error seems to be a mistranslation by naga.

[2021-06-28T19:36:06Z ERROR wgpu::backend::direct] wgpu error: Validation Error

    Caused by:
        In Device::create_render_pipeline
          note: label = `Render pipeline`
        Internal error in VERTEX shader: Error compiling the shader "\"Compilation failed: 

program_source:72:103: error: cannot initialize an array element of type 'char' with an lvalue of type 'metal::float3' (aka 'float3')
    VertexOut _e57 = VertexOut {_e52 * metal::float4(metal::float3(_e44.pos.xy, _e49), 1.0), _e44.uv, _e44.pos};
                                                                                                      ^~~~~~~~\
program_source:72:103: warning: suggest braces around initialization of subobject\
    VertexOut _e57 = VertexOut {_e52 * metal::float4(metal::float3(_e44.pos.xy, _e49), 1.0), _e44.uv, _e44.pos};
                                                                                                      ^~~~~~~~
                                                                                                      {       }
program_source:99:45: error: cannot initialize an array element of type 'char' with an lvalue of type 'metal::float3' (aka 'float3')
    VertexOut _e44 = VertexOut {_e41, _e42, _e43};
                                            ^~~~
program_source:99:45: warning: suggest braces around initialization of subobject
    VertexOut _e44 = VertexOut {_e41, _e42, _e43};
                                            ^~~~
                                            {   }

thread 'main' panicked at 'Handling wgpu errors as fatal by default', /Users/foo/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.9.0/src/backend/direct.rs:1978:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

(I've made an attempt at formatting the error such that it's a little more intelligible)

The original offending shader (written in wgsl, translated to spir-v also with naga, although possibly with a slightly different version) is as follows:

[[block]]
struct Globals {
    vp: mat4x4<f32>;
};
[[group(0), binding(0)]] var<uniform> globals: Globals;

[[group(1), binding(0)]] var heightmap_texture: texture_2d<f32>;
[[group(1), binding(1)]] var heightmap_sampler: sampler;
[[group(1), binding(2)]] var noise_texture: texture_2d<f32>;
[[group(1), binding(3)]] var noise_sampler: sampler;
[[group(1), binding(4)]] var top_texture: texture_2d<f32>;
[[group(1), binding(5)]] var top_sampler: sampler;
[[group(1), binding(6)]] var side_texture: texture_2d<f32>;
[[group(1), binding(7)]] var side_sampler: sampler;

struct VertexIn {
	[[location(0)]] pos: vec3<f32>;
	[[location(1)]] norm: vec3<f32>;
	[[location(2)]] uv: vec2<f32>;
};

struct VertexOut {
	[[builtin(position)]] pos: vec4<f32>;
	[[location(0)]] uv: vec2<f32>;
	[[location(1)]] wpos: vec3<f32>;
};

fn terrain_alt_at(wpos: vec2<f32>) -> f32 {
	let base_alt = textureSampleLevel(heightmap_texture, heightmap_sampler, wpos * 0.00004, 0.0).x * 5000.0;

	return base_alt
		+ base_alt * textureSampleLevel(noise_texture, noise_sampler, wpos * 0.0005, 0.0).x * 0.05
		+ base_alt * textureSampleLevel(noise_texture, noise_sampler, wpos * 0.0001, 0.0).x * 0.25
	;
}

fn terrain_norm_at(wpos: vec2<f32>) -> vec3<f32> {
	let planck = 10.0;
	let alt00 = terrain_alt_at(wpos);
	let alt10 = terrain_alt_at(wpos + vec2<f32>(planck, 0.0));
	let alt01 = terrain_alt_at(wpos + vec2<f32>(0.0, planck));
	let slope = abs((alt00 - alt10) * (alt00 - alt01)) + 0.0001;
	let nmap = vec3<f32>(
        (alt00 - alt10) / planck,
        (alt00 - alt01) / planck,
        1.0,
    );
	return normalize(nmap);
}

[[stage(vertex)]]
fn vs_main(vert: VertexIn) -> VertexOut {
	let wpos = vec3<f32>(vert.pos.xy, terrain_alt_at(vert.pos.xy));
	return VertexOut(
		globals.vp * vec4<f32>(wpos, 1.0),
		vert.uv,
		vert.pos,
	);
}

var sun_dir: vec3<f32> = vec3<f32>(0.0, -0.5, -1.0);

[[stage(fragment)]]
fn fs_main(frag: VertexOut) -> [[location(0)]] vec4<f32> {
	let norm = terrain_norm_at(frag.wpos.xy);
	let col = mix(
		textureSample(side_texture, side_sampler, frag.wpos.xy * 0.00025).rgb,
		textureSample(top_texture, top_sampler, frag.wpos.xy * 0.001).rgb,
		vec3<f32>(clamp(pow(norm.z + 0.25, 8.0), 0.0, 1.0)),
	);
	let col = col * max(dot(normalize(norm), normalize(-sun_dir)), 0.0);
	return vec4<f32>(col, 1.0);
}

Unfortunately, I'm not the owner of the Mac in question, so my ability to reduce this to a minimal example is limited.

However, based on the error message produced by Metal (VertexOut {_e52 * metal::float4(metal::float3(_e44.pos.xy, _e49), 1.0), _e44.uv, _e44.pos}), it looks like the error occurs in the instantiation of the VertexOut structure in the vertex shader, here:

	return VertexOut(
		globals.vp * vec4<f32>(wpos, 1.0),
		vert.uv,
		vert.pos,
	);

[kvark]

Checking this locally, I'm seeing:

metal.metal:43:11: warning: unused variable 'slope' [-Wunused-variable]
    float slope = metal::abs((_e11 - _e15) * (_e11 - _e19)) + 0.0001;
          ^
metal.metal:101:19: error: redefinition of 'col'
    metal::float3 col = col * metal::max(metal::dot(metal::normalize(_e13), metal::normalize(-_e37)), 0.0);
                  ^
metal.metal:99:19: note: previous definition is here
    metal::float3 col = metal::mix(_e18.xyz, _e24.xyz, metal::float3(metal::clamp(metal::pow(_e13.z + 0.25, 8.0), 0.0, 1.0)));
                  ^

WGSL doesn't allow shadowing local variables. We just need to detect this.

[zesterer]

This doesn't seem to be the cause of the original error. I've removed all name shadowing that I can find, and I still get a similar error.

[kvark]

Are you trying this on Naga directly? What do you get now, exactly, on the output?

[zesterer]

I updated Naga to the latest commit 2 days ago. The code I'm using is:

#include shaders.util.globals
#include shaders.util.light

[[block]]
struct Locals {
    m: mat4x4<f32>;
};
[[group(1), binding(0)]] var<uniform> locals: Locals;

struct VertexIn {
	[[location(0)]] pos: vec3<f32>;
	[[location(1)]] norm: vec3<f32>;
	[[location(2)]] uv: vec2<f32>;
};

struct VertexOut {
	[[builtin(position)]] pos: vec4<f32>;
	[[location(0)]] norm: vec3<f32>;
	[[location(1)]] uv: vec2<f32>;
	[[location(2)]] wpos: vec3<f32>;
};

[[stage(vertex)]]
fn vs_main(vert: VertexIn) -> VertexOut {
	let wpos = locals.m * vec4<f32>(vert.pos, 1.0);
	return VertexOut(
		globals.vp * wpos,
		vert.norm,
		vert.uv,
		wpos.xyz,
	);
}

[[group(1), binding(1)]] var color_texture: texture_2d<f32>;
[[group(1), binding(2)]] var color_sampler: sampler;

[[stage(fragment)]]
fn fs_main(frag: VertexOut) -> [[location(0)]] vec4<f32> {
	let albedo = textureSample(color_texture, color_sampler, frag.uv).rgb;
	return vec4<f32>(illuminate(frag.wpos, normalize(frag.norm), albedo, 0.5, 1.0), 1.0);
}

(Ignore the #include pragmas)

The error we're getting on Mac OS is as follows:

[2021-07-06T17:59:47Z ERROR wgpu::backend::direct] wgpu error: Validation Error
    
    Caused by:
        In Device::create_render_pipeline
          note: label = `Render pipeline`
        Internal error in VERTEX shader: Error compiling the shader "
"Compilation failed:
program_source:151:66: error: cannot initialize an array element of type 'char' with an lvalue of type 'float3' (vector of 3 'float' values)
    VertexOut _e57 = VertexOut {_e52 * _e50, _e45.norm, _e45.uv, _e50.xyz};
                                                                 ^~~~~~~~
program_source:151:66: warning: suggest braces around initialization of subobject
    VertexOut _e57 = VertexOut {_e52 * _e50, _e45.norm, _e45.uv, _e50.xyz};
                                                                 ^~~~~~~~
                                                                 {       }
program_source:176:51: error: cannot initialize an array element of type 'char' with an lvalue of type 'metal::float3' (aka 'float3')
    VertexOut _e46 = VertexOut {_e42, _e43, _e44, _e45};
                                                  ^~~~
program_source:176:51: warning: suggest braces around initialization of subobject
    VertexOut _e46 = VertexOut {_e42, _e43, _e44, _e45};
                                                  ^~~~
                                                  {   }
"

[kvark]

Looks like the backend is adding the padding to this structure and failing to take it into account. This is related to gpuweb/gpuweb#1792 (comment)

[zesterer]

I can confirm this is the case. After switching the fields to all be vec4<f32>, the shader compiles.

[kvark]

@zesterer the subject, body, and the labels on this issue are totally different from what you are now facing. Would you mind filing a separate issue?

[zesterer]

Sure. I'll mostly copy the text of my recent comment.

[zesterer]

I've opened #1081 to track this latter issue.

[jimblandy]

This is still broken:

// Fragment shader
@stage(fragment)
fn main() -> @builtin(frag_depth) f32 {
    let color = 1.0;
    let color = 2.0;
    return color;
}