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

Imports clobber field names in locally defined structs #88

Open
Runi-c opened this issue May 2, 2024 · 5 comments
Open

Imports clobber field names in locally defined structs #88

Runi-c opened this issue May 2, 2024 · 5 comments

Comments

@Runi-c
Copy link

Runi-c commented May 2, 2024

#import dep::{foo, bar}

struct Example {
    bar: u32,
}

fn test() {
    let example = Example(0u);
    foo(example.bar);
}

This code produces this error:

error: invalid field accessor `bar`
  ┌─ shaders/test.wgsl:9:39
  │
9 │     dep::foo(example.bar);
  │                                       ^^^ invalid accessor
  │
  = invalid field accessor `bar`

If I amend the code to not import a symbol named bar, the error goes away. But it took a lot more debugging than I'd have liked to figure out that that's what was happening.

@robtfm
Copy link
Collaborator

robtfm commented May 2, 2024

hm. i was aware that local names got stamped on, but i thought they got stamped everywhere including in the struct access. we probably need to widen the search/replace to catch example.bar and map it to example.dep__NAGA_OIL_MOD__bar as well.

i realise this is not a nice approach, but more sophisticated handling requires AST context, which we can't currently obtain without a complete/working shader as input.

@Runi-c
Copy link
Author

Runi-c commented May 2, 2024

That makes sense actually. It sounds like the current behavior is from #31? If there's already an exception carved out for identifiers following ., maybe an alternative here could be widening that exception to include identifiers preceding : (presumably, these are always struct members or function arguments)

@robtfm
Copy link
Collaborator

robtfm commented May 2, 2024

That sounds like a better idea.

@stefnotch
Copy link
Contributor

What about switch statements? They can also have a colon after a variable name.

switch foo {
  case bar: {       // Const-expression can be used in case selectors
    foo = 3;
  }
  default { 
    foo = 4;
  }
}

@robtfm
Copy link
Collaborator

robtfm commented Aug 3, 2024

Perhaps it’s best to explicitly disallow shadowing of modules with variables then.

Though we can’t easily prevent it in field names of imported structs …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants