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

GDScript: Don't highlight unexposed classes #98169

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

HolonProduction
Copy link
Member

Fixes #19834

@dalexeev
Copy link
Member

I'm not sure this makes sense. Yes, the user can't use unexposed classes, but the user also can't define a custom class with that name, so in my opinion it makes sense to highlight it. Maybe a different color, though that's probably overkill.

@HolonProduction
Copy link
Member Author

Using those names as class_name seems to work just fine, GDNative class name restrictions don't really seem relevant to GDScript highlighting IMO

@dalexeev
Copy link
Member

@HolonProduction
Copy link
Member Author

Now I wonder why it works with class_name 🤔 Seems a bit inconsistent

@HolonProduction
Copy link
Member Author

image
Hmmmm I don't know what I did but all of a sudden it does not produce the error anymore

@dalexeev
Copy link
Member

Nevermind, this is a bug in my test project. To some extent, it should work, because we have the following check:

bool GDScriptAnalyzer::class_exists(const StringName &p_class) const {
return ClassDB::class_exists(p_class) && ClassDB::is_class_exposed(p_class);
}

But still, naming a class or other identifier as a native class (whether it's exposed or not) is a bad idea, the user risks encountering strange bugs, since scopes and shadowing are not implemented very well.

@HolonProduction
Copy link
Member Author

To be honest I'd just emit a warning if a user is doing that, and keep it out of the highlighting. Users won't understand the implication behind it just from highlighting, and if there is a warning I don't feel an extra highlight color would add much value.

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.

To be honest I'd just emit a warning if a user is doing that, and keep it out of the highlighting. Users won't understand the implication behind it just from highlighting, and if there is a warning I don't feel an extra highlight color would add much value.

I think this makes sense. As far as I understand, the current assumption is that shadowing unexposed classes should work (and it does work to some extent, despite possible issues). If we decide to limit this, then we shouldn't rely on syntax highlighting alone. And we can always revisit syntax highlighting in the future.

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Oct 24, 2024
@Repiteo Repiteo merged commit 3c7fb9f into godotengine:master Oct 24, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 24, 2024

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.

Script editor indicates certain classes exist that don't seem to be there
4 participants