godotengine / godotengine/godot
GDShader has several syntax issues when you `#define` function-like macros
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
- Reproducible in v4.3.stable.flathub [77dcf97d8]
### System information
Godot v4.3.stable (77dcf97d8) - Freedesktop SDK 23.08 (Flatpak runtime) - X11 - Vulkan (Forward+) - integrated Intel(R) HD Graphics 5500 (BDW GT2) - Intel(R) Core(TM) i5-5300U CPU @ 2.30GHz (4 Threads)
### Issue description
I've identified several issues when defining macros with arguments in GDShader preprocessor.
Issues are confirmed by comparing expected behavior to GLSL (ShaderToy / glslang) and C Preprocessor (GCC).
- [ ] 1. Preprocessor must report error on duplicate parameter names in macros
- [ ] 2. Preprocessor should leave names of function-like macros alone when used without parentheses
- [ ] 3. Preprocessor should allow function-like macros with no parameters (empty parentheses)
- [x] 4. Preprocessor should not replace parameter names inside strings when expanding macros - *(won't fix)*
- [ ] 5. Macro expansion could disallow recursion to avoid issues
- [ ] 6. Expand macro itself before (not after) its arguments
- [ ] 8. Replace macro arguments all at once, not sequentially
- [ ] 9. Forbid defining or undefining a macro named "defined"
#### Issue 1: Report duplicate macro parameters
GLSL in ShaderToy gives the following error in this case:
> 'myParam' : duplicate macro parameter name
In the C preprocessor (GCC) the error looks like this:
> test.c:1:14: error: duplicate macro parameter "myParam"
#### Issue 2: Leave function-like macro name alone when used without parentheses
There is another inconsistency between GDShader and GLSL/C preprocessors.
If you define a function-like macro with parentheses, GLSL and C will still allow that name to appear without parentheses and leave it alone (it won't try to expand it). GDShader, however, doesn't do this, and its way of handling it causes issues (not entirely sure to what extent, but I found a very weird issue in my tests).
#### Issue 3: Support function-like macros with zero parameters
GLSL replaces `macroName()` (no arguments, but including the parentheses) when it's defined with parentheses, and just `macroName` if it's defined without parentheses. I tested the C preprocessor (GCC) and it works the same way.
This should be allowed in GDShader as well.
#### Issue 4: Do not expand macro parameters defined inside strings
Strings are not supported in GLSL, but since they are going to be supported in GDShader, their handling in the preprocessor should match C. I assume it's still an issue, but note that I didn't test this against master (please confirm).
Parameters in macros are replaced wherever they appear in their definition. But if they appear inside a string, it should of course not be replaced. That's how the C preprocessor handles it.
#### Other issues: see comments below
### Steps to reproduce
Hint: Using an invalid token like `$` logs the preprocessor output (even on otherwise "valid" code) to help testing.
`issue1.gdshader`
```glsl
shader_type spatial;
#define join(x, x) x ## x
// it seems to be simply using just the first parameter name
// it must raise a preprocessor error like "duplicate macro parameter name"
const int join(a,b) = join(1,2); // becomes: `const int aa = 11 ;`
$ // voluntary error to log preprocessor output
```
`issue2.gdshader`
```glsl
shader_type spatial;
#define bar(x) x ## x
const int a = bar a b / c d bar(12);
// incorrectly results in this code: `const int a = 1212 ;`
// no idea what the preprocessor is doing here; ignoring everything between macro name and `(` perhaps?
// should be: `const int a = bar a b / c d 1212 ;`
$ // voluntary error to log preprocessor output
```
`issue3.gdshader`
```glsl
shader_type spatial;
#define foo() whatever()
// raises "invalid argument name" error
// empty parentheses should be allowed; this form must require parentheses to expand it
foo foo() foo // should become: foo whatever() foo
```
`issue4.gdshader`
```glsl
shader_type spatial;
#define str(x) "x"
uniform int a: hint_enum(str(etc)); // should be "x", not "etc"
$ // voluntary error to log preprocessor output
```
### Minimal reproduction project (MRP)
N/A
Contributor guide
Research direction
Start with the GDShader preprocessor and run the supplied issue1.gdshader, issue2.gdshader, issue3.gdshader, and issue4.gdshader reproductions, using the logged output to compare behavior with GLSL and GCC. Done means the unchecked duplicate-parameter, function-like macro, expansion-order, recursion, argument-replacement, and defined-name cases behave as specified, while the string case remains excluded as marked won't fix.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100