Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't access parent's named enum from match statement #37631

Open
rsubtil opened this issue Apr 6, 2020 · 12 comments
Open

Can't access parent's named enum from match statement #37631

rsubtil opened this issue Apr 6, 2020 · 12 comments

Comments

@rsubtil
Copy link
Contributor

rsubtil commented Apr 6, 2020

Godot version: 3.2.1

OS/device including version: Arch Linux (kernel 5.5.13)

Issue description: Godot throws an error when I try to access a named enum from a parent inside a match statement. If I change to be a classic if/elif/else block, it works fine.

The error is built-in:8 - Parse Error: Invalid operator in pattern. Only index (A.B) is allowed, which suggests that Godot won't allow to access a field more than once.

Looks related to #10571

Steps to reproduce: Have a scene tree like:

Parent
└── Child

Parent.gd:

extends Node

enum Sides {
	LEFT,
	RIGHT
}

var currentSide = Sides.RIGHT

func _ready():
	$Child.foo(currentSide)

Child.gd

extends Node

func foo(side):
	match side:
		$"..".Sides.LEFT: # <--- Errors here
			print("Left side")
		$"..".Sides.RIGHT: # <--- and here
			print("Right side")

Minimal reproduction project: project.zip

@Zireael07
Copy link
Contributor

Just wondering, does it work if you move $".." or $"..".Sides to a var before match?
Like that:
foo
var en = $".."
match side:
en.Sides.LEFT:

@KoBeWi
Copy link
Member

KoBeWi commented Apr 6, 2020

Just wondering, does it work if you move $".." or $"..".Sides to a var before match?

It does. This does too:

	var left = $"..".Sides.LEFT
	
	match side:
		left:
			print("Left side")

@ThakeeNathees
Copy link
Contributor

ThakeeNathees commented Apr 7, 2020

I don't think this is a bug, the match pattern can't be a function call
(it's more like a switch statement, not a bunch of if else)

match side:
    f():  ##  <-- invalid
        print("f() matched")

and $"path" is a syntax sugar for get_node("path") so you're trying to do
something like this

match side:
    get_node("..").Side.LEFT:  ##  <-- also invalid
        print("Left side")

@rsubtil
Copy link
Contributor Author

rsubtil commented Apr 7, 2020

@ThakeeNathees I understand what you mean. If I move only the function call to a separate var (what @Zireael07 suggested), it works:

func foo(side):
	var node = $".."
	
	match side:
		node.Sides.LEFT:
			print("Left side")
		node.Sides.RIGHT:
			print("Right side")

But in that case, couldn't there be a way to evaluate a function at compile-time, to allow cases such as these to work?

@ThakeeNathees
Copy link
Contributor

ThakeeNathees commented Apr 7, 2020

the match pattern can't be an expression, because they're only labels for compiler to jump to the next address

unlike switch case (compile time evaluable pattern) the gdscript match statement evaluates pattern at at runtime ( like runtime constants) and all it does is check if it's equal and jump to the next address.

switch (val_1) {
    case 1: do_something(); break;
    case 2: do_something_else();
}

// is equal to 

goto case_1; // runtime
label case_1:
    do_something();
    goto case_break;
label case_2:  // <--- just a label, can't use expressions here
    do_something_else();
label case_break;

but some match pattern (binding pattern , array pattern) allow to declare a var and assign the match value inside the match statement scope, which is a syntax sugar for a default case declare and assign the match value immediately inside.
@ev1lbl0w

@rsubtil
Copy link
Contributor Author

rsubtil commented Apr 7, 2020

but some match pattern (binding pattern , array pattern) allow to declare a var and assign the match value inside the match statement scope, which is a syntax sugar for a default case declare and assign the match value immediately inside.

In that case, wouldn't it be possible to do the same "tricks" for this use-case? If the expression is evaluated at run-time, could a NodePath also be evaluated before-hand?

I think this would be useful, since the match already has a very "dynamic" usage/feel when compared to regular switch expressions, so it could also benefit from this.

But if what I'm asking is, in practice, very hard to implement for the current code structure, I can just use the working sample (it's not pretty, but it isn't that ugly either) and close this.

@adamscott
Copy link
Member

In Godot 4, this is errors out by design with the following error:

Expression in match pattern must be a constant expression, an identifier, or an attribute access ("A.B").

The problem here is the fact that $".." is dynamic, and typing cannot be inferred. But, what you can do, is tell GDScript that the getNode command is of some type.

extends Node

const Parent = preload("./parent.gd")

func foo(side):
	match side:
		($".." as Parent).Sides.LEFT:
			print("Left side")
		($".." as Parent).Sides.RIGHT:
			print("Right side")

So, I suggest that for 3.x, it would be a "won't fix" and we could close this issue for 4.x as "as designed".

@KoBeWi
Copy link
Member

KoBeWi commented Aug 28, 2023

($".." as Parent) can be simply Parent.

@dalexeev
Copy link
Member

I think that even if the example in this issue is unsuccessful, in general this is a valid observation. I see no reason for the restrictions, in my opinion a pattern should be allowed to be any dynamic expression.

@adamscott
Copy link
Member

This issue is then linked to godotengine/godot-proposals#2337

@rsubtil
Copy link
Contributor Author

rsubtil commented Aug 28, 2023

My reasoning at the time was because this works properly with if/elif/else, and I was assuming the match statement in GDScript wasn't fundamentally different to it. If the match statement is optimized in GDScript similarly to what compiled languages do (jump tables iirc), then I understand how this use case can be difficult or impossible to account for.

Regardless, the suggested workaround works, so I don't have a particularly strong opinion whether this should be supported or not. Whether to support this or not, I'm fine with the decision you'll make 🙂

@dalexeev
Copy link
Member

If the match statement is optimized in GDScript similarly to what compiled languages do (jump tables iirc)

No, currently match is not optimized (there is a draft PR #78742 that I want to continue working on after #80085 is merged). But this optimization does not interfere with dynamic patterns and/or pattern guards, just match with non-constant patterns will not get the optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants