AlexTMjugador
commented
1 year ago While I think I do understand the issue, this seems like a rather complex case and I'm worried it could show up again later if refactor was to come.
Indeed, I had to scratch my head quite a bit to understand what was going on 😄
Do you think you could add a regression test for this issue so we make sure this is fixed and stays this way?
Sure, I could give it a shot! I didn't add a regression test because I wasn't sure how to go about it, given that the modified files don't contain any tests. What's the rough outline of how you'd like such test to look and where would you put it?
alixinne
As of now, injecting preprocessor directives found in included files back into the AST when using the
v2-fulllexer is not supported. Attempting to do so will trap theDirectives::injectmethod in the following loop forever, as a different source file ID will cause neitherdirective_idxto be incremented orbreakto be reached:https://github.com/vtavernier/glsl-lang/blob/881c0e472bfa990150c7a3fcd8b3efc787b66ccc/lang-lexer/src/v2_full/directives.rs#L134-L158
After giving the situation some thought, I think it's more appropriate to inject the preprocessor directives that we are currently capable of injecting, regardless of whether they come from a different file:
#pragma: most of these directives are to be defined by the GLSL compiler implementation, and those documented in the GLSL specification change the compiler state in a way where only their location in the source is relevant, not the file they were expanded from.#extension: similar to#pragma. Furthermore, the specification explicitly states that "directives that occur later override those seen earlier".#version: the GLSL specification states that "the #version directive must occur in a shader before anything else". This suggests (but does not unequivocally assert) that there should only be one#versiondirective per shader.By now, I think it is clear that
#pragmaand#extensioncan be injected as they are without further ado. However, handling#versionin combination with#includerequires further work, in my view.Let's consider a shader A that is valid on its own, but does not contain a
void main()function (this is allowed by the GLSL specification, as long as the shader is linked together with another shader that defines this function). Now, consider a shader B that#includes shader A and definesvoid main(). Let's assume that the GLSL application compiles shader B. In this scenario, I'd argue that it makes sense for both A and B to have a#versiondirective in the beginning of their respective sources, because both are shaders, according to the specification. A could define avoid main()function only if a preprocessor directive set by B before the#includeis unset, allowing it to be linked as a completely standalone shader, where the#versiondirective can't be placed anywhere else. Remarkably, the specifications for#includeextensions likeARB_shading_language_includedo not mention how to handle multiple#versiondirectives at all.Given the above scenario, I think that a good solution is to make sure that
Directives::injectonly emits at most one#versiondirective, with its version and profile set to the highest version and most featureful profile among those specified in all#versiondirectives. As far as I know, GLSL versions are backwards compatible, and even if they weren't, several#versiondirectives are likely to violate the GLSL specification, so there would be no way to reconcile GLSL sources targeting incompatible GLSL versions anyway.These changes let
#pragmaand#extensiondirectives coming from included files be injected anyway.#versiondirectives are injected too, but only one#versiondirective is ever injected, according to the technique stated in the previous paragraph.(By the way, I appreciate your prompt PR reviews. As you can see, I'm taking these crates for a good spin, trying to improve what I find in the process. I think that Rust deserves a rock-solid GLSL parser library :heart:)