Sema: Emit diagnostics when walking into collection literals with defaulted types #60323
Conversation
|
@swift-ci please smoke test macOS |
|
@swift-ci please smoke test macOS |
simanerush
force-pushed
the
collection-literal-bug-fix
branch
from
1a9e00f to
da4a910
Compare
simanerush
force-pushed
the
collection-literal-bug-fix
branch
4 times, most recently
from
b0e8cd1 to
83b4a8b
Compare
simanerush
force-pushed
the
collection-literal-bug-fix
branch
2 times, most recently
from
59348a1 to
5a87a95
Compare
simanerush
changed the title
|
@swift-ci please smoke test macOS |
simanerush
force-pushed
the
collection-literal-bug-fix
branch
from
5a87a95 to
bdcf5c9
Compare
lib/Sema/MiscDiagnostics.cpp
Outdated
| // Diagnose type defaulted collection literals in subexpressions as | ||
| // warnings to preserve source compatibility. | ||
| diagnoseTypeDefaultedCollectionExpr( | ||
| const_cast<CollectionExpr *>(collection), Ctx, |
There was a problem hiding this comment.
Why do we need a const_cast<> here the original E is not const right?
| let _: [Any?] = [1, [2, [3]]] | ||
| // expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}} |
There was a problem hiding this comment.
@simanerush @AnthonyLatsis I think we don't want to diagnose cases like this one (note that it's different from the following one where inner collection literal is heterogeneous) and the ones where [] is used in return i.e. https://github.com/apple/swift/pull/60323/files#diff-55b6c1da2887f7323cf5ddb64f0a2add1f0b6a1392a51113d20bbbd04f8ccfc7R12 because there is a contextual type in this case so it's equivalent to as [Any] or as [Any?] which we suggest to silence the warning.
There was a problem hiding this comment.
I think the compiler is correct in this particular case because there is no explicit element type specified for [2, [3]], which is defaulted to [Any] and coerced to Any? as an element of the outer [Any?] literal.
There was a problem hiding this comment.
Agree. This change originated from @xedin's point earlier on about nested collection literals with a defaulted type.
There was a problem hiding this comment.
Ah yes, I missed that, sorry, this one is indeed correct but the result ones are mentioned are not I think.
There was a problem hiding this comment.
IMO we are also correct to warn on return [] for the same reason. func f() -> some Collection { return [] } is analogous to let _: some Collection = []: there is nothing to infer the element type from.
There was a problem hiding this comment.
Would we warn about
let _: [Any] = []?
We should not.
There was a problem hiding this comment.
@simanerush It must have been an older commit or something but I could swear I saw a test-case that got updated with the warning that returned [Any] similar to this one:
func test() -> [Any] {
[]
}
There was a problem hiding this comment.
Since it's not in the changes anymore I think we can land this with a // FIXME comment for all the test-cases mentioned in https://github.com/apple/swift/pull/60323/files#r942866744. That could be addressed separately.
There was a problem hiding this comment.
func test() -> [Any] { [] }
@simanerush Could you add test cases for this and let _: [Any] = [] as you fix the conflict?
| @@ -7,9 +7,13 @@ var x = 1 | |||
| _ = [x] as [NSNumber] | |||
|
|
|||
| _ = ["x":["y":"z","a":1]] as [String : [String : AnyObject]] | |||
| // expected-warning@-1{{heterogeneous collection literal could only be inferred to '[String : AnyObject]'; add explicit type annotation if this is intentiona}} | |||
There was a problem hiding this comment.
This is another instance of let _: [Any?] = [1, [2, [3]]] - we shouldn't diagnose it because there is an explicit coercion here.
There was a problem hiding this comment.
This one is interesting. Is the inner literal being marked as defaulted by mistake?
| if (c->getNumElements() == 0) { | ||
| InFlightDiagnostic inFlight = | ||
| diags.diagnose(c->getLoc(), diag::collection_literal_empty); | ||
| inFlight.highlight(c->getSourceRange()); |
There was a problem hiding this comment.
I see we produce a fix-it in the non-empty case. Should a fix-it be produced here as well?
There was a problem hiding this comment.
I think this was a conscious decision: coercing a type-defaulted collection literal to [Any] is far less likely to coincide with the programmer's intention if the literal is empty. Hence the tailored diag::collection_literal_empty that requires us to specify an unstated explicit type.
There was a problem hiding this comment.
I guess it makes sense, I just asked because to me we were suggesting explicit type but missing something actionable as the other case. Maybe could offer a fix-it with an element IDE placeholder e.g. as [<#Element#>]? But I agree is ok not emitting, that is just something to think about.
There was a problem hiding this comment.
Maybe could offer a fix-it with an element IDE placeholder e.g.
as [<#Element#>]?
Yeah, something along the lines of ; did you mean this to be an instance of 'Swift.Array'? plus this fix-it would probably be helpful.
simanerush
force-pushed
the
collection-literal-bug-fix
branch
3 times, most recently
from
b696c0c to
fe28cd6
Compare
…aulted types Resolves swiftlang#60011 Gardening: clean up `checkTypeDefaultedCollectionExpr` function
simanerush
force-pushed
the
collection-literal-bug-fix
branch
from
fe28cd6 to
f608802
Compare
|
@swift-ci please smoke test |
Resolves #60011