SE-0309: SILOptimizer fixes & reenable executable tests

[AnthonyLatsis]

Note

The executable test was XFAILed in #39334 due to a crash on builds with optimizations enabled. The issue turned out to be unrelated to the feature and may have had something to do with usage of strings in the test.

[AnthonyLatsis]

@swift-ci please smoke test macOS

[AnthonyLatsis]

@swift-ci please smoke test linux

[AnthonyLatsis]

@swift-ci please smoke test Windows

[slavapestov]

Why is this necessary? When do non-root archetypes appear in the map?

[AnthonyLatsis]

They don't, well, hopefully. It's Type::subst that queries the map with non-root archetypes, and it probably has been since the dinosaurs. Do you reckon we should fix that instead?

SubstitutionMap::lookupSubstitution has this early return as well btw.

[slavapestov]

Let's remove this part then, or change it to an assertion.

Type::subst() passing in non-root archetypes is something that I believe only associated type inference relies on. You can try taking it out and see what breaks.

[AnthonyLatsis]

I still need this tweak, otherwise QueryTypeSubstitutionMapOrIdentity won't work with non-root opened archetypes, and that makes the SIL cloner crash. I can use a custom substitution fn instead, WDYT?

P.S. This is about getASTTypeInClonedContext.

[AnthonyLatsis]

Would it not be safer to leave this part as is and add a FIXME, rather than propagate the check to clients? Associated type inference itself does not appear to be using these function objects.

[slavapestov]

CC @atrick

[slavapestov]

@AnthonyLatsis I was actually looking at this disabled test the other day and when I reduced the failure it did not involve non-root opened archetypes. What I had was this:

protocol P {
  var covariantSelfPropClosure: ((Self) -> Void) -> Void { get }
}
extension P {
  var covariantSelfPropClosure: ((Self) -> Void) -> Void { { $0(self) } }
}

struct S: P {}

let p: P = S()

p.covariantSelfPropClosure { _ in }

It was crashing because IRGen was trying to allocate a zero-sized heap context for the closure. Is this a different bug?

[AnthonyLatsis]

It was crashing because IRGen was trying to allocate a zero-sized heap context for the closure. Is this a different bug?

Interesting, I will take a look in a minute. For me it stopped crashing the moment I switched from strings to integers, which made me think whatever issue was unrelated.

when I reduced the failure it did not involve non-root opened archetypes

The issues with non-root archetypes were revealed after adding more tests for methods with covariant associated types.

[AnthonyLatsis]

@slavapestov How do I reproduce the crash? It compiles fine on main if I pass -emit-ir -O (should have figured it was a runtime crash).

[AnthonyLatsis]

@slavapestov It is a different issue. How did you manage to dig out the root cause?

[AnthonyLatsis]

Simplified the crasher a bit more:

protocol P {
  var prop: () -> Self { get }
}
extension P {
  var prop: () -> Self {
    { self }
  }
}

struct S: P {}
let p: P = S()

let y = p.prop()

[slavapestov]

It's an IRGen issue. @drexin is working on a fix: #58532

[AnthonyLatsis]

Thanks for the link. I definitely lack domain knowledge to fully understand why this happens with this specific closure and not others, but I have a rough idea of what's going on now. Interestingly, the crash won't manifest if Self is replaced with something else, regardless of whether self is captured by the closure.

[gottesmm]

Can you be sure to push this through full testing rather than smoke testing?

[AnthonyLatsis]

@swift-ci please test

[AnthonyLatsis]

@slavapestov is there anyone else we need an approval from?

[slavapestov]

I think it's fine, but let's re-run tests since it's been more than 2 weeks now.

[slavapestov]

@swift-ci Please test

[AnthonyLatsis]

I think it's fine, but let's re-run tests since it's been more than 2 weeks now.

May I ask you to sign it off then?

[AnthonyLatsis]

Thanks.