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

[SR-7111] Class function override T?? to T. #15050

Closed

Conversation

tarunon
Copy link
Contributor

@tarunon tarunon commented Mar 7, 2018

Resolves SR-7111.

@slavapestov
Copy link
Member

slavapestov commented Mar 7, 2018

I’d rather we didn’t make this change. Ideally method override matching would use the same logic as the type checker’s subtype relationship. But in the absence of that, just adding new special cases is probably not a good idea.

@jrose-apple
Copy link
Contributor

Override matching is already a different thing than subtypes, and I think the point is that this isn't a new special case; it's fixing the logic for an existing special case. FWIW this is already a reusable API, TypeBase::matches.

@slavapestov
Copy link
Member

My point is exactly that the only reason override matching is different than subtyping is historical accident, and we should either leave it alone or combine the two.

@tarunon
Copy link
Contributor Author

tarunon commented Mar 8, 2018

I think Differences is not historical accident, maybe it is about Existential.

protocol Animal {}
class Cat: Animal {}

class Foo {
    func get() -> Animal { fatalError() }
}

class Bar: Foo {
    override func get() -> Cat { fatalError() }
}

It seems be compile error today, can we solve it already? If my memory is correct, it was rutime error.

@slavapestov
Copy link
Member

@tarunon This example could be made to work pretty easily I think. SILGen can already emit the right thunk there because it supports all subtype conversions — what’s missing is the type checker admitting this case as an override. But my point is that it would be better in the long run to actually use the same code we use when solve subtype constraints, instead of using bespoke code to match overrides.

@slavapestov
Copy link
Member

@rudkx might have some ideas, I think this might be related to his Type::join work.

@rudkx rudkx self-requested a review March 9, 2018 02:44
@rudkx
Copy link
Member

rudkx commented Mar 9, 2018

There is definitely non-accidental history behind why we cannot just test subtyping here.

We allow overriding Objective-C methods that take unaudited pointer arguments (imported as IUOs) with Swift methods that take non-optional arguments.

There may be other reasons as well.

override func param_class_nested_optional(_ x : B??) {}
func param_class_nested_optional_rev(_ x : B) {}
override func param_subclass_nested_optional(_ x : A??) {}
func param_subclass_nested_optional_rev(_ x : B) {}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add tests for things which should not be allowed as well?

@tarunon
Copy link
Contributor Author

tarunon commented Mar 9, 2018

I know the various problems that optional covariances, I think T to T? subtyping should be abolished, but I also understand it is necessary for compatibility with Objective-C. Perhaps it is better to allow optional covariances in only @ objc attribute. I would like to discuss it in the swift-evolution.
I tried to generalize Optional's subtyping according to the current subtyping rule in this PR. If we need a big modification (such as merging common subtyping and override), I think it would be better to discuss in swift-evolution.

@tarunon
Copy link
Contributor Author

tarunon commented Mar 9, 2018

BTW, I try to using common subtyping way in this week end. @slavapestov

@tarunon
Copy link
Contributor Author

tarunon commented Mar 11, 2018

I add property test cases, it should be failure case. Can I add other test cases? @rudkx
I try to use swift::TypeChecker::isSubtypeOf instead of swift::type::matches, it seems work well(also Existential case).

@rudkx
Copy link
Member

rudkx commented Mar 12, 2018

@tarunon When you say that using isSubtypeOf works well, what tests did you run?

I would expect that to fail for the cases I already mentioned regarding the special-case for overriding unaudited Objective-C methods.

@tarunon
Copy link
Contributor Author

tarunon commented Mar 13, 2018

I replace matches to isSubtypeOf in this line, and every test cases was passed.

if (!propertyTy->matches(parentPropertyTy,

But I found 1 more use matches at this line,

if (declFnTy->matchesFunctionType(parentDeclFnTy, matchMode,

I think I should add some of implements for these if we use isSubtypeOf

@tarunon
Copy link
Contributor Author

tarunon commented Mar 22, 2019

I close PR cuz I can't see long time 😞
If I can, try make PR again 🧐

@tarunon tarunon closed this Mar 22, 2019
@tarunon tarunon deleted the SR7111/allow_override_nested_optional branch March 22, 2019 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants