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

Check if any global script class is shadowed by a variable #80587

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

garychia
Copy link
Contributor

@garychia garychia commented Aug 13, 2023

@garychia garychia requested a review from a team as a code owner August 13, 2023 09:38
@dalexeev dalexeev added this to the 4.2 milestone Aug 13, 2023
@dalexeev dalexeev added bug and removed enhancement labels Aug 13, 2023
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.

Looks great to me! Make sure to use keywords that github understands, like directly say Fixes #<pr-number>, which will autolink it :)

@dalexeev
Copy link
Member

I haven't tested, just guessing. Can you get a false positive warning when reloading the script in the editor if the class name is already in the global script cache?

@anvilfolk
Copy link
Contributor

Oh, good point. Let me try.

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.

I couldn't get the warning to fire when it shouldn't. This is an easy one to revert should problems crop up, so I'd say I still want to merge!

However, I'd actually like to see improved error handling, like so:

  1. The current warning should explicitly say it's a native class, because I think what ClassDB contains?
  2. We should add another case for the GDScript/scriptserver warning, which should also actually expose what file the class is defined in using ScriptServer::get_global_class_path(class_name). That way we easily know where the conflict is coming from!

@anvilfolk
Copy link
Contributor

Also, I notice you have quite a few PRs but often say related to instead of fixes/closes in them - feel free to edit those so that GH actually links the issues!

And you'd be very welcome in the Godot contributors chat if you wanted to discuss stuff with other godot devs!

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.

Just a small change more so the code is consistent, then I think it's good to merge :)

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
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.

Looks great!!! :)

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 10c3941 into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@garychia garychia deleted the shadowed_class_name branch September 26, 2023 12:52
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.

Variable name shadowing class_name class doesn't trigger any error/warning
4 participants