More errors instead of crashes for conditional conformances that are invalid or involve hard-to-resolve recursion #17356
Conversation
huonw
force-pushed
the
lazier-conditional-requirements
branch
2 times, most recently
from
969ac4b to
c29c6d5
Compare
|
So you now guard against conditional requirements not being computed in the conformance logic. |
|
|
||
| protocol P {} | ||
| struct A<C> {} | ||
| extension A: P where A: P {} // expected-error {{requirement involves recursion that is not currently supported}} |
There was a problem hiding this comment.
Can this ever be supported or make sense? I think the existing does not refer to gen param or assoc type is more appropriate and expressive in these cases.
There was a problem hiding this comment.
I don't think this particular form of tautological recursion makes sense, and I agree that that error message you quote gets to the fundamental error. I do however want to land this to stop the crashers since they come up so often, and then we can hammer on making the exact error messages perfect after that. I'll file a JIRA.
There was a problem hiding this comment.
lib/AST/ASTDumper.cpp
Outdated
| @@ -2926,10 +2926,16 @@ static void dumpProtocolConformanceRec( | |||
| } | |||
| } | |||
|
|
|||
| for (auto requirement : normal->getConditionalRequirements()) { | |||
| if (auto condReqs = normal->getConditionalRequirementsIfAvailable()) { | |||
There was a problem hiding this comment.
Hmm. It's unfortunate that the dumper can modify state here by triggering the computation of conditional requirements.
There was a problem hiding this comment.
Stopped the mutation in these cases.
lib/AST/ASTDumper.cpp
Outdated
| @@ -2957,10 +2963,15 @@ static void dumpProtocolConformanceRec( | |||
| SubstitutionMap::DumpStyle::Full, indent + 2, | |||
| visited); | |||
| out << '\n'; | |||
| for (auto subReq : conf->getConditionalRequirements()) { | |||
| if (auto condReqs = conf->getConditionalRequirementsIfAvailable()) { | |||
There was a problem hiding this comment.
Same comment about dumping mutating state.
This doesn't fix the fundamental problem of correctly handling such cases, but
it is better than the "error message" that occurred previously:
Assertion failed: ((bool)typeSig == (bool)extensionSig && "unexpected generic-ness mismatch on conformance").
Fixes the crash rdar://problem/41281406 (that in
https://bugs.swift.org/browse/SR-6569 (rdar://problem/36068136)),
https://bugs.swift.org/browse/SR-8019 (rdar://problem/41216423) and
https://bugs.swift.org/browse/SR-7989 (rdar://problem/41126254).
…n error cases. When we're trying to diagnose something, it's a bad look if we crash instead. This changes the bad-associated-type recovery path to not require that the conditional requirements have been computed, and instead detects that as a possible error. Fixes https://bugs.swift.org/browse/SR-8033.
huonw
force-pushed
the
lazier-conditional-requirements
branch
from
c29c6d5 to
a21a779
Compare
|
@swift-ci please smoke test |
Two cases go from being a crash to being a more explanatory error with locations etc.:
rdar://problem/41281406, https://bugs.swift.org/browse/SR-8019 (rdar://problem/41216423) and https://bugs.swift.org/browse/SR-7989 (rdar://problem/41126254).
https://bugs.swift.org/browse/SR-8033.
This doesn't fix the recursion itself. It also doesn't use the request evaluator, because I want to put this into the 4.2 branch.