[SR-1594] Provide a better diagnostic for nil comparisons. #2755
Conversation
|
Looks good. You might also consider the case of the UnsafePointer family. Please include a test case that shows your new diagnostic. Thanks! |
|
Good idea. We can provide a fixit too. |
|
@lattner How does it look now? |
CodaFi
changed the title
CodaFi
force-pushed
the
triple-value-meal
branch
2 times, most recently
from
4000daa to
abf5bf4
Compare
| class SR1594 { | ||
| func sr1594(bytes : UnsafeMutablePointer<Int>, _ i : Int?) { | ||
| _ = (i === nil) // expected-error {{value of type 'Int?' does not have reference semantics, comparison isn't allowed}} | ||
| _ = (bytes === nil) // expected-error {{pointer value of type 'UnsafeMutablePointer<Int>' cannot be compared by reference; did you mean to compare by value?}} |
There was a problem hiding this comment.
Should be checking the fixit for replacing the operator here too {{16-19===}}
|
@CodaFi I think the old existing diagnostic I'd like to see a fixit for the Finally, you probably shouldn't |
|
I agree, I think that "type %0 does not have reference semantics, comparison isn't allowed" is actively misleading, because it could causes someone to think that nullability goes with reference types. Something more along the lines of "type %0 is not optional, value cannot be nil" seems better. |
|
All excellent suggestions. Give me a moment to re-run the test suite on my end and we should be good to go. |
CodaFi
force-pushed
the
triple-value-meal
branch
2 times, most recently
from
6a0651d to
1325a52
Compare
|
LGTM! |
|
I don't think this patch is the best it can be yet. I'd suggest removing the reference_type_comparison_with_nil_illegal case entirely, since the more general message is better - just remove the " if (lhsType->hasReferenceSemantics()) {" code. The "getAnyPointerElementType() || lhsType->getAnyOptionalObjectType" may be correct, but it is confusing to me. I'd suggest making it more specific with something like: if (lhsType->getAnyOptionalObjectType() && (overloadName == "!==" || overloadName == "===")) Also, I don't think it is going to be a common error for people to use ===/!=== with UnsafePointer, I think they are most likely to use things like "!= nil" because they used to be nullable and now they aren't anymore. As such, I'd just special case the "type %0 is not optional, value can never be nil" message to say something like "type %0 cannot point to nil, use an %0? to achieve that" or something. |
|
If you choose to include the unsafe pointer case, please add a test case for it as well. |
|
It doesn't seem worth it to me to specialize for the pointer cases (yet, who knows?). I'll change the condition and remove the extraneous diagnostic. |
|
Sounds good, thanks! |
|
@swift-ci please test and merge |
|
Sorry about that. More test changes that I expected there with that last change. |
|
@swift-ci please test and merge |
What's in this pull request?
Comparisons to nil are already suspect, but the diagnostic about it applied only to types with reference semantics, which
Int?in the linked SR most certainly does not have. This patch differentiates the error for types with reference and value semantics.Resolved bug number: (SR-1594)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.