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

Don't allow weaker return type in overriden method #79765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 21, 2023

This code

class Parent:
	func override_me() -> bool:
		return true

class Child extends Parent:
	func override_me():
		pass

should result in an error, but it didn't.

EDIT:
This breaks methods that return Variant, I'll try to fix it xd

EDIT2:
Fixed and added more tests.

EDIT3:
I added another case for void, but it's rather questionable now 😬

@KoBeWi KoBeWi added this to the 4.2 milestone Jul 21, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner July 21, 2023 21:53
@KoBeWi KoBeWi force-pushed the weakness_disgusts_me branch 4 times, most recently from 8fb3b0c to ac66ebd Compare July 21, 2023 22:25
@dalexeev
Copy link
Member

dalexeev commented Aug 7, 2023

We should also remember about overriding native virtual functions. In this case we should make an exception and allow not specifying the return type (which is treated as Variant by default), for compatibility and convenience of users of untyped GDScript. However, an explicit less specific return type should not be allowed, in my opinion.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 7, 2023

In this case we should make an exception and allow not specifying the return type (which is treated as Variant by default), for compatibility and convenience of users of untyped GDScript.

This is already handled by the latest changes.

But can be more specific (covariance).

Yeah, but this is bugfix PR, so more specific type can be implemented later.

@YuriSizov
Copy link
Contributor

@dalexeev How does it look now in your opinion? Probably too risky for 4.2 (regressions in GDScript are known), but would appreciate an approval if this is otherwise good.

@KoBeWi Needs a rebase at some point though 🙃

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Oct 30, 2023
@dalexeev dalexeev self-requested a review October 30, 2023 17:56
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 16, 2024

In this case we should make an exception and allow not specifying the return type (which is treated as Variant by default), for compatibility and convenience of users of untyped GDScript.

This is already handled by the latest changes.

Actually I misunderstood, you are describing the example I specifically showed as undesired. If this needs to be allowed, it should result in a warning at least.

@vnen
Copy link
Member

vnen commented Apr 18, 2024

I see two conflicting problems:

  1. GDScript should still be dynamically typed, so it shouldn't be forcing users to add types anywhere.
  2. There are optimizations based on type information, skipping runtime validation. If some script breaks this contract it may lead to crashes.

Not entirely sure right now how to deal with this conflict.

One potential solution is to treat the overridden method as if it has the same type as the original one if not specified. It may give some type errors even if the user isn't using types, but those would be technically correct. Although it would be tricky to inform the user about the inherited return type.

I believe erroring like this would be annoying for someone who prefers to no use static types. If they inherit from an addon which does use types, they will get these errors, when types are supposed to be optional.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 18, 2024

Hence this could be a warning.
I opened this PR because a couple of times I forgot that a method I'm overriding is supposed to return something and it led to non-obvious bugs.

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.

GDScript overriden functions can override return type
5 participants