Skip to content

[CS] Improve placeholder diagnostics slightly #75046

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions include/swift/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@ enum class FixKind : uint8_t {
/// property wrapper.
AllowWrappedValueMismatch,

/// Ignore an out-of-place placeholder type.
IgnoreInvalidPlaceholder,

/// Specify a type for an explicitly written placeholder that could not be
/// resolved.
SpecifyTypeForPlaceholder,
Expand Down Expand Up @@ -3133,6 +3136,29 @@ class SpecifyContextualTypeForNil final : public ConstraintFix {
}
};

class IgnoreInvalidPlaceholder final : public ConstraintFix {
IgnoreInvalidPlaceholder(ConstraintSystem &cs, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::IgnoreInvalidPlaceholder, locator) {}

public:
std::string getName() const override {
return "ignore out-of-place placeholder type";
}

bool diagnose(const Solution &solution, bool asNote = false) const override;

bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
return diagnose(*commonFixes.front().first);
}

static IgnoreInvalidPlaceholder *create(ConstraintSystem &cs,
ConstraintLocator *locator);

static bool classof(const ConstraintFix *fix) {
return fix->getKind() == FixKind::IgnoreInvalidPlaceholder;
}
};

class SpecifyTypeForPlaceholder final : public ConstraintFix {
SpecifyTypeForPlaceholder(ConstraintSystem &cs, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::SpecifyTypeForPlaceholder, locator) {}
Expand Down
5 changes: 5 additions & 0 deletions lib/Sema/CSBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2529,6 +2529,11 @@ TypeVariableBinding::fixForHole(ConstraintSystem &cs) const {
return std::make_pair(fix, /*impact=*/(unsigned)10);
}

// If the placeholder is in an invalid position, we'll have already
// recorded a fix, and can skip recording another.
if (cs.hasFixFor(dstLocator, FixKind::IgnoreInvalidPlaceholder))
return std::nullopt;

ConstraintFix *fix = SpecifyTypeForPlaceholder::create(cs, srcLocator);
return std::make_pair(fix, defaultImpact);
}
Expand Down
25 changes: 20 additions & 5 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8668,7 +8668,21 @@ bool MissingContextualTypeForNil::diagnoseAsError() {
return true;
}

bool InvalidPlaceholderFailure::diagnoseAsError() {
auto elt = getLocator()->castLastElementTo<LocatorPathElt::PlaceholderType>();
emitDiagnosticAt(elt.getPlaceholderRepr()->getLoc(),
diag::placeholder_type_not_allowed);
return true;
}

bool CouldNotInferPlaceholderType::diagnoseAsError() {
// When placeholder type appears in an editor placeholder i.e.
// `<#T##() -> _#>` we rely on the parser to produce a diagnostic
// about editor placeholder and glance over all placeholder type
// inference issues.
if (isExpr<EditorPlaceholderExpr>(getAnchor()))
return true;

// If this placeholder was explicitly written out by the user, they can maybe
// fix things by specifying an actual type.
if (auto *typeExpr = getAsExpr<TypeExpr>(getAnchor())) {
Expand All @@ -8678,12 +8692,13 @@ bool CouldNotInferPlaceholderType::diagnoseAsError() {
}
}

// When placeholder type appears in an editor placeholder i.e.
// `<#T##() -> _#>` we rely on the parser to produce a diagnostic
// about editor placeholder and glance over all placeholder type
// inference issues.
if (isExpr<EditorPlaceholderExpr>(getAnchor()))
// Also check for a placeholder path element.
auto *loc = getLocator();
if (auto elt = loc->getLastElementAs<LocatorPathElt::PlaceholderType>()) {
emitDiagnosticAt(elt->getPlaceholderRepr()->getLoc(),
diag::could_not_infer_placeholder);
return true;
}

return false;
}
Expand Down
14 changes: 14 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -2621,6 +2621,20 @@ class MissingContextualTypeForNil final : public FailureDiagnostic {
bool diagnoseAsError() override;
};

/// Diagnose a placeholder type in an invalid place, e.g:
///
/// \code
/// y as? _
/// \endcode
class InvalidPlaceholderFailure final : public FailureDiagnostic {
public:
InvalidPlaceholderFailure(const Solution &solution,
ConstraintLocator *locator)
: FailureDiagnostic(solution, locator) {}

bool diagnoseAsError() override;
};

/// Diagnose situations where there is no context to determine the type of a
/// placeholder, e.g.,
///
Expand Down
12 changes: 12 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2123,6 +2123,18 @@ SpecifyContextualTypeForNil::create(ConstraintSystem &cs,
return new (cs.getAllocator()) SpecifyContextualTypeForNil(cs, locator);
}

bool IgnoreInvalidPlaceholder::diagnose(const Solution &solution,
bool asNote) const {
InvalidPlaceholderFailure failure(solution, getLocator());
return failure.diagnose(asNote);
}

IgnoreInvalidPlaceholder *
IgnoreInvalidPlaceholder::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) IgnoreInvalidPlaceholder(cs, locator);
}

