Borrowed value with `as` casting

Development Compiler
, , , ,
1

Hi,

I'm a mentee participating in the Swift Mentorship program. I have interest in the ownership system but I'm fairly new to the compiler implementation. I am picking up issue Compiler crash: `borrowing` parameter in actor method captured by closure · Issue #86458 · swiftlang/swift · GitHub (and SIL verification failed: init_existential_ref operand must be lowered to the right abstraction level for the existential · Issue #86530 · swiftlang/swift · GitHub) to as a starter, but I'm not sure if my solution is sound.

PR: Fixes #86458 and #86530 by FlickerSoul · Pull Request #90656 · swiftlang/swift · GitHub

The code that breaks when borrowing is introduced

final class S {}

func f(_ s: borrowing S) {
    s === s
}

It produces SIL like the following

bb0(%0 : @noImplicitCopy @guaranteed $S):
  %1 = copyable_to_moveonlywrapper [guaranteed] %0 // user: %2
  %2 = copy_value %1                              // user: %3
  %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 // users: %20, %9, %5, %4
  debug_value %3, let, name "s", argno 1          // id: %4
  %5 = begin_borrow %3                            // users: %18, %6
  %6 = copy_value %5                              // user: %7
  %7 = init_existential_ref %6 : $@moveOnly S : $S, $AnyObject // user: %8
... more after

because the introduction of borrowing keyword, in the argument setup phase, the argument variable is wrapped with a wrapper that annotates moveOnly. The computation of %7 crashes,

%7 = init_existential_ref %6 : $@moveOnly S : $S, $AnyObject

because when the SIL verifier verifies that if $@moveOnly S can be the same as $S, with the requireSameType assertioin, near the SILVerifier.cpp:5067

    SILType concreteType = IEI->getOperand()->getType();
    // omitted ....
    // The operand must be at the right abstraction level for the existential.
    SILType exType = IEI->getType();
    auto archetype = ExistentialArchetypeType::get(exType.getASTType());
    auto loweredTy = F.getLoweredType(Lowering::AbstractionPattern(archetype),
                                      IEI->getFormalConcreteType());
    requireSameType(concreteType, loweredTy,
                    "init_existential_ref operand must be lowered to the right "
                    "abstraction level for the existential");

My intuition tells me that the the moveOnly container needs to be "reverted", so a solution would be inserting a createOwnedMoveOnlyWrapperToCopyableValue instruction before doing the existential erasure. (After reading the documents more, it appears that @moveOnly is only part of SIL, not in formal type, and therefore causing the SIL verifier assertion failure, which wants the the type of the operand on the left side of as to be the same as the concrete formal type)

ManagedValue SILGenFunction::emitExistentialErasure(
                            SILLocation loc,
                            CanType concreteFormalType,
                            const TypeLowering &concreteTL,
                            const TypeLowering &existentialTL,
                            ArrayRef<ProtocolConformanceRef> conformances,
                            SGFContext C,
                            llvm::function_ref<ManagedValue (SGFContext)> F,
                            bool allowEmbeddedNSError) {
  // omitted ....
  // SILGenConvert.cpp:810
  case ExistentialRepresentation::Class: {
    assert(existentialTL.isLoadable());

    ManagedValue sub = F(SGFContext());

    // Unwrap the wrapper according to the value's
    // ownership: for an owned value this is a consuming use, which the
    // move-only checker will diagnose against borrowed values
    if (sub.getType().isMoveOnlyWrapped()) {
      if (sub.isPlusOne(*this)) {
        sub = B.createOwnedMoveOnlyWrapperToCopyableValue(loc, sub);
      } else {
        sub = B.createGuaranteedMoveOnlyWrapperToCopyableValue(loc, sub);
      }
    }

    return B.createInitExistentialRef(loc, existentialTL.getLoweredType(),
                                      concreteFormalType, sub, conformances);
  }
  // omitted ...
}

which produces the following, and seems to fix the crashing issue

bb0(%0 : @noImplicitCopy @guaranteed $S):
  %1 = copyable_to_moveonlywrapper [guaranteed] %0 // user: %2
  %2 = copy_value %1                              // user: %3
  %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 // users: %22, %10, %5, %4
  debug_value %3, let, name "s", argno 1          // id: %4
  %5 = begin_borrow %3                            // users: %20, %6
  %6 = copy_value %5                              // user: %7
  %7 = moveonlywrapper_to_copyable [owned] %6     // user: %8
  %8 = init_existential_ref %7 : $S : $S, $AnyObject // user: %9

