Should `OptionalTryExpression` changes require SILGen changes too?

Development Compiler
1

I've been working on an implementation for changes to try? handling (as discussed here: Make try? + optional chain flattening work together - #15 by Avi). This is my first foray into the swift codebase. @john_mccall suggested that I'd need to make changes to CSGen and CSApply, and I think I've made both of those changes correctly.

But then I found I needed to make changes in ASTValidator and SILGenExpr as well. I believe those changes are correct, but I would like a little bit of confirmation that I'm on the right road, and that SILGen changes would be expected for this feature.

Assuming that's correct, I have a question on how to proceed. In RValueEmitter::visitOptionalTryExpr(), there's a branch based on isByAddress. I think I've figured out the false branch, but I'm not sure how to handle the true branch.

  SILValue branchArg;
  if (isByAddress) {
    assert(optInit);
    SILValue optAddr = optInit->getAddressForInPlaceInitialization(SGF, E);

    // ** I don't want to wrap the subExpr result in another optional anymore,
    // but I'm not sure what I shoulld be doing instead. **
    SGF.emitInjectOptionalValueInto(E, E->getSubExpr(), optAddr, optTL);

  } else {
    ManagedValue subExprValue = SGF.emitRValueAsSingleValue(E->getSubExpr());

    // ** I removed these **
    // ManagedValue wrapped = SGF.getOptionalSomeValue(E, subExprValue, optTL);
    // branchArg = wrapped.forward(SGF);

    // ** And added this instead **
    branchArg = subExprValue.forward(SGF);
  }

In most places, I've been able to look at how OptionalEvaluationExpr is handled and do something similar, but the implementation of RValueEmitter::visitOptionalEvaluationExpr() is a bit beyond my understanding at the moment.

Is there an an example somewhere of what I should be doing in the isByAddress == true branch here?

2

Oh, I hadn't thought about that. I would suggest changing the AST in the old case, too, so that the sub-expression is always of the same type as the OptionalTryExpr. In CSApply, that just means wrapping it in a InjectIntoOptionalExpr. Then you change the verifier to unconditionally check that the types match instead of being optional-and-its-object, and the SILGen code just emits the sub-expression without needing to directly worry about wrapping it in .some.

3

I can't help with the compiler changes, but I do have a question.

Are the changes you've done for any try? expression, or only when regular Optional unwrapping will be taking place, such as with if let ...?

4

I anticipate that this would apply to any try? statement; that seems like a simpler and more consistent model.

5

While I have never wanted the double-Optional behavior, I am not sure we should eliminate it completely. As it stands today, when if let x = try? ... gives x a value of nil, you know an exception was thrown, even if you don't know which. That may be valuable to someone out there, and you would be breaking such code without providing a simple work-around. (The non-simple work-around is to use try instead of try?, but that can complicate the resultant code.)

If it doesn't complicate the implementation over-much, I would recommend limiting the try? folding to expressions where that is the obvious implicit intent, namely: if let and guard let.

func returnOrThrow(_ x: Int?, throw: Bool) throws -> Int? {
   if throw { throw SomeError }
   return x
}

let a: Int? = nil
let b: Int? = 3

if let x = try? returnOrThrow(a, false) {
    // x == nil
}

if let x = try? returnOrThrow(a, true) {
    // x == nil
}

if let x = try? returnOrThrow(b, false) {
    // x == 3
}

if let x = try? returnOrThrow(b, true) {
    // x == nil
}

let x = try? returnOrThrow(a, false) // x == Optional(nil)
let x = try? returnOrThrow(a, true)  // x == nil
let x = try? returnOrThrow(b, false) // x == Optional(Optional(3))
let x = try? returnOrThrow(b, true)  // x == nil

P.S. I'm not tied to this. I can't think of a case where the compiler would not warn of unwrapping a non-Optional after these changes go into effect. As long as the developer can get the same information in some way, consistency of the try? operator may be more important than leaving a back-door to get the current behavior by lifting the expression out of the if let into a separate assignment.

6

I strongly disagree. try? should have a single and consisitent semantics (whether existing or modified) that is used everywhere, not one that is context sensitive.