bool SpecifyTypeForPlaceholder::diagnose(const Solution &solution,
bool asNote) const {
CouldNotInferPlaceholderType failure(solution, getLocator());
Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1676,9 +1676,9 @@ namespace {
TVO_CanBindToHole);
}
// Diagnose top-level usages of placeholder types.
if (isa<PlaceholderTypeRepr>(repr->getWithoutParens())) {
CS.getASTContext().Diags.diagnose(repr->getLoc(),
diag::placeholder_type_not_allowed);
if (auto *ty = dyn_cast<PlaceholderTypeRepr>(repr->getWithoutParens())) {
auto *loc = CS.getConstraintLocator(locator, {LocatorPathElt::PlaceholderType(ty)});
CS.recordFix(IgnoreInvalidPlaceholder::create(CS, loc));
}
return result;
}
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15446,6 +15446,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::DestructureTupleToMatchPackExpansionParameter:
case FixKind::AllowValueExpansionWithoutPackReferences:
case FixKind::IgnoreInvalidPatternInExpr:
case FixKind::IgnoreInvalidPlaceholder:
case FixKind::IgnoreOutOfPlaceThenStmt:
case FixKind::IgnoreMissingEachKeyword:
llvm_unreachable("handled elsewhere");
Expand Down
11 changes: 5 additions & 6 deletions test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1152,8 +1152,7 @@ func badTypes() {
// rdar://34357545
func unresolvedTypeExistential() -> Bool {
return (Int.self==_{})
// expected-error@-1 {{could not infer type for placeholder}}
// expected-error@-2 {{type placeholder not allowed here}}
// expected-error@-1 {{type placeholder not allowed here}}
}

do {
Expand Down Expand Up @@ -1552,19 +1551,19 @@ func testNilCoalescingOperatorRemoveFix() {
let _ = "" /* This is a comment */ ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{13-43=}}

let _ = "" // This is a comment
?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1554:13-1555:10=}}
?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{-1:13-+0:10=}}

let _ = "" // This is a comment
/*
* The blank line below is part of the test case, do not delete it
*/
?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1557:13-1561:10=}}
?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{-4:13-+0:10=}}

if ("" ?? // This is a comment // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{9-1564:9=}}
if ("" ?? // This is a comment // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{9-+1:9=}}
"").isEmpty {}

if ("" // This is a comment
?? "").isEmpty {} // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1566:9-1567:12=}}
?? "").isEmpty {} // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{-1:9-+0:12=}}
}

// https://github.com/apple/swift/issues/74617
Expand Down
3 changes: 1 addition & 2 deletions test/Constraints/if_expr.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ func testNil3(_ x: Bool) {
let _: _? = if x { 42 } else { nil }
}
func testNil4(_ x: Bool) {
// FIXME: Bad diagnostic (#63130)
let _: _? = if x { nil } else { 42 } // expected-error {{type of expression is ambiguous without a type annotation}}
let _: _? = if x { nil } else { 42 } // expected-error {{could not infer type for placeholder}}
}

