Report missing import error

[IceSentry]

Objective

If an import is missing from the module it currently just unwraps and panics without any explanations.

Solution

Don't unwrap and bubble up the error instead.

The error reporting is a bit weird because it just points at the start of the file and I'm not sure how to improve it.

[robtfm]

Could you share a reproduction of the issue? Missing imports should be handled already.

[IceSentry]

I hit that bug while testing the SSR PR, but I also hit it at work and had no easy way to even diagnose it.

If you pull this commit, bevyengine/bevy@383e87e (#13418) (the commit itself isn't the issue, it's just the one right before the fix) and run the transmission example you'll see this error message:

thread 'Async Compute Task Pool (3)' panicked at C:\Users\Charles\.cargo\registry\src\index.crates.io-6f17d22bba15001f\naga_oil-0.13.0\src\compose\mod.rs:1330:59:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'Async Compute Task Pool (0)' panicked at crates\bevy_render\src\render_resource\pipeline_cache.rs:734:60:
called `Result::unwrap()` on an `Err` value: PoisonError { .. }
thread 'Async Compute Task Pool (2)' panicked at crates\bevy_render\src\render_resource\pipeline_cache.rs:734:60:
called `Result::unwrap()` on an `Err` value: PoisonError { .. }

The first error is related to an unwrap in naga_oil and this is what this PR is attempting to fix.

Essentially, the issue is that in the SSR PR (before it was fixed) when transmission is used without checking for the ENVIRONMENT_MAP shader_def, it tries to read the environment_map import that doesn't exist since it's behind the shader_def and that's when things blow up. I don't know how it reached that point, but it does and it gives a very cryptic error with no explanation as to what is going on.

[robtfm]

ok thanks, i reproduced it.

i think the problem here reduces down to

#ifdef USE_A
    #import a::b
#endif

fn main() -> u32 {
    return b::C;
}

for this, get_preprocessor_data claims that the total set of potential imports are [ImportDefinition { import: "a::b", items: ["C"] }]

but that's not true since if USE_A is not specified, we actually want a module called b.

i wonder if it is possible / desirable to make it so that this explicitly reports that b is a required dependency, but

#ifdef USE_A
    #import a::b
#endif

fn main() -> u32 {
#ifdef USE_A
    return b::C;
#else
    return 0;
#endif
}

would still report only a::b as required... i think this would probably be decently better for catching issues like this one at compile-time preprocess / asset-load time.

[robtfm]

after thinking about it a bit i think while contextually resolving the required imports would be better, it's not easy.

it would require

• capturing the def-state of each import (i.e. what defs need to be defined/undefined/match a given comparison, for the import statement to apply)
• capturing the def-state of each usage of an import
• check if the usage def-state overlaps with the def-state for each import that it could refer to, and marking the import as a dependency if so
• checking if the usage def-state is fully contained within the set of states for all the imports that it could be referring to, and adding it raw (i.e. adding b as a dependency in the above example) if not

.. which is not totally straightforward and might be pretty slow (a lot of cloning of def-states and non-trivial intersection / containment tests). so i think your approach is good for now.

ideally we'd avoid cloning the source strings on the happy path (they can be big), but i can't see a clean way to do that. we could put a dummy string in for the source, and check the path in a map_err then replace the source if it matches, or pass a closure into ensure_import or something, but these are both pretty ugly and criterion says it has no impact on the pbr_compose_test example, so maybe not worthwhile.

i also noticed that my small example above still fails with this pr, because the imports that were ensured at the top-level were based on the preprocessor metadata instead of the preprocess data (which uses the shader-defs). i fixed that by moving the preprocess step up a few lines, and added a couple of tests on a pr to your pr. it passes all the builtin tests but i will check on bevy as well since this is a slightly more invasive change (shouldn't break anything but just to be sure).

[IceSentry]

ideally we'd avoid cloning the source strings on the happy path

Yeah, I don't like it either, but I wasn't sure how to handle it because of the recursive case. I'll merge your PR and see if I can figure out something better.

[IceSentry]

Okay, I changed the error handling a bit to just bubble up the import name until it needs to be converted to a ComposerError. It's a bit annoying that I'm forced to create an intermediary enum for that, but now it avois the clone until it's actually needed.

One nice side effect is that now it reports at the place where that import is trying to be used which makes it much clearer. In the nested case though, it points the #import middle line in top_nested instead of pointing to the file that has the missing import. It's not ideal, but it's not too bad either I think.

[robtfm]

this approach is much nicer. i'll approve when the last point is addressed.