Make ConformanceLookupTable less recursive and don't imply conformances from conditional ones #15268
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
|
@swift-ci please test compiler performance |
|
@swift-ci please test source compatibility |
DougGregor
approved these changes
Mar 15, 2018
lib/AST/ConformanceLookupTable.cpp
Outdated
| auto hasAdditionalRequirements = [&](ConformanceEntry *entry) { | ||
| if (auto ED = dyn_cast<ExtensionDecl>(entry->getDeclContext())) | ||
| if (auto TWC = ED->getTrailingWhereClause()) | ||
| return !TWC->getRequirements().empty(); |
There was a problem hiding this comment.
I suppose we're avoiding ExtensionDecl::isConstrainedExtension() because it's looking at generic signatures. Yeah, that makes sense.
There was a problem hiding this comment.
Yeah, I guess my comment isn't very clear in that it doesn't explicit state that.
lib/Sema/TypeChecker.cpp
Outdated
| TypeResolutionOptions options = TypeResolutionFlags::GenericSignature; | ||
| options |= TypeResolutionFlags::InheritanceClause; | ||
| options |= TypeResolutionFlags::AllowUnavailableProtocol; | ||
| GenericTypeToArchetypeResolver resolver(ext->getInnermostDeclContext()); |
There was a problem hiding this comment.
GenericTypeToArchetypeResolver is going to want a full-fledged generic environment for the extension. Can we make this even more lazy by only attempting name binding on the types in the inheritance clause?
There was a problem hiding this comment.
Hm, I guess this should avoid marking things invalid then, given there might be the valid type SomeClass<T> there (which is invalid, but the type itself isn't, right?).
I guess using the generic environment of the nominal type isn't correct?
There was a problem hiding this comment.
The generic environment of the nominal type would have different archetypes, which would then get recorded in the extension's inheritance list for something like SomeClass<T>. You're right that we currently reject something like that further down the line, but it seems like taking the name-binding approach dodges the issue entirely.
lib/AST/ConformanceLookupTable.h
Outdated
| @@ -372,7 +372,8 @@ class ConformanceLookupTable { | |||
|
|
|||
| /// Resolve the given conformance entry to an actual protocol conformance. | |||
| ProtocolConformance *getConformance(NominalTypeDecl *nominal, | |||
| ConformanceEntry *entry); | |||
| ConformanceEntry *entry, | |||
| LazyResolver *resolver); | |||
There was a problem hiding this comment.
You can grab the lazy resolver from nominal->getASTContext().getLazyResolver(), rather than having to pass it along.
There was a problem hiding this comment.
Oh, I didn't realise that was okay, since the other places here seem to thread it through.
|
Note to self: CoreStore is failing in the source compat suite. One example is reproduced here for posterity/for when jenkins deletes old runs: |
huonw
force-pushed
the
dont-infer-conditional
branch
from
bdd3714 to
491c461
Compare
|
@swift-ci please smoke test |
huonw
commented
Mar 29, 2018
| @@ -404,6 +405,36 @@ static void bindExtensionDecl(ExtensionDecl *ED, TypeChecker &TC) { | |||
| void TypeChecker::bindExtension(ExtensionDecl *ext) { | |||
| ::bindExtensionDecl(ext, *this); | |||
| } | |||
| void TypeChecker::resolveExtensionForConformanceConstruction( | |||
There was a problem hiding this comment.
I've made this more complicated, using resolveType and ResolveStructure rather than validateType. Thoughts?
There was a problem hiding this comment.
Yeah, this looks like an improvement. I really want the iterative type checker to be the right answer here, but it doesn't really do the "resolve structure" thing yet.
DougGregor
reviewed
Mar 29, 2018
| @@ -1447,6 +1447,11 @@ ERROR(objc_generics_cannot_conditionally_conform,none, | |||
| "type %0 cannot conditionally conform to protocol %1 because " | |||
| "the type uses the Objective-C generics model", | |||
| (Type, Type)) | |||
| ERROR(conditional_conformances_cannot_imply_conformances,none, | |||
| "conformance of type %0 to protocol %1 does not imply conformance to " | |||
| "inherited protocol %2 because it is conditional; explicitly state the " | |||
There was a problem hiding this comment.
Can we say "conditional conformance" in the beginning and remove the "because it is conditional" bit, to make this shorter? Additionally, how about we only suggest the "separate extension" remedy, because that's the style we've been advocating elsewhere.
There was a problem hiding this comment.
Ah, that's a more succinct phrasing thanks.
huonw
force-pushed
the
dont-infer-conditional
branch
from
491c461 to
ad646ba
Compare
|
I've changed the non-Swifty fixit to ... three fixits! Similar to
The relaxed requirements is a heuristic trying to match the common patterns with protocols like The full experience of accepting any of them is, for instance: extension X: Sub where T: Sub {}↓ extension X: Base where <#requirements#> {
<#witnesses#>
}
extension X: Sub where T: Sub {} |
|
@swift-ci please smoke test |
DougGregor
approved these changes
Apr 3, 2018
huonw
force-pushed
the
dont-infer-conditional
branch
from
ad646ba to
d8e54bf
Compare
The old `existingEntry->getProtocol() == protocol` condition was always true (the only way for `existingEntry` to get into the array it comes from is if is exactly for `protocol`), which meant this checking loop always bailed out, and that there was never more than one implied candidate ever. This probably isn't what was intended, given there's more code for handling this case directly after this condition, and `compareConformances` has extensive code for ranking two implied conformances for the same nominal/protocol pair.
…er conformance searching. We need to be able to find which conformances need to be declared/constructed without forcing extensions to be completely validated. This is important for both SR-6569 and rdar://problem/36499373. The former due to the source-level recursion, and the latter because implied conformances weren't always constructed (but are needed for good diagnostics). They weren't always constructed because: 1. ConformanceLookupTable's updateLookupTable on an early stage (before implied conformances are found) triggers extension validation *before* constructing any conformances, but *after* updating the stage state 2. extension validation validates the conditional requirements 3. validating the conditional requirements requires setting up generic signatures 4. setting up generic signatures forces the types conformances and so ends up in updateLookupTable on the same nominal again, skipping over the earlier stages that are complete/in progress 5. we expand the conformances that are implied by all the conformances we know about... But we don't know any, because we haven't finished the first updateLookupTable. This breaks the loop at step 2: we instead do the minimal work needed to know what conformances an extension (might) declare, which is connect the extension to a type, and then resolve the inherited TypeReprs to Types.
Given
protocol P1 {}
protocol P2: P1 {}
protocol P3: P1 {}
struct S<T> {}
extension S: P2 {}
extension S: P3 where T: P1 {}
Per SE-0143, we need to make sure that we notice that we can infer S: P1
completely unconditionally from S: P2, and ignore the constrained
one. This is a fairly crude approximation, because doing it accurately
would make this extremely recursive. The approximation used is a
conformance non-conditional if the conformance is from the type, or
there's syntactically no 'where' clause, which is a sufficient condition
for being non-conditional but not necessary (an extension *could* have a
completely redundant 'where' clause).
The recursion problem is checking if something is truly conditional
requires knowing the generic signature of the DeclContext, but knowing
this generic signature requires validating the extension/knowing the
type's conformances, which means hitting this table. (This is made worse
by actual recursive use of conformances, as in SR-6569.)
This single location can, theoretically, be made more intelligent about deducing indent from elsewhere and all the consumers will just work.
Many uses of conditional conformance to protocols with super-protocols
are for wrappers, where the conformances to those super-protocols
usually ends up using weaker bounds, such as:
struct MyWrapper<Wrapped: Sequence> {}
extension Wrapped: Sequence {} // Wrapped: Sequence
extension Wrapped: Collection where Wrapped: Collection {} // *
extension Wrapped: BidirectionalCollection where Wrapped: BidirectionalCollection {}
If this code was instead written:
struct MyWrapper<Wrapped: Sequence> {}
extension Wrapped: Sequence {} // Wrapped: Sequence
extension Wrapped: BidirectionalCollection where Wrapped: BidirectionalCollection {}
Inferring a Collection conformance would have to mean
extension Wrapped: Collection where Wrapped: BidirectionalCollection {}
which is unnecessarily strong.
It is a breaking change to change a protocol bound, and so we're
thinking we'd prefer that the compiler didn't magic up that incorrect
conformance. It also only is a small change (and the compiler even
suggests it, with a fixit) to explicitly get the implying behaviour:
declare the conformance explicitly.
extension Wrapped: Collection, BidirectionalCollection where Wrapped: BidirectionalCollection {}
Fixes rdar://problem/36499373.
huonw
force-pushed
the
dont-infer-conditional
branch
from
d8e54bf to
c104452
Compare
huonw
changed the title
|
@swift-ci please smoke test |
|
@swift-ci smoke test |
|
@swift-ci please smoke test. |
|
Question: Can the fix-it be like this? protocol Base {}
protocol Sub: Base {}
struct X<T> {}extension X: Sub where T: Sub {
}↓ extension X: Base, Sub where T: Sub {
} |
|
I had that originally, but @DougGregor informed me (the last review comment above, it seems I can't easily link to it 🙁 ) that we're trying to move away from that style, and instead prefer having one conformance per extension. |
|
Thanks for clarifying. I understand, but the current command line experience is too bad: Maybe we might want to emit separate messages depending on |
|
Oh! Do command line diagnostics not include multi-line fixits? That does look bad. I've opened https://bugs.swift.org/browse/SR-7352. |
This was referenced Apr 5, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is preparation for https://bugs.swift.org/browse/SR-6569 along with a proposed fix for rdar://problem/36499373 . This is explicitly not what SE-0143 says, so I'll have to get to amending that before this lands (at least, before the last two commits do).
There was a pile of recursion between
ConformanceLookupTable::updateLookupTableand the TypeChecker/extension validation, especially for conditional conformances. This takes one approach to breaking that recursion by lettingupdateLookupTablevalidate much less of the extension until much later. The recursion meant that conformances weren't being constructed when they need to have been. (The commit messages themselves contain a lot more of text/description about exactly what and why things were recurring.)