[SR-1571][Sema] Don't synthesize super calls to out-of-band constructors #2854
Conversation
| // or an extension. We can use the type declared in that context to check | ||
| // if it's our immediate superclass and give up if we didn't. | ||
| // | ||
| // FIXME: Remove this when lookup of initializers becomes restricted to our |
There was a problem hiding this comment.
This looks good but it would be even better to address the FIXME too -- do you want to file a bug for that?
There was a problem hiding this comment.
Even better, I can provide a patch. @jrose-apple discussed the idea of plumbing NL_VisitSupertypes through as a proper part of NameLookupOptions and using it to affect lookupConstructors.
There was a problem hiding this comment.
Sounds good, thanks!
Although I think in this case, the problem is that piece of code we saw in NameLookup which checks if the found name was a protocol requirement, and looks up the witness -- I think this needs a special case where if the witness is a constructor, we skip it (since we would have found it otherwise I think)?
There was a problem hiding this comment.
But I'd like to do that in a different pull request. When I tried something like this before I have bad memories of breaking more tests than just this one.
There was a problem hiding this comment.
Alright, sounds good!
When you do the cleanup, I think it will suffice to verify that the existing tests pass.
There was a problem hiding this comment.
I think this needs a special case where if the witness is a constructor, we skip it (since we would have found it otherwise I think)?
The witness is the initializer in A and we're back to square one. Or, if I special-case this, I can't see where the super call actually goes, so I'm stuck on top of breaking any initializers in extensions that return Self et al. like RawRepresentable.
There was a problem hiding this comment.
By 'witness' I mean the implementation of the protocol requirement in the special code path, not the lookup result in general.
Let's discuss this in person tomorrow.
There was a problem hiding this comment.
My particular concern is whether we expect to find convenience initializers from extensions of protocols adopted by immediate or non-immediate superclasses.
There was a problem hiding this comment.
We also talked about whether it ever makes sense to find initializers that aren't immediately on a type or its immediate protocols, but that's potentially a much more disruptive change.
|
@swift-ci Please test |
|
Linux failure looks unrelated. |
|
Let's get it clean anyway. @swift-ci Please test Linux platform |
|
Still failing. Looks like it's @bitjammer's? |
|
I pushed a fix for that just now |
|
@swift-ci Please test and merge |
|
Still failing in the same test file. |
|
It looks like the Test and Merge job doesn't have the fix. Might need to run it again. |
|
@swift-ci Please test and merge |
|
Same place, same failure unfortunately. |
|
@jrose-apple If it helps, I can rebase onto the new changes and see if that helps. |
CodaFi
force-pushed
the
invalid-init-picks
branch
from
9012ecc to
9636752
Compare
|
@swift-ci please test |
CodaFi
force-pushed
the
invalid-init-picks
branch
from
9636752 to
9012ecc
Compare
|
I have a feeling this and the problems we've been having with #2854 are github bugs. Seems like it's seeing the rebase pushes as similar enough to the old changes that it squashes them together and reports the old CI results instead of the new ones. |
|
Here is the real linux build for this pull request. |
|
All right, everything's passing on master, and this test really shouldn't change anything. I'm going to merge it. |
|
In the future, please provide a bit more information in your commit message, so that people don't have to go look up the bug to see what's being fixed. (In this case I copy/pasted your description as the commit message.) Thanks, Robert! |
What's in this pull request?
Lookup can occasionally produce a call to a superclass constructor here that is not in our immediate superclass and we would trust it was completely valid. Instead, following the idea @slavapestov had, we reject the notion of creating such a constructor entirely unless its decl can prove it is actually in our superclass.
Thanks to @jrose-apple for the logic behind this patch and for sticking with me during our discussions about this.
Resolved bug number: (SR-1571)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.