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 for not being able to ignore shadowing warnings on class scope #75620

Conversation

jpcerrone
Copy link
Contributor

Fixes #75208

Unifies the code for checking both the class scope and local scope shadowing of variables inside the 'is_shadowing' function from the gdscript analyzer. The class scope shadowing was being incorrectly checked during the parsing stage, and thus the annotations to ignore warnings were not being taken into account.

Adds two new scenarios to the shadowing unit test that check for a class level shadow warning. Validating that it correctly appears when there's no annotation present and that it doesn't when there's an annotation that ignores it.

Modifies another test that would start breaking with this change as it contained a variable that was shadowing a global one but wasn't being detected as such.

This is my first PR :) please let me know if I'm missing something!

@jpcerrone jpcerrone requested a review from a team as a code owner April 3, 2023 14:22
@YuriSizov YuriSizov added this to the 4.1 milestone Apr 3, 2023
@jpcerrone jpcerrone force-pushed the fix_shadow_warnings_not_going_away_after_ignoring branch from d8d95c6 to 68463bf Compare April 3, 2023 18:07
@jpcerrone jpcerrone force-pushed the fix_shadow_warnings_not_going_away_after_ignoring branch from 68463bf to 5a3c7f9 Compare May 3, 2023 15:17
@adamscott adamscott modified the milestones: 4.1, 4.2 Jun 19, 2023
@adamscott
Copy link
Member

Hi @jpcerrone! Sorry for the delay.
Thanks for your "first PR"! I saw that you finally contributed to the Godot game engine, so congratulations.

I set the milestone to 4.2, as we're close of launching 4.1. But I added your PR to the list of PRs that the team will have to review soon.

Thanks again for your contribution. You'll get news soon!

@jpcerrone
Copy link
Contributor Author

Hi @jpcerrone! Sorry for the delay. Thanks for your "first PR"! I saw that you finally contributed to the Godot game engine, so congratulations.

I set the milestone to 4.2, as we're close of launching 4.1. But I added your PR to the list of PRs that the team will have to review soon.

Thanks again for your contribution. You'll get news soon!

Thank you!!

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me! I have tested these changes, works fine. While I've noticed that shadowing warnings are not generated in some cases, that doesn't seem to be relevant to this PR. Note that the PR still needs to be reviewed at the GDScript team meeting. Also, this PR should be rebased so that CI will run tests for all platforms.

@YuriSizov YuriSizov requested a review from a team July 24, 2023 17:17
@jpcerrone jpcerrone force-pushed the fix_shadow_warnings_not_going_away_after_ignoring branch from 5a3c7f9 to 13c7350 Compare July 24, 2023 21:15
@jpcerrone
Copy link
Contributor Author

Looks good to me! I have tested these changes, works fine. While I've noticed that shadowing warnings are not generated in some cases, that doesn't seem to be relevant to this PR. Note that the PR still needs to be reviewed at the GDScript team meeting. Also, this PR should be rebased so that CI will run tests for all platforms.

Got it, branch rebased

@YuriSizov YuriSizov merged commit 0e1c953 into godotengine:master Jul 25, 2023
@YuriSizov
Copy link
Contributor

Thanks!

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.

Using @warning_ignore to ignore "shadowed_global_identifier" still issues warning(s)
4 participants