GDShader parser should give errors in incorrect usage of `const` and certain array type declarations

geekley commented 3 months ago

Tested versions

Reproducible in:

v4.2.2.stable.flathub [15073afe3]
v4.0.4.stable.mono.official [fc0b241c9]

In Godot 3, some constructs are not allowed, so it seems to only have the const function issue. So, partially reproducible in:

v3.5.3.stable.official [6c814135b]
v3.3.stable.official

System information

Godot v4.2.2.stable (15073afe3) - 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

The Shader code below should give errors of various types, but it's being parsed as valid.

Problems:

[x] Functions allowing const qualifier before return type
[x] Array of void being allowed at all
[ ] Local variables of implied array size allowing non-arrays indirectly

buggy.gdshader

shader_type canvas_item;

// Allowing const here has to be a bug; it doesn't seem to mean anything.
const int const_returned() { return 0; }

// Adding const even causes an error to not trigger. Without const, this gives ERROR:
// "Unknown array size is forbidden in that context".
const bool[] unknown_array_return_being_allowed_with_const() { return true; }

// It's weird that such "unknown array size" allows non-arrays too.
bool bool_fn() { return true; }
uniform int int_var;
void unknown_array_vars_allowing_non_arrays() {
    bool[] a = bool_fn();
    int b[] = int_var;
    // This seems to be a bug, as the line below is an error:
    //bool[] c = true; // Gives ERROR: Expected a '{'
    // Is this form allowed in other contexts, apart from local vars? If so, I didn't test.

    // In fact, why is this form even allowed at all? It's better to always require the size.
    // If functions, fields, etc. need to specify size, then it's always known statically, no?
    // It's not useful even as syntatic sugar, as you cannot mix different sizes anyway.
    //int[] d = {2}, e = {3, 4}; // Gives ERROR: Array size mismatch
    // If the purpose is to allow specifying values without having to count them manually,
    // then I suggest instead telling the expected count in the error message
    //int[1] f = {1, 2, 3}; // Could be ERROR: Array size mismatch, expected int[3]
}

// I guess an array of nothings is also nothing. Still, there's no reason to allow it.
// Expected ERROR like "Type void cannot be an array" or "Expected function identifier".
void[2] nothings() { return; }

Steps to reproduce

Simply create a Shader file with the code above and note it shows no errors in Godot's Shader code editor. I did not make any tests whatsoever involving Visual Shader, I don't know if it's applicable.

Minimal reproduction project (MRP)

N/A

I was told to report the bug by @Chaosus on devel chat.

Chaosus commented 3 months ago

In fact, why is this form even allowed at all?

Because it's comfortable to declare and define, as you stated. And it's indeed a syntax sugar. Changing it back to require a size will cause a huge compatibility breakage for the user shaders.

int[] d = {2}, e = {3, 4}; // Gives ERROR: Array size mismatch

This may be a subject to fix.

then I suggest instead telling the expected count in the error message

Will be fixed in https://github.com/godotengine/godot/pull/93822

I guess an array of nothings is also nothing. Still, there's no reason to allow it.

I've fixed it in https://github.com/godotengine/godot/pull/95276, interesting that It's still a valid GLSL code.

geekley commented 3 months ago

Thanks, @Chaosus . I have several questions about shader arrays.

So, are arrays in GDShader always more like structs containing just the data, and statically known size (implied, not stored in a field at runtime)? Instead of more like a pointer to data + length field with dynamic size? Because since .length() "method" exists, I'd assume it's most useful for the latter, meaning that implementation could be possible on size-less declarations like int[], also allowing it on return values, parameters, etc. so you could have e.g. functions that deal with sizes dynamically (similar to languages like C# and Java).

But since it's not supported (I think) then does that mean arrays can never have dynamic sizes? I guess this makes sense if you can't dynamically allocate like malloc/new. Or am I wrong and can it have dynamic size somehow (just an example) on something like void my_func(int size, float[size] array) {...} (I can't remember if C allows something like this)? Or perhaps "semi-dynamic" values like:

void my_func(int size) {
  float[size] array = {...}; // somehow initialize all to zero or whatever
}

Also, what could be the max for arrays roughly speaking? I.e. something closer to e.g. 255 or to MAX_INT32? Is it related to stack size? GPU-dependent? How does that work?

geekley commented 3 months ago

@akien-mga @Chaosus I believe one of the bugs in the checklist is still unresolved, with no PR for it. Can you reopen?

Local variables of implied array size allowing non-arrays indirectly

You shouldn't be able to do this:

bool[] a = function_returning_non_array_bool();

Or was this fixed too?

godotengine / godot