Casts of collections of function types

Development Compiler
1

TL;DR

Casting collections of function types crashes at runtime. I've been working on a fix for ~10 months across three PRs:

  • First PR was approved and merged, but it didn't handle some use cases and was reverted.
  • Second PR follows the approach approved in the first PR also fixing identified gaps. But it got stuck in code review. There is a disagreement about the scope of the fix. Review feedback on the two PRs gives contradictory guidance about synthesizing ClosureExpr. I've got no response from the reviewers since April.
  • Third PR provides alternative implementation - avoiding AST changes, as requested in the second PR feedback. But it does not address disagreement about the scope of the fix.

I would like to get some fresh eyes on this topic, to:

  • clarify the scope of the fix
  • make a decision about the best place to synthesize ClosureExpr or work out another strategy for lowering KeyConversion/ValueConversion.

The Bug

All casts of collections of function types crash at runtime, while the equivalent scalar casts work fine. This is independent of @preconcurrency or typealiases — here's a pure Swift 6 reproducer:

func upcastArray(_ x: [@Sendable () -> Void]) -> [@MainActor () -> Void] {
    return x // no diagnostics, crashes at runtime
}

func upcastScalar(_ x: @escaping @Sendable () -> Void) -> @MainActor () -> Void {
    return x // works fine
}

Or even one unrelated to concurrency:

func upcastArray(_ x: [(Base) -> Void]) -> [(Derived) -> Void] {
    return x // no diagnostics, crashes at runtime
}

func upcastScalar(_ x: @escaping (Base) -> Void) -> (Derived) -> Void {
    return x // works fine
}

Root Cause

There are two underlying problems:

  1. Collection element casts and scalar casts use different machinery. For both cases CSApply emits code for coercion of function types. For scalars it ends up being used in the SILGen, but for collections it is recorded inside CollectionUpcastConversionExpr and not used after that. Instead, collection casts are lowered using runtime functions (_arrayForceCast, etc.) that use as?/as! for casting elements.

  2. Dynamic casts of function types are not supported. The runtime always fails them.

Fixing #1 makes collection casts behave identically to scalar casts — coercions work, dynamic casts fail consistently. Fixing #2 would make dynamic casts work, but without #1, @preconcurrency coercions in collections would still crash, because @preconcurrency coercions are not valid dynamic casts.

I believe #1 is higher priority.

Why a Narrow Fix Doesn't Work

It was suggested in review that the original issue could be solved more narrowly by changing how isolation attributes are stripped from @preconcurrency typealias, so the problematic cast would never be emitted. However:

  • The crash reproduces in cases that don't involve @preconcurrency, typealias or minimal concurrency checking.
  • Differences in stripping concurrency attributes seem to be intentional as per SE-0337.

The crash is in collection-cast runtime infrastructure, not in concurrency attribute handling. I'd like confirmation that fixing it at the collection-cast level is the right scope.

The Fix

Both open PRs solve problem (1) by introducing new runtime functions for collection casts that take a closure for casting elements. Type coercion code emitted by CSApply needs to be lowered into a closure passed to these runtime functions. This lets the compiler emit element-cast code identically to how it handles scalar function casts. These closures don't capture values, but might capture types from a generic context.

The disagreement is about where that closure is constructed.

Two Approaches

In the AST, CollectionUpcastConversionExpr has KeyConversion/ValueConversion fields currently modeled as ConversionPair. These were populated by CSApply but never consumed by SILGen since the original implementation by @John_McCall. SILGen currently has no infrastructure to lower a ConversionPair into a closure.

