-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[Compiler] add 'value' label for NSNumber
to prevent roundabout err…
#39261
Conversation
@xedin Please review. |
2b76a13
to
abe7008
Compare
@xedin Please review my new commit. How to write test cases? Enumerate all the types? |
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.
Looks much better! I added a couple of comments inline.
lib/Sema/CSDiagnostics.h
Outdated
@@ -701,6 +701,31 @@ class ContextualFailure : public FailureDiagnostic { | |||
|
|||
static Optional<Diag<Type, Type>> | |||
getDiagnosticFor(ContextualTypePurpose context, Type contextualType); | |||
|
|||
protected: | |||
bool exprNeedsParensBeforeAddingAs(const Expr *expr) const { |
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.
Let's not duplicate the logic here. You could extract these methods from MissingExplicitConversionFailure
into a static function in CSDiagnostics.cpp just need to add DeclContext *
as an argument to both of them.
test/Constraints/rdar82828226.swift
Outdated
|
||
func f(_ n: NSNumber) {} | ||
let int: Int = 0 | ||
f(int) // expected-error {{cannot convert value of type 'Int' to expected argument type 'NSNumber'}} {{3-3=NSNumber(value: }} |
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.
You'd have to adjust this test case to make sure that as NSNumber
is used instead of NSNumber(value:)
.
Also please add some more test cases e.g. using NSInteger
and as an argument to an operator (you could add a custom one which accepts (NSNumber, NSNumber) to make that easier).
@xedin Please review. 'NSInteger' is alias of 'Int', 'Int' passed to 'NSInteger' do not happen anything. 'NSDecimal' renamed 'Decimal', 'Decimal(Int)' works well. 'NSUInteger' produce a error 'Cannot find type 'NSUInteger'. Anything to add about the test? |
Hm, I guess our mock SDK doesn't have these types. That's fine though! |
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.
Thank you!
@swift-ci please smoke test |
@swift-ci please test macOS platform |
@swift-ci Please test macOS platform |
Build failed |
@swift-ci Please smoke test MacOS Platform |
@swift-ci please test macOS platform |
Roundabout error message is viewed when
Int
is passed to whereNSNumber
is expected. NSNumber(value: ) should be suggested first.Resolves SR-15165.