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

Support covariant return types of overriding functions #80457

Closed

Conversation

bitbrain
Copy link
Contributor

@bitbrain bitbrain commented Aug 9, 2023

Implements proposal godotengine/godot-proposals#7450

This pull request aims to implement covariant support for return types of overriding functions.

Bugsquad edit:

@bitbrain bitbrain requested a review from a team as a code owner August 9, 2023 19:43
@bitbrain bitbrain force-pushed the support-covariant-return-types branch 3 times, most recently from 3ddd91d to 7e0f4a0 Compare August 9, 2023 20:48
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 don't completely understand covariant types and am too sleepy to dig into it right now, but pressed review by mistake so here we are! 😅 I looked through the code and saw a couple of things that probably could use changing, so I marked them out.

Also, it feels like this logic ought to be applicable to more than function return types? Could the new GDScriptAnalyzer::is_super_type() function be used elsewhere? I feel like I've seen this code somewhere else, maybe in assignments?

I also have a question about what would happen with:
C extends B extends A, and A.greetings() -> BaseClass, B.greetings() -> SubClass but then C.greetings() -> BaseClass again. It should error because the return of C.greetings() is not the same or subclass of the return of B.greetings(), right?

modules/gdscript/gdscript_analyzer.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member

I don't think we need the is_super_type() method at this point and in this form. This brings up the subtle question of the difference between type compatibility and type inclusion (implicit conversions, whether Nil is a subtype of Object, metatypes). Even if we add the method, it is actually useful in a relatively small number of cases (the is operator), in most cases we should still use is_type_compatible().

I will try to refactor is_type_compatible() so that it can be used in this PR.

@bitbrain bitbrain force-pushed the support-covariant-return-types branch 2 times, most recently from 097f080 to b0119d6 Compare August 10, 2023 12:34
@bitbrain
Copy link
Contributor Author

I also have a question about what would happen with:
C extends B extends A, and A.greetings() -> BaseClass, B.greetings() -> SubClass but then C.greetings() -> BaseClass again. It should error because the return of C.greetings() is not the same or subclass of the return of B.greetings(), right?

@anvilfolk thank you - I have added this test case to verify that it fails correctly: https://github.com/godotengine/godot/pull/80457/files#diff-918b56d949925f87a6f93c202553fabea909d3287eaf34348d8d55df264c5b27R2

@bitbrain bitbrain force-pushed the support-covariant-return-types branch from b0119d6 to f2d5d19 Compare August 10, 2023 19:45
@bitbrain bitbrain force-pushed the support-covariant-return-types branch from f2d5d19 to 8e40bbc Compare August 10, 2023 20:13
@adamscott adamscott modified the milestones: 4.x, 4.2 Sep 26, 2023
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Discussed during our GDScript team meeting. This feature would be great for the language and we think it should make it to the 4.2 release. Thanks @bitbrain !

@adamscott
Copy link
Member

We should check if it works with the builtin types, just for safety mesures.

@vonagam
Copy link
Contributor

vonagam commented Sep 26, 2023

I see a problem with current statement that checks for covariance:

!is_type_compatible(p_function->get_datatype(), parent_return_type, true) && 
is_type_compatible(parent_return_type, p_function->get_datatype())

Second line is obviously needed as it is a definition of covariance but first line looks hacky - it is there to filter out Variants, which I think should be done properly.

For example, if I understand correctly, current implementation will not allow to override a typed Variant return with a more specific one. But this should be valid covariance.

So need proper handling of Variant/weak types instead of reverse call to is_type_compatible and yeah more tests with built-ins, specifically Variants (explicit (hard ones) and implicit (weak ones)).

@dalexeev
Copy link
Member

Also note that this PR implements return value covariance, but does not implement parameter contravariance. This doesn't have to be added in one release, but it makes more sense.

Comment on lines +1618 to +1619
// Verify that return type of function is either equal or a subtype of the return type of the parent.
valid = valid && (parent_return_type == p_function->get_datatype() || (!is_type_compatible(p_function->get_datatype(), parent_return_type, true) && is_type_compatible(parent_return_type, p_function->get_datatype())));
Copy link
Member

Choose a reason for hiding this comment

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

This check does not work correctly in some cases.

@YuriSizov
Copy link
Contributor

Superseded by #82477. Thanks for your contribution nonetheless!

@YuriSizov YuriSizov closed this Sep 28, 2023
@YuriSizov YuriSizov removed this from the 4.2 milestone Sep 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.

Support covariant return types for overriding functions
7 participants