[TypeChecker] Don’t crash if a ExplicitCastExpr doesn’t have a cast type

[ahoppen]

If the type of an ExplicitCastExpr is not valid, it is a null type, which causes a crash when checking if the cast type has parameterized existentials.

rdar://95629905

[ahoppen]

@swift-ci Please smoke test

[lucianopa-msft]

Should we add a regression test for that?

[ahoppen]

I don’t have a reproducing test case unfortunately. The fix is just based on a crash trace.

[AnthonyLatsis]

The fix is just based on a crash trace.

So the radar just included the trace and no code? Hopefully Pavel can come up with one, not too familiar with result builders myself. It could be that the absence of a type here is not benign to begin with.

[xedin]

I am out this week so I will take a look at this as soon as possible when I am back!

[xedin]

I took a quick look at this and I might have an idea about what is going on. Based on the stacktrace -diagnoseStmtAvailability walks into an AbstractClosureExpr because ExprAvailabilityWalker always returns true from shouldWalkIntoClosure but:

1. Result builders currently have multi-statement closure inference disabled due to some ordering issues related to one-way constraints;
2. In code completion mode we always pass LeaveClosureBodiesUnchecked to the constraint system;

I wonder if something like this might reproduce the crash:

import SwiftUI

struct S<T> {
}

extension S : ExpressibleByIntegerLiteral {
  typealias IntegerLiteralType = Int8

  init(integerLiteral: IntegerLiteralType) {
  }
}

struct MyView : View {
  var body: some View {
    Group {
      EmptyView()

      test {
        let x =  #^COMPLETE^# as S<String>
        print(x)
      }
    }
  }

  func test(_: () -> Void) -> some View {
    return EmptyView()
  }
}

[ahoppen]

That example doesn’t reproduce the crash for me. AFAICT closures aren’t walked
https://github.com/apple/swift/blob/ba42be5fcf2d663a589a3f5e4efe0ffcffb101fa/lib/Sema/MiscDiagnostics.cpp#L64-L65

I very much suspect that this issue will be resolved fundamentally when we get rid of LeaveClosureBodiesUnchecked and I’m wondering if it makes sense to hunt for a reproducing test case now since the fix seems pretty straight-forward.

[xedin]

The check you pointed out would skip walking into some decls in the closures, not the closures themselves though. I think this issue needs to be addressed in a different way by adjusting the check in ExprAvailabilityWalker, otherwise it would just crash in some other place…

[ahoppen]

I managed to find a reproducer that’s not code completion specific. With this reproducer at hand, I would argue that the fix is correct because AFAICT it appears correct that a ExplicitCastExpr has no CastType if that type is invalid. So we need to check if the type is valid (i.e. not null) before calling hasParameterizedExistential.

[xedin]

How did an expression with incorrect cast end up getting past to a syntactic diagnostics?

[LucianoPAlmeida]

AFAICT it appears correct that a ExplicitCastExpr has no CastType if that type is invalid.

I don't know for sure if that makes sense in this case, but some other places I saw where the type was invalid the expected was to have an ErrorType instead of a null type. But again not sure if that is the case here...

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please SourceKit stress test

[hamishknight]

I'm curious why this change is needed, is it not sufficient to return an ErrorExpr body if we encounter an error? For some context, we eventually want SILGen to be able to handle emitting potentially invalid ASTs with an inverted compiler pipeline, so the idea was we could teach SILGen to emit an ErrorExpr as an undef and just emit bodies that way.

[ahoppen]

No, because editor functionality (most notably cursor info + indexing) still wants to walk the partially type-checked brace statement to salvage as much information as possible from it. If we just return ErrorExpr (or similar here), that’s no longer possible.

[hamishknight]

Hmm okay, makes sense. Eventually we really ought to do this at a finer grained level though, e.g have typeCheckExpression produce individual ErrorExprs or type expressions as ErrorType. It's really unfortunate to have type-checking requests return un-type-checked AST on failure.

[ahoppen]

The stress tester found issues with the principled fix of propagating HadError from elements in BraceStmts, related to how we type check closures separately for code completion.

I decided to scope the fix down to the original nullptr check because it’s way less risky. I’ll look into the proper fix once we get rid of LeaveClosureBodyUnchecked.

[ahoppen]

@swift-ci Please smoke test