-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Expose reason for unchecked local class warning #16086
Conversation
1dba232
to
e980e8f
Compare
e980e8f
to
417c9f5
Compare
The tabloid headline: "Unchecked Warning: Exposed!" |
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.
Otherwise LGTM
* 1. if `X <:< P`, TRUE | ||
* 2. if `P` is a singleton type, TRUE | ||
* 3. if `P` refers to an abstract type member or type parameter, FALSE | ||
* 1. if `X <:< P`, "" |
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'd prefix this with a comment that the method returns a string explaining why the type is not checkable, or the empty string if it is checkable.
if (!isTrusted && !checkable(expr.tpe, argType, tree.span)) | ||
report.uncheckedWarning(i"the type test for $argType cannot be checked at runtime", expr.srcPos) | ||
transformTypeTest(expr, tree.args.head.tpe, | ||
val unchecked = expr.tpe.widenTermRefExpr.hasAnnotation(defn.UncheckedAnnot) |
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.
No need to encode isTrusted as empty strings.
I think renaming checkable
to whyUncheckable
and changing the logic as below would make things clearer.
val isUnchecked = expr.tpe.widenTermRefExpr.hasAnnotation(defn.UncheckedAnnot)
if !isTrusted && !isUnchecked then
val whyNot = whyUncheckable(expr.tpe, argType, tree.span)
if !whyNot.isEmpty then
report.uncheckedWarning
No description provided.