Approach A — Use ClosureExpr as representation for KeyConversion/ValueConversion (PR #87512):

  • Replaces ConversionPair with ClosureExpr in CollectionUpcastConversionExpr
  • CSApply directly creates the closure.
  • Sema assigns discriminator and capture info
  • Covered by ASTVerifier
  • SILGen lowers it like any other closure

Approach B — Synthesize closure in SILGen (PR #91638):

  • Keeps ConversionPair in the AST unchanged
  • SILGen creates a detached ClosureExpr on the fly from the ConversionPair
  • Bypasses ASTVerifier
  • Duplicates discriminator assignment and type-capture computation from Sema
  • Includes a ConversionClosureEmitter that clones a limited subset of Expr subtypes

I've implemented Approach B to address this feedback, but I believe Approach A is the better long-term solution — it's simpler, uses existing infrastructure, and has verifier coverage. I'm willing to land either one.

The last reviewer response was in April 2026. I'd appreciate @kavon and @xedin to resume the discussion, or get input from anyone else from the Core team (cc @John_McCall as the original designer of CollectionUpcastConversionExpr, @Douglas_Gregor and @beccadax as authors of SE-0337) on which direction to take.

4 Likes
2

I would like us to turn conversions into a more abstract representation that doesn't rely on having separate expression nodes for each step of the conversion. That is then something we ought to be able to generate code for in SILGen. But ConversionPair is already not really that representation because it relies on an OVE and an expression. I don't think it's really worse to just build a closure expression in the AST.

3

Could you elaborate on this? Do you mean simplifying AST to have a single conversation node storing only source expression and destination type and letting SILGenPoly.cpp to sort it out? If so, what are the benefits of this approach?

Note that implementation PRs still have some cases not fully implemented. FunctionConversionExpr does not store conversions for arguments and results and they need to be inferred in SILGenPoly.cpp. And if they involve collection casts, then inferring KeyConversion/ValueConversion is not implemented yet. I was planning to address this in a separate PR. But so far, I was leaning towards enriching FunctionConversionExpr with sub expressions for conversion of arguments and result. I’ve got an impression that constraint solver already has this information and re-inferring it in SILGen would lead to duplication of logic.

4

No, I mean giving conversions essentially their own AST which just goes type->type rather than applying to a source expression. And then most conversions in Expr would just involve a single expression plus one of these Conversion values, which might describe several successive conversions. One of the merits of this is exactly that it gives us a space to describe recursively-complex conversions, like optional/tuple/function conversions, without having to use OVEs or non-user-defined ClosureExprs.

Anyway it’s a big change, I’m not asking you to do it here.

5

I understand the appeal of this idea where Sema “works out” the conversions into a structured form, which SILGen then interprets. This also enables some flexibility where Sema can choose between multiple possible conversion paths somehow, and tell SILGen which one to use, and it makes sense if the solver's current matchTypes() is the single "source of truth" for the subtype relation. But I wonder if this generality is really needed in practice.

When the constraint solver finds a solution, all type variables must satisfy the given constraints. So if "$T0 conv $T1" is true, then after substituting T0 and T1 with fixed types recursively, the predicate "does this convert to that" must return true when evaluated with those types as inputs. As long as this predicate is implemented correctly and only once, you ought to be able to use it to recover the "conversion path" when needed. The problem is today, we don't have this implemented as one predicate!

The subtype relation is implemented in three places:

  1. matchTypes() in the solver is something we've talked about overhauling for many years. It's way too long as a function because it too much all at once, like breaking down a constraint into sub-constraints, handing various error recovery situations, merging type variables, binding type variables to their fixed types, etc.
  2. Also in the solver, Subtyping.cpp implements the subtype relation on fully-substituted types, as well as joins and meets. This is used by the constraint solver's disjunction selection and binding optimizations. When you give it a type that contains type variables, it does something conservative.
  3. SILGenPoly.cpp, which only has to deal with fully-substituted types, also has to completely understand the subtype relation, mostly because of function conversions, but also the places that SILGen must emit conversions that are not AST, like witness thunks and vtable thunks.

For 2) and 3), we could extract out the common visitor, and this would already be an improvement, I think.

Like I said, 1) needs a major refactoring to untangle everything anyway. Once that is done, it could also use the same visitor, but instead of always giving a yes/no answer, you sometimes get a breakdown into a list of pairs of the form (type var, concrete type), (concrete type ,type var), or (type var, type var), and then use that to generate sub-constraints.

(Another caveat with 1) is that there are various "sub-kinds" of conversions, like Subtype, Conversion, and ArgumentConversion in the solver. We're trying to get rid of the distinction between the first two, and the rest can be handled as special cases because they only influence a few things like pointer conversions. Subtyping.cpp already handles those, but SILGen doesn't "see" pointer conversions inside function positions because of language rules.)

