Skip to content

Sema: Emit diagnostics when walking into collection literals with defaulted types #60323

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Sema: Emit diagnostics when walking into collection literals with defaulted types #60323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 46 additions & 27 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,15 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
SmallPtrSet<DeclRefExpr*, 4> AlreadyDiagnosedBitCasts;

bool IsExprStmt;
bool HasReachedSemanticsProvidingExpr;

public:
ASTContext &Ctx;
const DeclContext *DC;

public:
DiagnoseWalker(const DeclContext *DC, bool isExprStmt)
: IsExprStmt(isExprStmt), Ctx(DC->getASTContext()), DC(DC) {}
: IsExprStmt(isExprStmt), HasReachedSemanticsProvidingExpr(false),
Ctx(DC->getASTContext()), DC(DC) {}

std::pair<bool, Pattern*> walkToPatternPre(Pattern *P) override {
return { false, P };
Expand All @@ -128,6 +130,17 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
bool shouldWalkIntoTapExpression() override { return false; }

std::pair<bool, Expr *> walkToExprPre(Expr *E) override {

if (auto collection = dyn_cast<CollectionExpr>(E)) {
if (collection->isTypeDefaulted()) {
// Diagnose type defaulted collection literals in subexpressions as
// warnings to preserve source compatibility.
diagnoseTypeDefaultedCollectionExpr(
collection, Ctx,
/*downgradeToWarning=*/HasReachedSemanticsProvidingExpr);
}
}

// See through implicit conversions of the expression. We want to be able
// to associate the parent of this expression with the ultimate callee.
auto Base = E;
Expand Down Expand Up @@ -328,6 +341,11 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
checkMoveExpr(moveExpr);
}

if (!HasReachedSemanticsProvidingExpr &&
E == E->getSemanticsProvidingExpr()) {
HasReachedSemanticsProvidingExpr = true;
}

return { true, E };
}

Expand Down Expand Up @@ -444,23 +462,34 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
return false;
}

/// We have a collection literal with a defaulted type, e.g. of [Any]. Emit
/// an error if it was inferred to this type in an invalid context, which is
/// one in which the parent expression is not itself a collection literal.
void checkTypeDefaultedCollectionExpr(CollectionExpr *c) {
// If the parent is a non-expression, or is not itself a literal, then
// produce an error with a fixit to add the type as an explicit
// annotation.
if (c->getNumElements() == 0)
Ctx.Diags.diagnose(c->getLoc(), diag::collection_literal_empty)
.highlight(c->getSourceRange());
else {
/// Diagnose a collection literal with a defaulted type such as \c [Any].
static void diagnoseTypeDefaultedCollectionExpr(CollectionExpr *c,
ASTContext &ctx,
bool downgradeToWarning) {
// Produce a diagnostic with a fixit to add the defaulted type as an
// explicit annotation.
auto &diags = ctx.Diags;

if (c->getNumElements() == 0) {
InFlightDiagnostic inFlight =
diags.diagnose(c->getLoc(), diag::collection_literal_empty);
inFlight.highlight(c->getSourceRange());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we produce a fix-it in the non-empty case. Should a fix-it be produced here as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (downgradeToWarning) {
inFlight.limitBehavior(DiagnosticBehavior::Warning);
}
} else {
assert(c->getType()->hasTypeRepr() &&
"a defaulted type should always be printable");
Ctx.Diags.diagnose(c->getLoc(), diag::collection_literal_heterogeneous,
c->getType())
.highlight(c->getSourceRange())
.fixItInsertAfter(c->getEndLoc(), " as " + c->getType()->getString());
InFlightDiagnostic inFlight = diags.diagnose(
c->getLoc(), diag::collection_literal_heterogeneous, c->getType());
inFlight.highlight(c->getSourceRange());
inFlight.fixItInsertAfter(c->getEndLoc(),
" as " + c->getType()->getString());

if (downgradeToWarning) {
inFlight.limitBehavior(DiagnosticBehavior::Warning);
}
}
}

Expand Down Expand Up @@ -1334,16 +1363,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,

DiagnoseWalker Walker(DC, isExprStmt);
const_cast<Expr *>(E)->walk(Walker);

// Diagnose uses of collection literals with defaulted types at the top
// level.
if (auto collection
= dyn_cast<CollectionExpr>(E->getSemanticsProvidingExpr())) {
if (collection->isTypeDefaulted()) {
Walker.checkTypeDefaultedCollectionExpr(
const_cast<CollectionExpr *>(collection));
}
}
}


Expand Down
39 changes: 38 additions & 1 deletion test/Constraints/array_literal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -disable-availability-checking

struct IntList : ExpressibleByArrayLiteral {
typealias Element = Int
Expand Down Expand Up @@ -142,19 +142,56 @@ func defaultToAny(i: Int, s: String) {
// expected-error@-1{{empty collection literal requires an explicit type}}
let _: Int = a5 // expected-error{{value of type '[Any]'}}

let _: [Any] = []
let _: [Any] = [1, "a", 3.5]
let _: [Any] = [1, "a", [3.5, 3.7, 3.9]]
let _: [Any] = [1, "a", [3.5, "b", 3]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}
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}}

func f1() -> [Any] {
[]
}

let _: [Any?] = [1, "a", nil, 3.5]
let _: [Any?] = [1, "a", nil, [3.5, 3.7, 3.9]]
let _: [Any?] = [1, "a", nil, [3.5, "b", nil]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any?]'; add explicit type annotation if this is intentional}}
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}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. This change originated from @xedin's point earlier on about nested collection literals with a defaulted type.