2 Likes
7

Not that my opinion means much to begin with, but I'm not really attached to it.

However, consider the following:

let y: Uint? = 5
let x = y      // x is Optional<UInt>

if let x = y { // x is UInt
}

The difference between the sub-expression let x = y is the context. One triggers unwrapping, one doesn't. I don't think it's too inconsistent for try?'s behavior to be similarly dependent on context.

8

@john_mccall: I've got a partial implementation of making the sub-expression type equal to the try? expression type. I still have a couple questions about the SILGen component of this, though. I understand I can just emit the sub-expression directly, but I still need to capture the result in order to set it to nil for the throwing case, so I can't just forward directly to the subexpression and be done. I have a couple questions about how that's supposed to be done.

I've put up a PR on my own fork to make it easier to see and discuss my current changes. I'm happy to look at comments in-line there, if that's easier.

9

Sure, I'll comment there.

10

Thanks so much. I know this is a busy time of year.

11

Okay, I think I have nearly everything done on my local branch. The only thing remaining is one failing SILGen test.

1 Like
12

Thank you for your help! A pull request has been added for the implementation, and a swift-evolution proposal submitted.

I still need to add Swift 4 compatibility mode (which I think should be easy; just check LangOpts.EffectiveLanguageVersion?), and add a migration. I assume that I can look at things like TypeOfMigratorPass to see how that should be done. Is there any thing else I need to be aware of?

13

What migration do you intend to add? (try? E) => (try? (E as EType?))?

14

My thought was that we could possibly recognize the if let optX = try? E, let x = optX pattern. But it may be less common than I had anticipated, and may not merit migration.

15

There's definitely a best-effort curve with migrations. But it'd be nice (assuming this is accepted) to make some effort to make it migrate well.

16

Doing a migration that preserves the existing semantics is tricky, because we are removing the ability to distinguish between “an error occurred” and “nil is the intended result”. Even if we use as? to end up with the backward-compatible double optional type, it probably won’t have the same distinction between .some(nil) and nil.

If a migration needs to preserve the ability to detect the error case, it will probably need to migrate to do/try/catch. It’s encouraging, though, that every case I found in the compatibility suite, the code did not make that distinction.

17

My idea was to try to recognize the cases where the error case is not handled specially (which is likely the large majority of cases) by rewriting them to the simpler form enabled by this change, if a rewrite is needed at all. Then, if there are any cases where the error was handled specifically, we would leave those as errors to be handled by the developer. We might insert #error Use ‘try’ if you need to handle the error case or something.

I guess we could do this:

// Before:
let x = try? someExpression

// After:
let x: EType?
do {
    x = try .some(someExpression)
}
catch {
    x = nil
}

That rewrite preserves semantics. I’m not sure how you’d rewrite something like this, though:

let x = optThing() ?? (try? otherOptThing())
18

Does this patch give try? different behavior for concrete types (say String and Int?) vs protocols which are or may be implemented by Optional (CustomStringConvertible, Any) and generic parameters which could be specified as an Optional?

19

Generic types will be treated as non-optional, because the compiler has no knowledge of what the type is at compile-time. Nothing in this proposal is evaluated at runtime; it's all compile-time. So nothing changes for generic types under this proposal.

For example, the behavior of this program does is identical in both Swift 4 and in my implemented branch:

func test<T: Any>(_ arg: () throws -> T) {
	print("test<\(T.self)>")
	let x = try? arg()
	print("   x: \(type(of: x)) =", x as Any)
}

test({  return Optional<Int>(3) })
// test<Optional<Int>>
//    x: Optional<Optional<Int>> = Optional(Optional(3))

test({ return 3 })
// test<Int>
//   x: Optional<Int> = Optional(3)

However, the following code exhibits differing behavior:

func testExplicit(_ arg: () throws -> String?) {
	let x = try? arg()
	print("   x: \(type(of: x)) =", x as Any)
}

testExplicit({ return "Hi" })

In Swift 4, x is a String??. Under this proposal, it would be a String?.

20

To simplify: this proposal is based on whether a value is declared as optional, not whether it happens to contain an optional at run-time. It’s the declared type that matters.