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

fix access to identifiers that are reserved keywords #62830

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Jul 8, 2022

Fixes #61981

All reserved keywords listed here were interpreted as tokens and returned false when tested with is_identifier(). So i made it possible to indicate that a token can be interpreted as an identifier whatever is type. Then in parse_attribute when we know we want exclusively identifiers i marked keywords as identifiers.

@ajreckof ajreckof requested a review from a team as a code owner July 8, 2022 06:25
@akien-mga akien-mga added this to the 4.0 milestone Jul 8, 2022
@ajreckof ajreckof force-pushed the access-identifier-keywords branch from d1e8bc0 to 9440216 Compare July 8, 2022 22:04
@ajreckof ajreckof requested a review from akien-mga July 8, 2022 22:15
@ajreckof
Copy link
Member Author

ajreckof commented Jul 8, 2022

I just realised that instead of creating a variable to mark them as identifier i could directly change the type of the token as the function that gets the identifier from the token isn't using the type but another variable which store the string directly.
Changing the type will make clearer code as it will make only changes in the get_attribute function

@ajreckof ajreckof force-pushed the access-identifier-keywords branch from 9440216 to 871180b Compare July 8, 2022 23:07
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 13, 2023
@akien-mga akien-mga requested a review from vnen April 5, 2023 18:55
@anvilfolk
Copy link
Contributor

I've never fooled around with the tokenizer so I don't feel confident reviewing :)

Couple of thoughts though!

  1. Is there a chance you could add a couple of tests that showcase this working and so the same error doesn't creep back up later?
  2. Might there be a situation where something that should be a reserved name can now become just an identifier and cause a bug? Or is that just not possible?

@ajreckof
Copy link
Member Author

ajreckof commented Apr 7, 2023

  1. Is there a chance you could add a couple of tests that showcase this working and so the same error doesn't creep back up later?

That seems like a super good idea I'll try to make some but i don't know how to make one so I'll have to look into it.

  1. Might there be a situation where something that should be a reserved name can now become just an identifier and cause a bug? Or is that just not possible?

If it reaches this point and it is not interpreted as an identifier it will just raise an error at the next line. That's why we can here safely modify it.

@anvilfolk
Copy link
Contributor

I tried looking through the source code some more to try to give this a review but it's a little too much for me atm :)

The added test looks good though! From now we can forever make sure that you can use reserved keywords as dictionary keys in Lua-style accessors!

Copy link
Contributor

@anvilfolk anvilfolk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I ran this in debug mode and I do believe that it makes sense! :)

The function changed is already specifically for attribute lookups. The changes make sure that if what comes after the . is basically text (the call to is_node_name()), then it should be interpreted as an identifier. Again, only specifically within the context of an attribute lookup, i.e. base.attribute.

Anything else that isn't text, when is_node_name() returns false, will still raise an error due to the consume function wanting to see an identifier there.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by the GDScript team on a PR review meeting.

Thanks for your contribution!

(as discussed, the PR will be ready when more cases will be added to the test)

@YuriSizov YuriSizov marked this pull request as draft April 14, 2023 18:02
@YuriSizov
Copy link
Contributor

(as discussed, the PR will be ready when more cases will be added to the test)

Marked as a draft until then. Feel free to mark as ready when appropriate.

@ajreckof ajreckof marked this pull request as ready for review April 14, 2023 20:07
@adamscott
Copy link
Member

It's ready to review! Hurray! 🎉

@YuriSizov YuriSizov merged commit d220680 into godotengine:master Apr 17, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@ajreckof ajreckof deleted the access-identifier-keywords branch April 17, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't access keyword identifiers on a Dictionary
5 participants