Copy link
Contributor

@xedin xedin Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I missed that, sorry, this one is indeed correct but the result ones are mentioned are not I think.

Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we warn about let _: [Any] = []?

We should not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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] {
  []
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

func test() -> [Any] {
  []
}

@simanerush Could you add test cases for this and let _: [Any] = [] as you fix the conflict?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnthonyLatsis sure!

let _: [Any?] = [1, nil, [2, nil, [3]]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any?]'; add explicit type annotation if this is intentional}}

let a6 = [B(), C()]
let _: Int = a6 // expected-error{{value of type '[A]'}}

let a7: some Collection = [1, "Swift"]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}} {{41-41= as [Any]}}
let _: (any Sequence)? = [1, "Swift"]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}
let _: any Sequence = [1, nil, "Swift"]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any?]'; add explicit type annotation if this is intentional}}
let _ = [1, true, ([], 1)]
// expected-error@-1 {{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}
// expected-warning@-2 {{empty collection literal requires an explicit type}}
let _ = true ? [] : []
// expected-warning@-1{{empty collection literal requires an explicit type}}
let _ = (true, ([1, "Swift"]))
//expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}
let _ = ([1, true])
//expected-error@-1{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}

func f2<T>(_: [T]) {}

func f3<T>() -> [T]? {}

f2([])
// expected-warning@-1{{empty collection literal requires an explicit type}}
f2([1, nil, ""])
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[Any?]'; add explicit type annotation if this is intentional}}
_ = f3() ?? []
// expected-warning@-1{{empty collection literal requires an explicit type}}
}