enum F<T> {
Expand Down
3 changes: 1 addition & 2 deletions test/Constraints/switch_expr.swift
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,7 @@ func testNil3(_ x: Bool) {
let _: _? = switch x { case true: 42 case false: nil }
}
func testNil4(_ x: Bool) {
// FIXME: Bad diagnostic (#63130)
let _: _? = switch x { case true: nil case false: 42 } // expected-error {{type of expression is ambiguous without a type annotation}}
let _: _? = switch x { case true: nil case false: 42 } // expected-error {{could not infer type for placeholder}}
}

enum G<T> {
Expand Down
65 changes: 36 additions & 29 deletions test/Sema/placeholder_type.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let arr = [_](repeating: "hi", count: 3)
func foo(_ arr: [_] = [0]) {} // expected-error {{type placeholder may not appear in top-level parameter}}
// expected-note@-1 {{replace the placeholder with the inferred type 'Int'}}

let foo = _.foo // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
let foo = _.foo // expected-error {{type placeholder not allowed here}}
let zero: _ = .zero // expected-error {{cannot infer contextual base in reference to member 'zero'}}

struct S<T> {
Expand Down Expand Up @@ -110,15 +110,15 @@ extension Bar {
}

// FIXME: We should probably have better diagnostics for these situations--the user probably meant to use implicit member syntax
let _: Int = _() // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
let _: () -> Int = { _() } // expected-error 2 {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
let _: Int = _.init() // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
let _: () -> Int = { _.init() } // expected-error 2 {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
let _: Int = _() // expected-error {{type placeholder not allowed here}}
let _: () -> Int = { _() } // expected-error {{type placeholder not allowed here}}
let _: Int = _.init() // expected-error {{type placeholder not allowed here}}
let _: () -> Int = { _.init() } // expected-error {{type placeholder not allowed here}}

func returnsInt() -> Int { _() } // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
func returnsIntClosure() -> () -> Int { { _() } } // expected-error 2 {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
func returnsInt2() -> Int { _.init() } // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
func returnsIntClosure2() -> () -> Int { { _.init() } } // expected-error 2 {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
func returnsInt() -> Int { _() } // expected-error {{type placeholder not allowed here}}
func returnsIntClosure() -> () -> Int { { _() } } // expected-error {{type placeholder not allowed here}}
func returnsInt2() -> Int { _.init() } // expected-error {{type placeholder not allowed here}}
func returnsIntClosure2() -> () -> Int { { _.init() } } // expected-error {{type placeholder not allowed here}}

let _: Int.Type = _ // expected-error {{'_' can only appear in a pattern or on the left side of an assignment}}
let _: Int.Type = _.self // expected-error {{type placeholder not allowed here}}
Expand All @@ -145,17 +145,17 @@ let _ = [_].otherStaticMember.member
let _ = [_].otherStaticMember.method()

func f(x: Any, arr: [Int]) {
// FIXME: Better diagnostics here. Maybe we should suggest replacing placeholders with 'Any'?

if x is _ {} // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
if x is [_] {} // expected-error {{type of expression is ambiguous without a type annotation}}
if x is () -> _ {} // expected-error {{type of expression is ambiguous without a type annotation}}
if let y = x as? _ {} // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
if let y = x as? [_] {} // expected-error {{type of expression is ambiguous without a type annotation}}
if let y = x as? () -> _ {} // expected-error {{type of expression is ambiguous without a type annotation}}
let y1 = x as! _ // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
let y2 = x as! [_] // expected-error {{type of expression is ambiguous without a type annotation}}
let y3 = x as! () -> _ // expected-error {{type of expression is ambiguous without a type annotation}}
// TODO: Maybe we should suggest replacing placeholders with 'Any'?

if x is _ {} // expected-error {{type placeholder not allowed here}}
if x is [_] {} // expected-error {{could not infer type for placeholder}}
if x is () -> _ {} // expected-error {{could not infer type for placeholder}}
if let y = x as? _ {} // expected-error {{type placeholder not allowed here}}
if let y = x as? [_] {} // expected-error {{could not infer type for placeholder}}
if let y = x as? () -> _ {} // expected-error {{could not infer type for placeholder}}
let y1 = x as! _ // expected-error {{type placeholder not allowed here}}
let y2 = x as! [_] // expected-error {{could not infer type for placeholder}}
let y3 = x as! () -> _ // expected-error {{could not infer type for placeholder}}

switch x {
case is _: break // expected-error {{type placeholder not allowed here}}
Expand All @@ -166,15 +166,22 @@ func f(x: Any, arr: [Int]) {
case let y as () -> _: break // expected-error {{type placeholder not allowed here}}
}

if arr is _ {} // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
if arr is [_] {} // expected-error {{type of expression is ambiguous without a type annotation}}
if arr is () -> _ {} // expected-error {{type of expression is ambiguous without a type annotation}}
if let y = arr as? _ {} // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
if let y = arr as? [_] {} // expected-error {{type of expression is ambiguous without a type annotation}}
if let y = arr as? () -> _ {} // expected-error {{type of expression is ambiguous without a type annotation}}
let y1 = arr as! _ // expected-error {{type placeholder not allowed here}} expected-error {{type of expression is ambiguous without a type annotation}}
let y2 = arr as! [_] // expected-error {{type of expression is ambiguous without a type annotation}}
let y3 = arr as! () -> _ // expected-error {{type of expression is ambiguous without a type annotation}}
if case is _ = x {} // expected-error {{type placeholder not allowed here}}
if case is [_] = x {} // expected-error {{could not infer type for placeholder}}
if case is () -> _ = x {} // expected-error {{could not infer type for placeholder}}
if case let y as _ = x {} // expected-error {{type placeholder not allowed here}}
if case let y as [_] = x {} // expected-error {{could not infer type for placeholder}}
if case let y as () -> _ = x {} // expected-error {{could not infer type for placeholder}}

if arr is _ {} // expected-error {{type placeholder not allowed here}}
if arr is [_] {} // expected-error {{could not infer type for placeholder}}
if arr is () -> _ {} // expected-error {{could not infer type for placeholder}}
if let y = arr as? _ {} // expected-error {{type placeholder not allowed here}}
if let y = arr as? [_] {} // expected-error {{could not infer type for placeholder}}
if let y = arr as? () -> _ {} // expected-error {{could not infer type for placeholder}}
let y1 = arr as! _ // expected-error {{type placeholder not allowed here}}
let y2 = arr as! [_] // expected-error {{could not infer type for placeholder}}
let y3 = arr as! () -> _ // expected-error {{could not infer type for placeholder}}

switch arr {
case is _: break // expected-error {{type placeholder not allowed here}}
Expand Down
2 changes: 1 addition & 1 deletion test/expr/expressions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,7 @@ func r20802757(_ z: inout Int = &g20802757) { // expected-error {{cannot provide
print(z)
}

_ = _.foo // expected-error {{type placeholder not allowed here}} expected-error {{could not infer type for placeholder}}
_ = _.foo // expected-error {{type placeholder not allowed here}}

// <rdar://problem/22211854> wrong arg list crashing sourcekit
func r22211854() {
Expand Down