So instead of defining a new representation for Type -> Type operations and then writing little "interpreters" for it in various places, we have a single source of truth which doesn't guarantee that any specific conversion path is chosen, just a valid one (as long as all users of the visitor use it correctly.)

Most conversions (and all conversions that can appear in nested positions, for this to work) should be represented uniformly in the AST after Sema, but they don't really need much further structure, I think. It's already described in the types.

1 Like
6

Well, we do have conversions that have more structure, though. Specifically, existential erasure uses the conformances that it’s checked against, and that’s already context-sensitive (and could easily get more context-sensitive in time). And some of the conversion analysis is relatively tricky, like the ambiguity between optional injection and optional conversion. So while I understand the thought that consumers should just rediscover the conversions from the type pair, I do think there’s virtue in not just being explicit about it in the AST, but being explicit in a less ad hoc way than we currently are.

7

Yeah, but we're so far from that today that it still feels like a case of "you're not going to need it". Recording conformances in substitution maps is a win for when the SIL optimizer repeatedly performs type substitutions while inlining and specializing for example, but performing a single lookup when lowering an AST conversion expression to SIL doesn't seem like the end of the world.

The other concern is that if the repeated lookup will return the same conformance in the same compilation session at least, but it really has to, because we assume in too many places today that lookupConformance(conformance->getType(), conformance->getProtocol()) == conformance is just a basic invariant that always holds. Unwinding that would be even more difficult than "the module system knows what to do with overlapping conformances and presents this to lookup in a useful way", and I think the wins from anything it could possibly enable will be marginal at best.

There's some ambiguity in the solver related to that, but once types are fully concrete I don't think it comes into play.

I don't think anything I'm suggesting really forecloses the possibility of doing the reified representation of conversions later, if we end up needing it.

Alternatively, you can see my idea of extracting some common visitors here as an intermediate step to your vision. ;-)

8

Well, the algorithm we tend to naively recreate when consumers are in charge is “if the destination type is optional, check if the source type is optional; if so, it’s optional-to-optional, and otherwise it’s an injection”. Unfortunately, that’s really not correct when there’s a difference in optional depth, because T? should be injected into T?? rather than optional-to-optional’ed, with the difference being that .none becomes .some(.none) instead of .none. Ironically, I think we actually had a bug at one point where the constraint solver was doing the wrong thing and SILGen saved the day by ignoring it. Anyway, you’re not wrong that we always can figure out the right conversion from scratch given the resolved types, it’s just more complex than a simple recursive analysis.

Maybe, but the path to a solution here clearly starts with having fewer places redoing the lookup. Besides, a lot of these redundant lookups in constraint solving and so on end up being fine because they’re using a consistent DC; the problem with doing them in SILGen and (especially) later is that the DC is more or less arbitrary w.r.t the original context of the operation in question.

9

So, shall I merge PR#87512? @John_McCall, can I get an approval in the PR please? @xedin, any objections?

10

The problem that I have faced is not describing the conversions, but lowering them. SILGen cannot lower ConversionPair into a function. This is the reason why I introduced ClosureExprs. And lowering into a function requires identity (to produce the function name) and computing captures (only types). ClosureExpr's already provide identity and computation of captures. How would this be handled with a conversions own AST?

Is SILGen even a suitable place for this? Or should instead Sema calculate all the conversions:

  • for witness/vtable thunks - and record them in Witness
  • for converting function arguments and result types recursively - and record them in FunctionConversionExpr ,
  • etc..
11

This discussion seems to be stuck as well :disappointed_face: . I really want to achieve a solution aligned with and approved by the Core Team, but I'm out of ideas how to achieve this. Unless there are any objections before the end of this week, I'm gonna merge PR#87512 this Friday. I'm aware that this does look like a violation of the Code Review policy, but I don't have any better ideas. @John_McCall @Slava_Pestov @kavon @xedin

1 Like
12

I'm sorry that the PR has stalled out. I'll try to make sure we get eyes on it ASAP; please don't merge without approval, though.

13

Thank you! I'll wait.

14

I see in the PR this comment from @xedin. @John_McCall, does this constitute "getting eyes on it", or can I hope for something more constructive?

@xedin, have you had a chance to look at the alternative version?

