Import under ifdef unexpected behavior

[arcashka]

imported module is being validated even though it should never be imported

fn main() -> u32 {
#ifdef USE_A
    return a::func();
#else
    return 0u;
#endif
}

#define_import_path a

fn func() -> u32 {
    incorrect;
}

    #[test]
    fn conditional_import2() {
        let mut composer = Composer::default();

        composer
            .add_composable_module(ComposableModuleDescriptor {
                source: include_str!("tests/conditional_import2/mod.wgsl"),
                file_path: "tests/conditional_import2/mod.wgsl",
                ..Default::default()
            })
            .unwrap();

        composer
            .make_naga_module(NagaModuleDescriptor {
                source: include_str!("tests/conditional_import2/top.wgsl"),
                file_path: "tests/conditional_import2/top.wgsl",
                ..Default::default()
            })
            .map_err(|err| println!("{}", err.emit_to_string(&composer)))
            .unwrap();
    }

results in

error: expected assignment or increment/decrement, found ';'
  ┌─ tests/conditional_import2/mod.wgsl:4:14
  │
4 │     incorrect;
  │              ^ expected assignment or increment/decrement
  │
  = expected assignment or increment/decrement, found ';'

I have a branch with this test here: https://github.com/arcashka/naga_oil/blob/failing_import_test/src/compose/test.rs#L1098

This example is silly looking because I tried to make it as minimal as possible. I found this issue when I used shader defs as an index for tonemapping lut bindings.

Conditional import

#ifdef TONEMAP_IN_SHADER
#import bevy_core_pipeline::tonemapping::tone_mapping
#endif

in tonemapping_shared.wgsl i had

@group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>;
@group(0) @binding(#TONEMAPPING_LUT_SAMPLER_BINDING_INDEX) var dt_lut_sampler: sampler;

which resulted in

error: expected expression, found '#'
   ┌─ /home/arcashka/Documents/projects/other/bevy/crates/bevy_core_pipeline/src/tonemapping/tonemapping_shared.wgsl:13:20
   │
13 │ @group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>;
   │                    ^ expected expression
   │
   = expected expression, found '#'

when TONEMAP_IN_SHADER is undefined

[robtfm]

This is more a bevy issue than a naga oil issue. I think building the module would work in raw naga oil without ever importing A. But in bevy for asset loading to be complete we need all possible dependents loaded, and your dependent doesn’t load successfully.

The obvious workaround is to put all of A behind the same directive.

[arcashka]

Sorry, I don't think I understand :(
I wrote a test with this scenario and it fails in naga_oil repository. It doesn't have bevy as a dependency according to Cargo.toml.
Or you are saying it behaves as it does because it's required for bevy?

[robtfm]

Ah sorry I misunderstood. And you’re right, it looks like we iterate and pull used items from all possible imports (which isn’t necessary in this case since there are none).

it would be a trivial fix to not try and load modules with no used items, but that would break virtual functions (no imported items would appear to be used so the module would be ignored). These might be broken anyway but I’d like to revive them if they are.

So we’d instead need to get preprocessor data with defines context … which is also a bit messy because modules can add shaderdefs themselves, and we can end up in an inconsistent state.

There is maybe a clean fix, but I wonder why you’re in this situation? Cant you remove the module import if it’s not valid, or fix it so it is?

[arcashka]

Thank you for fast responses! :)

For my situation it's a little more complicated. My module is valid when correct shader defs are provided. It uses them for binding indices.
So in my code I either provide these indices or hide import under ifdef. But hiding fails, and leads to an error

error: expected expression, found '#'
   ┌─ /home/arcashka/Documents/projects/other/bevy/crates/bevy_core_pipeline/src/tonemapping/tonemapping_shared.wgsl:13:20
   │
13 │ @group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>;
   │                    ^ expected expression
   │
   = expected expression, found '#'

There is more information regarding my scenario in the description. And you can also check PR where I encountered this behavior bevyengine/bevy#13262

Also, right now it's not really an issue for me, I managed to bypass it by moving bindings into nested module like this https://github.com/bevyengine/bevy/pull/13262/files#diff-49ec0e7e6216fe88b72aa04b3ef5684978a425f9afbd8f09c5b1c025440c163f
I just thought it's better to at least document the issue

[robtfm]

Ok that makes sense. Then you could wrap the binding declaration itself in an ifdef on the def for the binding index? I don’t think nesting achieves anything that you can’t do with a flatter structure.

The other alternative is to use a consistent binding across all the bevy use points, like 99 or something. Binding indexes don’t need to be sequential.

[arcashka]

You are right, wrapping bindings in ifdef's should work, but it has it's own issues related to some specifics. Right now I feel like nesting is the cleanest solution.
Consistent binding is also an option, but I tried to do bindings sequential based on https://discord.com/channels/691052431525675048/743663924229963868/1237084482780270655 😆