Check value casts are to non-foreign types #2754
Conversation
| @@ -663,6 +663,9 @@ ERROR(super_initializer_not_in_initializer,none, | |||
| "'super.init' cannot be called outside of an initializer", ()) | |||
| WARNING(isa_is_always_true,none, "'%0' test is always true", | |||
| (StringRef)) | |||
| WARNING(isa_is_foreign_check,none, | |||
| "'%0' test always succeeds because %1 is a foreign type", | |||
There was a problem hiding this comment.
This warning text needs to be improved or this term needs to be defined. Suggestions?
There was a problem hiding this comment.
What do we do for as!? Do we just use the generic message?
Unless @rjmccall has any objections, I'd be okay with just hardcoding "Core Foundation type" for now. I think I'd also change the message to say why the check always returns true (and avoid "succeeds").
There was a problem hiding this comment.
We do nothing for as!, but I can insert the diagnostic (though it's counter to the point of ! for one).
There was a problem hiding this comment.
Ah, maybe we don't diagnose for as! because there's no way to silence that diagnostic and it's sometimes useful…whereas this one isn't.
There was a problem hiding this comment.
Do you want to leave the conditional cast warning in? On the one hand the cast always succeeds, on the other it is the form we want people to be using.
There was a problem hiding this comment.
I'm fine with saying "Core Foundation type".
CodaFi
force-pushed
the
foreign-affairs
branch
6 times, most recently
from
f68d9ba to
95f5b87
Compare
|
Looks good to me. |
|
@swift-ci Please test |
|
@swift-ci Please test |
|
This is going to fail a bunch of tests. I pushed the wrong branch here. Give me a moment to amend this with the proper patch we discussed in the comments. |
|
There. Thanks! |
|
@swift-ci please test |
| WARNING(isa_is_always_true,none, "'%0' test is always true", | ||
| (StringRef)) | ||
| WARNING(isa_is_foreign_check,none, | ||
| "'is' test always succeeds because %0 is a CoreFoundation type", |
There was a problem hiding this comment.
Phrasing nitpick: "Core Foundation", not "CoreFoundation". I'm also still not happy with "succeeds" (as opposed to "returns true" or "is always true" or "evaluates to true") because it's not going to do a test at all.
There was a problem hiding this comment.
Phrasing nitpick: "Core Foundation", not "CoreFoundation".
...
I'm also still not happy with "succeeds"
Agree. Will fix.
because it's not going to do a test at all.
This is styled on the language of the warning above. I can change both if that is what is required.
There was a problem hiding this comment.
Yeah, but a cast does something in addition to a type-check. It's less obviously a "success" when it's just the type-check. (Thanks.)
There was a problem hiding this comment.
Precisely. We agree on that point. Should I change language with regards to the other?
CodaFi
force-pushed
the
foreign-affairs
branch
3 times, most recently
from
95f5b87 to
e880782
Compare
|
@swift-ci please test |
|
Linux failure is unrelated. |
|
One more time! @swift-ci Please test |
Per the discussion in SR-1612, we don’t have a robust mechanism for checking this and should warn about it. Signed-off-by: Robert Widmann <devteam.codafi@gmail.com>
|
Once more, with feeling! |
CodaFi
force-pushed
the
foreign-affairs
branch
2 times, most recently
from
e880782 to
dee7aaa
Compare
|
Yep, this passed a build ago. Let's see what happens now. |
|
I think we're good. Thanks, Robert! |
What's in this pull request?
Emits a warning when
iscasts are used with foreign types per the discussion on the attached SR.Resolved bug number: (SR-1612)
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.