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: Disallow mismatching native type when attaching to object #70956

Closed
wants to merge 1 commit into from

Conversation

vnen
Copy link
Member

@vnen vnen commented Jan 5, 2023

Note: This is a compatibility breakage so it might be too late to change. I did intend to send this quite some time ago but unfortunately there has been unforeseen circumstances. Still, I will open this for the discussion since the only uses for this "feature" are hacks and it does get in the way of somethings the GDScript compiler do to analyze and optimize scripts.

If this is accepted, the PR can be improved with error messages in the editor. We can also consider if this will be done for GDScript or for all languages.


This used to allow subtypes to be attached to an object but this is not really expected by the GDScript compiler. It assumes that the extends statement is exact, which allow for some optimizations and preemptive errors.

There's really no reason to allow different types, even if a subtype, since the only cases for this is to allow some hacks that could be avoided with better solutions.

This used to allow subtypes to be attached to an object but this is not
really expected by the GDScript compiler. It assumes that the `extends`
statement is exact, which allow for some optimizations and preemptive
errors.

There's really no reason to allow different types, even if a subtype,
since the only cases for this is to allow some hacks that could be
avoided with better solutions.
@timothyqiu
Copy link
Member

I usually change the type to a base class when creating UI scenes, especially when the node is a container. This allows me to change the container type without needing to modify the script.

@Zireael07
Copy link
Contributor

Same here

@vnen
Copy link
Member Author

vnen commented Jan 6, 2023

@timothyqiu this is also something that can have a better solution. Changing the node type could also change the type in the script automatically.

@ajreckof
Copy link
Member

ajreckof commented Jan 9, 2023

I think this should not be done as a lot of people use this functionality (me included) to make one script for a ton of different types to add a functionality to all of them.
Out of my head what I think of is this #46073
In this issue people stumble upon not being able to do it on abstract class.

@vnen
Copy link
Member Author

vnen commented Jan 11, 2023

I think this should not be done as a lot of people use this functionality (me included) to make one script for a ton of different types to add a functionality to all of them.

In general this is used a lot to inherit scripts and reuse the functionality across multiple nodes, which is essentially multiple inheritance. Since this isn't really supported and brings lots of troubles, forbidding would be better.

The only acceptable case I can see this being useful is to have a helper script that is attached to multiple nodes itself (e.g. a debug helper for Controls that use extends Control but is attached to buttons, labels, etc.). I do wonder if this has a more elegant solution that doesn't rely on the quirk of the system, because then we would be able to be more strict on the cases we don't intend to support.

This should still be avoided and the only reason it won't be dropped is because we don't yet have alternatives in place.

@Chaosus Chaosus added this to the 4.0 milestone Jan 23, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 24, 2023
@vnen
Copy link
Member Author

vnen commented Jan 28, 2023

This won't be done for now since we don't have a good alternative for the use cases. We can change to a warning once those are available to avoid breaking compatibility in future versions.

@vnen vnen closed this Jan 28, 2023
@vnen vnen removed this from the 4.x milestone Jan 28, 2023
@vnen vnen added the archived label Jan 28, 2023
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.

6 participants