fix(lang-lexer): fix directive injection from multiple files #38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ltiple files As of now, injecting preprocessor directives found in included files back into the AST when using the `v2-full` lexer is not supported. Attempting to do so will trap the `Directives::inject` method in the following loop forever, as a different source file ID will cause neither `directive_idx` to be incremented or `break` to 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 `#version` directive per shader. By now, I think it is clear that `#pragma` and `#extension` can be injected as they are without further ado. However, handling `#version` in combination with `#include` requires 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 `#include`s shader A and defines `void 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 `#version` directive in the beginning of their respective sources, because both are shaders, according to the specification. A could define a `void main()` function only if a preprocessor directive set by B before the `#include` is unset, allowing it to be linked as a completely standalone shader, where the `#version` directive can't be placed anywhere else. Remarkably, [the specifications for `#include` extensions like `ARB_shading_language_include`](https://registry.khronos.org/OpenGL/extensions/ARB/ARB_shading_language_include.txt) do not mention how to handle multiple `#version` directives at all. Given the above scenario, I think that a good solution is to make sure that `Directives::inject` only emits at most one `#version` directive, with its version and profile set to the highest version and most featureful profile among those specified in all `#version` directives. As far as I know, GLSL versions are backwards compatible, and even if they weren't, several `#version` directives 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 `#pragma` and `#extension` directives coming from included files be injected anyway. `#version` directives are injected too, but only one `#version` directive is ever injected, according to the technique stated in the previous paragraph.
…injected at wrong positions Preprocessor directives from included files have different span positions than directives in the top-level shader file. So a included preprocessor directive may span from 0 to 100, while the #include directive in the top-level file ends at position 50, which may cause the included preprocessor directive to be injected after the first external declaration that follows the #include directive that starts after position 100, not at the #include position. To fix that, bring back the source ID to the checks, relying on previous include directive events to output the child directives at just the right time.
alixinne
force-pushed
the
fix/pr-24-with-tests
branch
from
5bf71a6
to
60c2016
Compare
alixinne
force-pushed
the
fix/pr-24-with-tests
branch
from
60c2016
to
df5f33c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #24 for the actual fix and discussion, this only adds a few tests.