-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Dont trigger for non-optional type casting, or for optional types #5805
base: main
Are you sure you want to change the base?
Dont trigger for non-optional type casting, or for optional types #5805
Conversation
Generated by 🚫 Danger |
2020e54
to
4999b47
Compare
4999b47
to
efa6b93
Compare
efa6b93
to
1b33458
Compare
@@ -24,6 +24,9 @@ struct PreferTypeCheckingRule: Rule { | |||
foo.run() | |||
} | |||
"""), | |||
Example("bar as Foo? != nil"), | |||
Example("bar as Foo? != nil"), |
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.
This line is a duplicate.
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.
🤦
@@ -24,6 +24,9 @@ struct PreferTypeCheckingRule: Rule { | |||
foo.run() | |||
} | |||
"""), | |||
Example("bar as Foo? != nil"), | |||
Example("bar as Foo? != nil"), | |||
Example("bar as? Foo? != nil"), |
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.
This could actually be written as bar is Foo
, couldn't it?
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.
I think that depends on whether bar is nil or not:
var bar: String? = nil
print(bar as? String? != nil)
print(bar is String?)
print(bar is String)
prints
true
true
false
but
var bar: String? = "foo"
print(bar as? String? != nil)
print(bar is String?)
print(bar is String)
prints
true
true
true
var asExprWithOptionalTypeChecking: AsExprSyntax? { | ||
if let asExpr = leftOperand.as(AsExprSyntax.self), | ||
asExpr.questionOrExclamationMark?.tokenKind == .postfixQuestionMark, | ||
asExpr.type.is(OptionalTypeSyntax.self) == false, |
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.
asExpr.type.is(OptionalTypeSyntax.self) == false, | |
!asExpr.type.is(OptionalTypeSyntax.self), |
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.
This rule doesn't consider that !=
is commutative. So cases like nil != bar as? Int
are not found.
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.
That is true - I'm not sure how common that is, but it should be pretty easy to cover
63e1ee5
to
bddd8c2
Compare
Addresses #5802
We no longer trigger unless the type casting is optional (
as?
), or if the type is optional.Essentially, in either case, the correction would change the resulting
true
orfalse
value. See #5802 for details.We also now check for
nil != foo as? Bar
cases