[Sema][GSB] Fix crash on cond conformances with invalid req

[AnthonyLatsis]

Example

protocol P1 {}; protocol P2 {}; protocol P3 {}

class Foo<T> {}
extension Foo: P1 where T: P1 {}
extension Foo: P2 where Foo: P1 {} // #1 crash

extension Foo: P3 where Foo: P3 {} // #2 crash

The crash happens because an extension doesn't yet have a generic signature. The signature is retrieved eventually when calling lookupConformance during addTypeParameter. In #2, the reason is quite obvious: lookupConformance is called on Foo and P3, which leads to retrieving the gen sig of the same extension which is still being built. #1 is a bit trickier. First, consider this example that does not crash:

extension Foo: P1 where T: P1 {}
extension Foo where Foo: P1 {}

This doesn't crash because the absence of Foo: P2, unlike #1, results in the extensions being type-checked sequentially. This means that while type-checking the second extension, the first already has a gen sig and the assertion doesn't get tripped. Now, back to #1:

extension Foo: P1 where T: P1 {}
extension Foo: P2 where Foo: P1 {}

Here, with Foo: P2, type-checking the first extension for some reason triggers the second extension to be type-checked as well; by the time we reach the assertion both extensions still don't have a gen sig.

The call chain is rather long and I yet haven't found out the reason of the difference in type-checking. Maybe that's the way it should be..

Solution

The solution applied is more general and does not focus on how each individual case is type-checked. Check whether the subject of the requirement is self and prevent looking up conformances and adding additional requirements in that case. This isn't harmful since the requirement is invalid anyway.

Resolves SR-7989.

Do crash fixes have to go to 4.2?

[huonw]

Thanks for taking the time to look at this. Unfortunately, I'm not so sure about this; I'd definitely appreciate some more explanation (see my inline comments), and I don't think it's the right long term fix: there's some fundamental recursion here that we need to break (https://bugs.swift.org/browse/SR-6569). I'm going put a bit of effort into trying to stop the assertion, is it alright if we leave this stop-gap PR unmerged for a bit (as a back-up) in the hope that I'll be able to land those improvements?

[huonw]

Could you explain why you're checking the declared types? I suspect that NTD != subjectNTD might work.

[AnthonyLatsis]

Because Foo != Foo<T> or Foo<Int>, but Foo == Foo, where Foo is the declared type (I am comparing the base types, simply comparing the types would result in some cases falling through. In particular, the inequalities above).

[huonw]

I think one of us isn't understanding the other: I agree that we can't just compare NTD->getDeclaredType() with subjectType, but I wasn't proposing that. I believe that the declared types of two decls are equal if and only if the decls are equal (i.e. are the exactly the same decl), meaning this line can just be NTD != subjectNTD, and there's no need to do the work to get the declared types.

[AnthonyLatsis]

Will this work if we are comparing Foo<T> and Foo<Int>? Example:

extension Foo: P where Foo<Int>: P {...}

[huonw]

Yes; getting down to the NominalTypeDecls gets away from all the information about a specific instantiation of the type. Each type declaration corresponds to a single (nominal type) Decl, and one way to get a type corresponding to an instance of this unique Decl is getDeclaredType.

The problem here is we've got lots of Types that all might connect to the same real underlying type, e.g., as you say, Foo<T> and Foo<Int>, and we want to make sure those match because they're both instantiations of the Foo type. The code is currently mapping both of those to a decl, and then asking for "what's the type that this decl declares" on each to compare, I'm suggesting we just map to the decl and compare that: if we have Foo<T> and Bar<Int> the nominal type decls will be different (the one for Foo and the one for Bar), and if we have two Foos they'll both be the decl for Foo.

[AnthonyLatsis]

Thank you! I was confusing types with type declarations, unbelievable 😅

[huonw]

Hm, I think this could do with some explanation: is this new code trying to deduce if we're in a circular situation, where the requirement is referring to the "original" type (the one for which we're building the generic signature)? And these few lines are trying to work out that original type?

[AnthonyLatsis]

Yes, I am proceeding with the conformance lookup if the subject type has a null decl context or isn't a nominal type declaration context, or otherwise isn't the type we're extending or declaring. Simply put, I am just making sure not to look up conformances if the base type of the subject and the current context match.

[AnthonyLatsis]

Unfortunately, I'm not so sure about this; and I don't think it's the right long term fix

Neither am I after bumping into SR-8033 and your linked bug that crash on the same assertion. I was going to look deeper into what the root of the problem is, and since the individual cases are so different I had the idea of assigning a default generic signature to anything that could refer to it in the process of building the actual gen sig. But I don't know what else can be done in cases like extension Foo: P where Foo: P. We would still need to guard against Foo in requirements.

I'm going put a bit of effort into trying to stop the assertion, is it alright if we leave this stop-gap PR unmerged for a bit (as a back-up) in the hope that I'll be able to land those improvements?

Sure, go ahead. I'll too continue working on this branch to see if I can improve the approach. Can you leave a link if there's any progress?

I'll answer the comments in a moment.

[huonw]

I opened #17356 which turns a lot of the crashers into errors.

[AnthonyLatsis]

I guess we can close this workaround in favor of #17356