After the fix is introduced, the compiler doesn't crash anymore

❯ ../build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/bin/swiftc 86458.swift
86458.swift:4:7: warning: result of operator '===' is unused [#NoUsage]
2 |
3 | func f(_ s: borrowing S) {
4 |     s === s
  |       `- warning: result of operator '===' is unused [#NoUsage]
5 | }
6 |

86458.swift:3:10: error: 's' is borrowed and cannot be consumed
1 | final class S {}
2 |
3 | func f(_ s: borrowing S) {
  |          `- error: 's' is borrowed and cannot be consumed
4 |     s === s
  |     |     `- note: consumed here
  |     `- note: consumed here
5 | }
6 |

It looks like my approach only fixes the branch when existential representation is a class and may not be comprehensive. I vaguely remember if a type resides in other modules, it may be address only (opaque?), and may have similar issue, which I haven't checked yet.

Please let me know what you think and I appreciate your feedback!!

Thanks!

Best regards,
Larry

1 Like
Existential casting of borrowed values
2

With the disclaimer that I'm no expert in this domain, your reasoning seems logical to me. From looking at the code a bit, I began to wonder about a few things.

First, the issue you're tackling with existentials seems to have two different problems – the SIL verifier rejects the code SILGen produces, and the SILMoveOnlyWrappedTypeEliminatorVisitor hits an unreachable when it encounters the @moveOnly type in an InitExistentialRef instruction. This means in asserts builds the compiler bails in the SIL passes, but in release builds (where SIL verification is often disabled), the compiler hits the later crash.

I can imagine "fixing" this in a few possible ways (not mutually exclusive):

  1. Your proposed approach, which will avoid the verification error and subsequent crash
  2. Updating the wrapper elimination logic to treat InitExistentialRef as a NO_UPDATE_NEEDED case, as it does for many other instructions
  3. Adjusting the verifier logic to tolerate differences in "move-only-wrapper-ness" of SIL types (possibly a Bad Idea... but not entirely clear to me)

Just updating the elimination pass (option 2) to be more tolerant seemingly allows the compiler to get far enough along to detect an invalid implicit copy with no other changes, but unless the verifier is changed or the unwrapping instruction is emitted, that's probably not a good enough solution. Also, it's not clear to me what the logic is for how the instructions are handled in the wrapper elimination pass so it might not be appropriate to do that. Similarly, updating the verifier to be more lenient seems like it might be an incorrect workaround.

That said, personally I don't really find it clear when and what should be responsible for emitting explicit unwrapping conversions in SILGen. The existing code seems quite scattered, and doesn't seem to handle a number of cases. For example, the following all seem to have similar issues to the existential case (godbolt):

class T {}
final class S: T {}

// uncomment a line to see failure modes

func f(_ s: borrowing S) {
//   _ = s as AnyObject // fails verification; crashes when verification disabled

//   let _: S? = s // fails verification; errors when verification disabled

//   let _: T = s // fails verification; errors when verification disabled

//    _ = s as? T // fails verification; errors when verification disabled

//    _ = type(of: s) // does _not_ fail verification; crashes during moveonly elimination
}

struct Z {}

func g(_ z: borrowing Z) {
//    _ = z as AnyObject // no verifier error; crashes

//   let _: Z? = z  // same as class case above

//    _ = z as? Z // no verifier error; crashes
}

enum Q { case one }

func h(_ q: borrowing Q) {
//    _ = q as AnyObject // no verifier error; crashes

//    let _: Q? = q // same as class case above

//    _ = q as? Q // no verifier error; crashes
}

Do each of these cases need some more special emission logic in SILGen? I'm not sure... Anyway, I don't know if that was actually helpful or not, but I appreciate your working on this problem!

1 Like
3

That was super helpful! Thanks for suggesting all the approaches and more test cases. I also thought that my solution could only cover one case but not solve the root case. I'll take a deeper look into all the variants. Some of them don't crash in 6.3 but crash in 6.5 (main branch).

2 Likes
4

All the crashes can be fixed with similar approach: unwrapping the @moveOnly wrapper before the actual cast/erasure SIL. I updated my PR and tests to include fixes for those cases as well. :))

1 Like
5

Also found that the following would crash, but it can be fixed with similar approach.

class Base { func f() {} }

func test<T: Base>(_ a: borrowing T) {
    (a as Base).f()
}
1 Like