func noInferAny(iob: inout B, ioc: inout C) {
Expand Down
1 change: 1 addition & 0 deletions test/Constraints/casts.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?, _ dict: [Strin

// The array can also be inferred to be [Any].
_ = ([] ?? []) as Array // expected-warning {{left side of nil coalescing operator '??' has non-optional type '[Any]', so the right side is never used}}
// expected-warning@-1{{empty collection literal requires an explicit type}}

// rdar://88334481 – Don't apply the compatibility logic for collection literals.
typealias Magic<T> = T
Expand Down
1 change: 1 addition & 0 deletions test/Constraints/casts_swift6.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?, _ dict: [Strin

// The array can also be inferred to be [Any].
_ = ([] ?? []) as Array // expected-warning {{left side of nil coalescing operator '??' has non-optional type '[Any]', so the right side is never used}}
// expected-warning@-1 {{empty collection literal requires an explicit type}}

// Cases from rdar://88334481
typealias Magic<T> = T
Expand Down
1 change: 1 addition & 0 deletions test/Constraints/construction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ SR_5245(s: SR_5245.S(f: [.e1, .e2]))

// rdar://problem/34670592 - Compiler crash on heterogeneous collection literal
_ = Array([1, "hello"]) // Ok
// expected-warning@-1 {{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}

func init_via_non_const_metatype(_ s1: S1.Type) {
_ = s1(i: 42) // expected-error {{initializing from a metatype value must reference 'init' explicitly}} {{9-9=.init}}
Expand Down
3 changes: 2 additions & 1 deletion test/Constraints/dictionary_literal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ func testDefaultExistentials() {
"b": ["a", 2, 3.14159],
"c": ["a": 2, "b": 3.5]]
// expected-error@-3{{heterogeneous collection literal could only be inferred to '[String : Any]'; add explicit type annotation if this is intentional}}
// expected-warning@-3{{heterogeneous collection literal could only be inferred to '[Any]'; add explicit type annotation if this is intentional}}

let d3 = ["b" : B(), "c" : C()]
let _: Int = d3 // expected-error{{value of type '[String : A]'}}
Expand Down Expand Up @@ -193,4 +194,4 @@ f59215(["", ""]) //expected-error{{dictionary of type '[String : String]' cannot
f59215(["", "", "", ""]) //expected-error{{dictionary of type '[String : String]' cannot be used with array literal}}
// expected-note@-1{{did you mean to use a dictionary literal instead?}} {{11-12=:}} {{19-20=:}}
f59215(["", "", "", ""]) //expected-error{{dictionary of type '[String : String]' cannot be used with array literal}}
// expected-note@-1{{did you mean to use a dictionary literal instead?}} {{11-12=:}} {{19-20=:}}
// expected-note@-1{{did you mean to use a dictionary literal instead?}} {{11-12=:}} {{19-20=:}}
1 change: 1 addition & 0 deletions test/Constraints/subscript.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ extension Int {
let _ = 1["1"] // expected-error {{ambiguous use of 'subscript(_:)'}}

let squares = [ 1, 2, 3 ].reduce([:]) { (dict, n) in
// expected-warning@-1 {{empty collection literal requires an explicit type}}
var dict = dict
dict[n] = n * n
return dict
Expand Down
10 changes: 10 additions & 0 deletions test/IDE/print_usrs_opaque_types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,34 @@
func testUnifyingGenericParams<T, U>(x: T) -> some Collection where T == U {
// expected-warning@-1 {{same-type requirement makes generic parameters 'U' and 'T' equivalent}}
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}

// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test0C22UnifyingGenericParams21xQrx_tSlRz7ElementQzRs_r0_lF
func testUnifyingGenericParams2<T, U>(x: T) -> some Collection where T: Collection, U == T.Element {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}

// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test0C24ConcretizingGenericParam1xQrSi_tSiRszlF
func testConcretizingGenericParam<T>(x: T) -> some Collection where T == Int {
// expected-warning@-1 {{same-type requirement makes generic parameter 'T' non-generic}}
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}

struct GenericContext<T> {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test14GenericContextV0c8UnifyingD6Params1xQrx_tqd__RszlF
func testUnifyingGenericParams<U>(x: T) -> some Collection where T == U {
// expected-warning@-1 {{same-type requirement makes generic parameters 'U' and 'T' equivalent}}
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}

// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test14GenericContextV0c8UnifyingD7Params21xQrx_tSlRz7ElementQzRsd__lF
func testUnifyingGenericParams2<U>(x: T) -> some Collection where T: Collection, U == T.Element {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}

// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test14GenericContextVyQrxcqd__Rszluip
Expand All @@ -40,6 +45,7 @@ struct GenericContext<T> {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test14GenericContextVyQrxcqd__Rszluig
get {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}
}
}
Expand All @@ -48,6 +54,7 @@ extension GenericContext where T == Int {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test14GenericContextVAASiRszlE0c12ConcretizingD5Param1xQrSi_tF
func testConcretizingGenericParam(x: T) -> some Collection {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}
}

Expand All @@ -58,19 +65,22 @@ extension TooGenericTooContext where T == U {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test010TooGenericD7ContextVAAq_RszrlE0c8UnifyingE6Params1xQrx_tF
func testUnifyingGenericParams(x: T) -> some Collection {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}
}

extension TooGenericTooContext where T: Collection, U == T.Element {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test010TooGenericD7ContextVAASlRz7ElementQzRs_rlE0c8UnifyingE7Params21xQrx_tF
func testUnifyingGenericParams2(x: T) -> some Collection {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}
}
extension TooGenericTooContext where T == Int {
// CHECK: [[@LINE+1]]:{{[0-9]+}} s:14swift_ide_test010TooGenericD7ContextVAASiRszrlE0c12ConcretizingE5Param1xQrSi_tF
func testConcretizingGenericParam(x: T) -> some Collection {
return []
// expected-warning@-1 {{empty collection literal requires an explicit type}}
}
}

4 changes: 4 additions & 0 deletions test/expr/cast/objc_coerce_array.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another instance of let _: [Any?] = [1, [2, [3]]] - we shouldn't diagnose it because there is an explicit coercion here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is interesting. Is the inner literal being marked as defaulted by mistake?

_ = ["x":["z",1]] as [String : [AnyObject]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[AnyObject]'; add explicit type annotation if this is intentional}}
_ = [["y":"z","a":1]] as [[String : AnyObject]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[String : AnyObject]'; add explicit type annotation if this is intentional}}
_ = [["z",1]] as [[AnyObject]]
// expected-warning@-1{{heterogeneous collection literal could only be inferred to '[AnyObject]'; add explicit type annotation if this is intentional}}

var y: Any = 1

Expand Down