-
Notifications
You must be signed in to change notification settings - Fork 252
Fix invalid rewrite in UseExplicitNilCheckInConditions by skipping when explicit optional type annotation is present #1075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
cb994a7
to
5c91359
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO having both examples is redundant; I would drop the one with S?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied!
…en explicit optional type annotation is present
5c91359
to
5bd4ec0
Compare
Resolve #1066