I've also reported this issue as FB24935580, this time avoiding typealias or @preconcurrencyin the reproducing code. I hope this helps to get aligned on the scope of the issue and the fix.

15

Pavel is, unfortunately, going to be going on break for a few weeks, so I can't speak for him. I personally would rather that we just synthesize this conversion function in SILGen.

16

I agree, I don’t think we need drastic Sema changes here, we already produce a function conversion expression that should be sufficient. I mentioned that in the thread above.

1 Like
17

Could you pls take a look at the alternative fix? It avoids AST changes at the cost of duplicating some Sema functionality in SILGen.

18

If you have to create AST nodes in SILGen to lower them it’s never the right thing, it should be possible to emit that code directly.

19

It is not obvious to me how to directly lower ConversionPair into a function (actually, a closure as a converted R-value). Could you please give me some pointers about how to implement this?

20

Your patch mostly has the right idea, you just need to create a function instead of synthesizing a closure.

/// Creates a value conversion function that maps between the most general
/// abstraction patterns of the given types, taking a @in_guaranteed parameter
/// and returning an @out result.
ManagedValue SILGenFunction::createOpaqueValueConversionFunction(
    SILLocation loc,
    CanType inputSubstType,
    CanType outputSubstType) {
  auto expectedFnType = getSILFunctionType(TypeExpansionContext::maximal(), AbstractionPattern::getOpaque(), FunctionType::get({inputSubstType}, {}, outputSubstType));
  auto inputOrigType = AbstractionPattern::getOpaque();
  auto outputOrigType = AbstractionPattern::getOpaque();
  return createValueConversionFunction(loc, expectedFnType, inputOrigType, inputSubstType, outputOrigType, outputSubstType);
}

/// Creates a function that converts one value to another.
/// The conversion has to be something we can do with an implicit conversion,
/// so basically a subtype relationship.
ManagedValue SILGenFunction::createValueConversionFunction(
    SILLocation loc,
    CanSILFunctionType expectedFunctionTy,
    AbstractionPattern inputOrigType,
    CanType inputSubstType,
    AbstractionPattern outputOrigType,
    CanType outputSubstType) {
  ...
}

The basic pattern you'll be following in the main function here is createThunk in SILGenPoly.cpp.
You'll need to understand what that's doing in order to understand how it needs to change for your function. emitThunk is creating a "reabstraction thunk", basically a helper function that's used to convert one function signature to another. That is, we start with a function value like this:

%fn : $@callee_guaranteed (@inout C, Int) -> ()

and we want to convert that to, say,

$@callee_guaranteed (@inout C, @in_guaranteed Int) -> @out ()

We do that by creating a function:

sil shared @thunk : $@convention(thin) (@inout C, @in_guaranteed Int, @guaranteed @callee_guaranteed (@inout C, Int) -> ()) -> @out ()

and then "partially applying" it to the original value:

%thunk = function_ref @thunk : $@convention(thin) (@inout C, @in_guaranteed Int, @guaranteed @callee_guaranteed (@inout C, Int) -> ()) -> @out ()
%convertedFn = partial_apply %thunk(%fn)
// %convertedFn: $@callee_guaranteed (@inout C, @in_guaranteed Int) -> @out ()

The body of the helper function then splits out the input function from the rest of its parameters, converts the parameters into whatever the input function value, does the call, and converts the result to whatever the output is supposed to be.

In your case, you don't have an input function, and there's nothing to call; your function is just taking the one parameter and converting that to the result type:

sil shared @conversion_thunk : $@convention(thin) (@in_guaranteed Int) -> @out Optional<Int>

But basically, the pattern of emitThunk holds: you will build a thunk type, get-or-create the thunk function (i.e. mangling it and creating it in the SILModule if it doesn't already exist), fill in the body if it's empty, build a reference to it in the current SIL function, and then convert that to the expected function type using either a thin-to-thick conversion or a partial_apply. (A partial_apply may be necessary if the converted types refer to archetypes of the current context.)

The body of the thunk will be much simpler; you'll just collect the indirect result parameter and the value parameter and then call emitTransformedValue. You'll use the result address as the destination for the transform by passing an SGFContext that holds a TemporaryInitialization for that address.

3 Likes