Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subtracting float from product requires parentheses #4445

Closed
wrnrlr opened this issue Nov 18, 2021 · 6 comments · Fixed by #4870
Closed

Subtracting float from product requires parentheses #4445

wrnrlr opened this issue Nov 18, 2021 · 6 comments · Fixed by #4870
Labels
area: naga front-end lang: WGSL WebGPU Shading Language naga Shader Translator type: bug Something isn't working

Comments

@wrnrlr
Copy link

wrnrlr commented Nov 18, 2021

It looks like using a minus sign in an "additive_expression" wrapped in a "paren_expression" results in a parser error

// This ok:
let a = (3.0*2.0-(1.0)) * 1.0;
let b = (3.0*2.0+1.0) * 1.0;
// This fails:
let c = (3.0*2.0-1.0) * 1.0;
Parse Error: expected ')', found '-1.0'

This confused me for a while, and from what I understand from the grammar it should be allowed.

To reproduce this you can run this fragment shader with my slimshader or any such tool:

[[block]]
struct Uniforms {
  resolution: vec2<f32>; // in pixels
  playtime: f32; // in seconds
};

[[group(0), binding(0)]]
var<uniform> uniforms: Uniforms;

struct VertexOutput {
  [[location(0)]] coord: vec2<f32>;
  [[builtin(position)]] position: vec4<f32>;
};

[[stage(fragment)]]
fn main(in: VertexOutput) -> [[location(0)]] vec4<f32> {
  // This ok
  let a = (3.0*2.0-(1.0)) * 1.0;
  let b = (3.0*2.0+1.0) * 1.0;
  // This fails
  let c = (3.0*2.0-1.0) * 1.0;
  return vec4<f32>(1.0, 0.0, 0.0, 1.0);
}
@wrnrlr
Copy link
Author

wrnrlr commented Nov 20, 2021

Strangely subtracting does work when you add whitespace after the minus sign:

// Okey
let c = (3.0*2.0 - 1.0) * 1.0;
// Fail
let c = (3.0*2.0-1.0) * 1.0;

@meshula
Copy link

meshula commented Nov 20, 2021

parser preferred to parse the negative number, instead of expecting an operator or close paren? maybe the tokenizer isn't aware of the grammar context?

@jimblandy
Copy link
Member

jimblandy commented Feb 23, 2022

This is apparently per spec:

Note: literals are parsed greedily. This means that for statements like a -5 this will not parse as a minus 5 but instead as a -5 which may be unexpected. A space must be inserted after the - if the first expression is desired.

This is considered a bug in the spec, but it hasn't been fixed yet.

@jimblandy
Copy link
Member

For the record, the general plan is to let constant expression folding handle the negation, so -5 will be treated not as a single token, but as a unary negation operator followed by a literal 5. Then, a-5 will be parsed as three tokens, and the - will be taken as a binary operator, not part of the integer literal.

There's a wrinkle: the largest negative i32 value is -2147483648, but its positive counterpart 2147483648 is out of range for i32, so it is not possible to write this value as an i32 and then negate it. To work around this, WGSL plans to specify that evaluation of constant expressions occurs in some anonymous wider type.

Since the behavior reported is per spec, I'll close this bug.

@teoxoy
Copy link
Member

teoxoy commented Aug 29, 2023

The spec issue has been resolved, reopening.

We should be able to tackle this once the const-expressions work lands.

@teoxoy teoxoy reopened this Aug 29, 2023
@cwfitzgerald cwfitzgerald added the naga Shader Translator label Oct 25, 2023
@cwfitzgerald cwfitzgerald transferred this issue from gfx-rs/naga Oct 25, 2023
@cwfitzgerald cwfitzgerald added type: bug Something isn't working and removed kind: bug labels Oct 25, 2023
@teoxoy teoxoy added this to the WebGPU Specification V1 milestone Nov 3, 2023
@jimblandy
Copy link
Member

The following passes with #4870:

fn f() {
    // This ok:
    let a = (3.0*2.0-(1.0)) * 1.0;
    let b = (3.0*2.0+1.0) * 1.0;
    // This fails:
    let c = (3.0*2.0-1.0) * 1.0;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga front-end lang: WGSL WebGPU Shading Language naga Shader Translator type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants