Fix invalid rewrite in UseExplicitNilCheckInConditions by skipping when explicit optional type annotation is present #1075
Conversation
TTOzzi requested review from allevato, bnbarham, hamishknight and rintaro as code owners 
| /// We skip rewriting in this case. While we could preserve the coercion by rewriting | ||
| /// to `(expr as S?) != nil`, skipping is safer—it avoids potential changes in operator | ||
| /// precedence and keeps the original semantics intact. | ||
| private func hasOptionalTypeAnnotation(_ bindingCondition: OptionalBindingConditionSyntax) -> Bool { |
There was a problem hiding this comment.
The fact that the type in the example in the issue is Optional isn't actually relevant here: the problem is that the type annotation is being used to satisfy type inference for a function that only mentions a type by return value. The same code would be a problem if it was written like this:
if let _: S = getValue(forKey: "fnord") { ... }Because changing that to if getValue(forKey: "fnord") != nil means that the compiler has lost the context that the user wanted the generic parameter T to getValue to be specifically S. The even deeper problem is that in this case, if T doesn't have any constraints, the compiler will just pick T == Any, which is enough to successfully compile but is not what the user intended at all if they actually try to do something with that metatype—we've changed the meaning of their program!
So we should just check for any type annotation being present and skip the transformation.
There was a problem hiding this comment.
Ah, I see.
// error: Cannot convert value of type 'any A' to specified type 'S' if let _: S = getValue(forKey: "fnord") { } func getValue(forKey: String) -> A? { return nil } protocol A { } class S: A { }I’d only been thinking about cases like this—where a type mismatch would produce a compile error—so I assumed that when it wasn’t Optional we could always skip it.
But in a case like:
if let _: A = getValue(forKey: "fnord") { } func getValue(forKey: String) -> S? { return nil } protocol A { } class S: A { }the type annotation can carry meaningful intent, just as you mentioned.
Thank you for the feedback. I’ll make the change!
TTOzzi force-pushed the fix-UseExplicitNilCheckInConditions branch from cb994a7 to 5c91359 Compare | /// Note: If the conditional binding carries an explicit type annotation (e.g. `if let _: S? = expr` | ||
| /// or `if let _: S = expr`), we skip the transformation. Such annotations can be necessary to |
There was a problem hiding this comment.
IMO having both examples is redundant; I would drop the one with S?.
…en explicit optional type annotation is present
TTOzzi force-pushed the fix-UseExplicitNilCheckInConditions branch from 5c91359 to 5bd4ec0 Compare 2 participants
Resolve #1066