[Sema] Improve Optional-to-Any diagnostics for collections, nested optionals & IUOs #14221
Conversation
lib/Sema/MiscDiagnostics.cpp
Outdated
| // to Any coercions (this doesn't take into consideration implicit | ||
| // coercions such as Any?! to Any?, however; we warn on those). | ||
| if (subExpr->getType()->getImplicitlyUnwrappedOptionalObjectType()) | ||
| return; |
There was a problem hiding this comment.
It would be great if we could remove this, which would enable warnings for implicit IUO to Any coercions, such as shown in the dontWarnIUOToAnyCoercion function in the tests (which should really produce warnings, as they're not being implicitly unwrapped; they're just being quietly erased to Any). With IUOs being removed from the type system, I believe this should happen at some point; but not sure if it's okay to just enable them this PR.
There was a problem hiding this comment.
I would say feel free to warn about the IUOs as well. It will fall out from my last commit to always generate Optional from '!', but there is no reason not to do so now, and it's really inconsistent that we haven't been emitting the warnings in that case as well.
There was a problem hiding this comment.
@rudkx Thanks, I'll go ahead and make that change then. It looks like we have 3 tests that make such implicit IUO coercions, two of which look like they can be updated with as Any without affecting the test (1 & 2). Though I'm not sure about the third one, can it be updated with an explicit as Any or as [Any], or would it be best to just add an expected warning?
There was a problem hiding this comment.
I would add the expected warning on the last one, but yes I think you're probably right that adding the coercions for the others is fine.
There was a problem hiding this comment.
@rudkx Thanks, I've gone ahead and done that now.
hamishknight
changed the title
hamishknight
changed the title
|
@swift-ci Please smoke test |
…ion type This will let us customise the diagnostic for collections and nested optionals.
When implicitly coercing a nested optional to a less optional `Any`, e.g `Any??` to `Any?`, emit a slightly better diagnostic: Expression implicitly coerced from 'Any??' to 'Any?' and allow for silencing of the diagnostic with an explicit coercion, e.g `as Any?`.
When implicitly coercing a collection of optional elements to a collection of less-optional `Any` elements, e.g `[Any?]` to `[Any]`, emit a slightly better diagnostic: Expression implicitly coerced from '[Any?]' to '[Any]' and allow for silencing of the diagnostic with an explicit coercion, e.g `as [Any]`.
For example, the implicit coercion from `Int!` to `Any` now produces a warning. Previously a warning wasn't emitted despite the optional being implicitly erased to `Any` (but warnings were emitted in some other cases, such as `Int?!` to `Any?`; this commit fixes that inconsistency).
hamishknight
force-pushed
the
optional-to-any-diag
branch
from
cc1b5b3 to
f2b804d
Compare
As IUOs types are no longer generated, we don't show them in diagnostics.
hamishknight
force-pushed
the
optional-to-any-diag
branch
from
f2b804d to
416cb7f
Compare
|
@swift-ci Please smoke test |
|
I'll take a closer look at the changes as well. |
|
LGTM. I'll merge. That will result in conflicts in another of my PR but I'll get that fixed up ASAP. |
When implicitly coercing a collection of optional elements to a collection of less-optional
Anyelements, e.g[Any?]to[Any], emit a slightly better diagnostic:and allow for silencing of the diagnostic with an explicit coercion, e.g
as [Any].When implicitly coercing a nested optional to a less optional
Any, e.gAny??toAny?, emit a slightly better diagnostic:and allow for silencing of the diagnostic with an explicit coercion, e.g
as Any?.(both diagnostics were previously "Expression implicitly coerced from 'Any?' to Any", and neither could be silenced by an explicit coercion to their destination types)
In addition, this PR enables Optional-to-Any warnings for IUOs, e.g the implicit coercion from
Int!toAnynow produces a warning. Previously a warning wasn't emitted despite the optional being implicitly erased toAny(but warnings were emitted in some other cases, such asInt?!toAny?; this PR fixes that inconsistency).Resolves SR-2928 & SR-6859.