Skip to content

[TypeChecker] Produce a tailored diagnostic for for-in sequence fai… #27834

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
Oct 23, 2019
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
5 changes: 5 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -3397,6 +3397,11 @@ ERROR(unresolved_label_corrected,none,
"use of unresolved label %0; did you mean %1?",
(Identifier, Identifier))

ERROR(foreach_sequence_does_not_conform_to_expected_protocol,none,
"for-in loop requires %0 to conform to %1"
"%select{|; did you mean to unwrap optional?}2",
(Type, Type, bool))

// Switch Stmt
ERROR(no_match_operator,none,
"no binary '~=' operator available for 'switch' statement", ())
Expand Down
18 changes: 18 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@ Optional<Diag<Type, Type>> GenericArgumentsMismatchFailure::getDiagnosticFor(
return diag::cannot_convert_condition_value;

case CTP_ThrowStmt:
case CTP_ForEachStmt:
case CTP_Unused:
case CTP_CannotFail:
case CTP_YieldByReference:
Expand Down Expand Up @@ -1928,6 +1929,21 @@ bool ContextualFailure::diagnoseAsError() {
if (diagnoseYieldByReferenceMismatch())
return true;

if (CTP == CTP_ForEachStmt) {
if (FromType->isAnyExistentialType()) {
emitDiagnostic(anchor->getLoc(), diag::type_cannot_conform,
/*isExistentialType=*/true, FromType, ToType);
return true;
}

emitDiagnostic(
anchor->getLoc(),
diag::foreach_sequence_does_not_conform_to_expected_protocol,
FromType, ToType, bool(FromType->getOptionalObjectType()))
.highlight(anchor->getSourceRange());
return true;
}

auto contextualType = getToType();
if (auto msg = getDiagnosticFor(CTP, contextualType->isExistentialType())) {
diagnostic = *msg;
Expand Down Expand Up @@ -1965,6 +1981,7 @@ getContextualNilDiagnostic(ContextualTypePurpose CTP) {
return diag::cannot_convert_to_return_type_nil;

case CTP_ThrowStmt:
case CTP_ForEachStmt:
case CTP_YieldByReference:
return None;

Expand Down Expand Up @@ -2739,6 +2756,7 @@ ContextualFailure::getDiagnosticFor(ContextualTypePurpose context,
return diag::cannot_convert_condition_value;

case CTP_ThrowStmt:
case CTP_ForEachStmt:
case CTP_Unused:
case CTP_CannotFail:
case CTP_YieldByReference:
Expand Down
19 changes: 19 additions & 0 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3357,6 +3357,25 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
formUnsolvedResult);
}
}

// If the left-hand side of a 'sequence element' constraint
// is a dependent member type without any type variables it
// means that conformance check has been "fixed".
// Let's record other side of the conversion as a "hole"
// to give the solver a chance to continue and avoid
// producing diagnostics for both missing conformance and
// invalid element type.
if (shouldAttemptFixes()) {
if (auto last = locator.last()) {
if (last->is<LocatorPathElt::SequenceElementType>() &&
desugar1->is<DependentMemberType>() &&
!desugar1->hasTypeVariable()) {
recordHole(typeVar2);
return getTypeMatchSuccess();
}
}
}

return formUnsolvedResult();
}

Expand Down
15 changes: 11 additions & 4 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2908,6 +2908,8 @@ bool TypeChecker::typeCheckForEachBinding(DeclContext *dc, ForEachStmt *stmt) {
bool builtConstraints(ConstraintSystem &cs, Expr *expr) override {
// Save the locator we're using for the expression.
Locator = cs.getConstraintLocator(expr);
auto *contextualLocator =
cs.getConstraintLocator(expr, LocatorPathElt::ContextualType());

// The expression type must conform to the Sequence.
auto &tc = cs.getTypeChecker();
Expand All @@ -2924,11 +2926,16 @@ bool TypeChecker::typeCheckForEachBinding(DeclContext *dc, ForEachStmt *stmt) {
cs.addConstraint(ConstraintKind::Conversion, cs.getType(expr),
SequenceType, Locator);
cs.addConstraint(ConstraintKind::ConformsTo, SequenceType,
sequenceProto->getDeclaredType(), Locator);
sequenceProto->getDeclaredType(), contextualLocator);

auto elementLocator =
cs.getConstraintLocator(Locator,
ConstraintLocator::SequenceElementType);
// Since we are using "contextual type" here, it has to be recorded
// in the constraint system for diagnostics to have access to "purpose".
cs.setContextualType(
expr, TypeLoc::withoutLoc(sequenceProto->getDeclaredType()),
CTP_ForEachStmt);

auto elementLocator = cs.getConstraintLocator(
contextualLocator, ConstraintLocator::SequenceElementType);

// Collect constraints from the element pattern.
auto pattern = Stmt->getPattern();
Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ enum ContextualTypePurpose {
///< result type.
CTP_Condition, ///< Condition expression of various statements e.g.
///< `if`, `for`, `while` etc.
CTP_ForEachStmt, ///< "expression/sequence" associated with 'for-in' loop
///< is expected to conform to 'Sequence' protocol.

CTP_CannotFail, ///< Conversion can never fail. abort() if it does.
};
Expand Down
2 changes: 1 addition & 1 deletion test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct A : P2 {
func wonka() {}
}
let a = A()
for j in i.wibble(a, a) { // expected-error {{type 'A' does not conform to protocol 'Sequence'}} expected-error{{variable 'j' is not bound by any pattern}}
for j in i.wibble(a, a) { // expected-error {{for-in loop requires 'A' to conform to 'Sequence'}} expected-error{{variable 'j' is not bound by any pattern}}
}

// Generic as part of function/tuple types
Expand Down
2 changes: 1 addition & 1 deletion test/Constraints/generic_protocol_witness.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,5 @@ func usesAGenericMethod<U : NeedsAGenericMethod>(_ x: U) {
struct L<T>: Sequence {} // expected-error {{type 'L<T>' does not conform to protocol 'Sequence'}}

func z(_ x: L<Int>) {
for xx in x {} // expected-error {{variable 'xx' is not bound by any pattern}}
for xx in x {} // expected-warning {{immutable value 'xx' was never used; consider replacing with '_' or removing it}}
}
2 changes: 1 addition & 1 deletion test/Constraints/members.swift
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ struct S22490787 {
func f22490787() {
var path: S22490787 = S22490787()

for p in path { // expected-error {{type 'S22490787' does not conform to protocol 'Sequence'}} expected-error{{variable 'p' is not bound by any pattern}}
for p in path { // expected-error {{for-in loop requires 'S22490787' to conform to 'Sequence'}} expected-error{{variable 'p' is not bound by any pattern}}
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/Generics/deduction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ class DeducePropertyParams {
// SR-69
struct A {}
func foo() {
for i in min(1,2) { // expected-error{{type 'Int' does not conform to protocol 'Sequence'}} expected-error {{variable 'i' is not bound by any pattern}}
for i in min(1,2) { // expected-error{{for-in loop requires 'Int' to conform to 'Sequence'}} expected-error {{variable 'i' is not bound by any pattern}}
}
let j = min(Int(3), Float(2.5)) // expected-error{{cannot convert value of type 'Float' to expected argument type 'Int'}}
let k = min(A(), A()) // expected-error{{global function 'min' requires that 'A' conform to 'Comparable'}}
Expand Down
2 changes: 1 addition & 1 deletion test/Sema/editor_placeholders.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ f(<#T#> + 1) // expected-error{{editor placeholder in source file}}
f(<#T##Int#>) // expected-error{{editor placeholder in source file}}
f(<#T##String#>) // expected-error{{editor placeholder in source file}} expected-error{{cannot convert value of type 'String' to expected argument type 'Int'}}

for x in <#T#> { // expected-error{{editor placeholder in source file}} expected-error{{type '()' does not conform to protocol 'Sequence'}}
for x in <#T#> { // expected-error{{editor placeholder in source file}} expected-error{{for-in loop requires '()' to conform to 'Sequence'}}
// expected-error@-1{{variable 'x' is not bound by any pattern}}
}
10 changes: 8 additions & 2 deletions test/SourceKit/Sema/placeholders.swift.placeholders.response
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@
key.column: 19,
key.filepath: placeholders.swift,
key.severity: source.diagnostic.severity.error,
key.description: "type '()' does not conform to protocol 'Sequence'",
key.diagnostic_stage: source.diagnostic.stage.swift.sema
key.description: "for-in loop requires '()' to conform to 'Sequence'",
key.diagnostic_stage: source.diagnostic.stage.swift.sema,
key.ranges: [
{
key.offset: 75,
key.length: 9
}
]
}
]
4 changes: 2 additions & 2 deletions test/attr/attr_dynamic_member_lookup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,8 @@ func keypath_with_subscripts(_ arr: SubscriptLens<[Int]>,

func keypath_with_incorrect_return_type(_ arr: Lens<Array<Int>>) {
for idx in 0..<arr.count {
// expected-error@-1 {{operator function '..<' requires that 'Lens<Int>' conform to 'Strideable'}}
// expected-error@-2 {{operator function '..<' requires that 'Lens<Int>.Stride' conform to 'SignedInteger'}}
// expected-error@-1 {{protocol 'Sequence' requires that 'Lens<Int>' conform to 'Strideable'}}
// expected-error@-2 {{protocol 'Sequence' requires that 'Lens<Int>.Stride' conform to 'SignedInteger'}}
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 technically not accurate, the requirement is not coming from the Sequence protocol itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diagnostics here aren't great because we still don't have a full coverage, but that's something on my radar.

// expected-error@-3 {{cannot convert value of type 'Int' to expected argument type 'Lens<Int>'}}
// expected-error@-4 {{referencing operator function '..<' on 'Comparable' requires that 'Lens<Int>' conform to 'Comparable'}}
// expected-error@-5 {{variable 'idx' is not bound by any pattern}}
Expand Down
14 changes: 6 additions & 8 deletions test/stmt/foreach.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ struct BadContainer1 {
}

func bad_containers_1(bc: BadContainer1) {
for e in bc { } // expected-error{{type 'BadContainer1' does not conform to protocol 'Sequence'}}
for e in bc { } // expected-error{{for-in loop requires 'BadContainer1' to conform to 'Sequence'}}
// expected-error@-1{{variable 'e' is not bound by any pattern}}
}

Expand All @@ -15,7 +15,7 @@ struct BadContainer2 : Sequence { // expected-error{{type 'BadContainer2' does n

func bad_containers_2(bc: BadContainer2) {
for e in bc { }
// expected-error@-1{{variable 'e' is not bound by any pattern}}
// expected-warning@-1 {{immutable value 'e' was never used; consider replacing with '_' or removing it}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this and a few others below suddenly become valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because BadContainer2 is declared to be conforming to Sequence, although it's invalid conformance constraint succeeds. It used to fail on the dependent member type associated with sequence element but I made it so that is handled as a hole.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everywhere variable 'e' is not bound by any pattern exists on a non-recursive decl is a sign that we are either not invalidating the AST properly, or we aren't constructing constraint systems with sufficient information to invalidate an associated pattern binding specifically. It's good to see movement here.

}

struct BadContainer3 : Sequence { // expected-error{{type 'BadContainer3' does not conform to protocol 'Sequence'}}
Expand All @@ -24,7 +24,7 @@ struct BadContainer3 : Sequence { // expected-error{{type 'BadContainer3' does n

func bad_containers_3(bc: BadContainer3) {
for e in bc { }
// expected-error@-1{{variable 'e' is not bound by any pattern}}
// expected-warning@-1 {{immutable value 'e' was never used; consider replacing with '_' or removing it}}
}

struct BadIterator1 {}
Expand All @@ -36,7 +36,7 @@ struct BadContainer4 : Sequence { // expected-error{{type 'BadContainer4' does n

func bad_containers_4(bc: BadContainer4) {
for e in bc { }
// expected-error@-1{{variable 'e' is not bound by any pattern}}
// expected-warning@-1 {{immutable value 'e' was never used; consider replacing with '_' or removing it}}
}

// Pattern type-checking
Expand Down Expand Up @@ -176,10 +176,8 @@ func testMatchingPatterns() {
// <rdar://problem/21662365> QoI: diagnostic for for-each over an optional sequence isn't great
func testOptionalSequence() {
let array : [Int]?
for x in array { // expected-error {{value of optional type '[Int]?' must be unwrapped}}
// expected-note@-1{{coalesce}}
// expected-note@-2{{force-unwrap}}
// expected-error@-3{{variable 'x' is not bound by any pattern}}
for x in array { // expected-error {{for-in loop requires '[Int]?' to conform to 'Sequence'; did you mean to unwrap optional?}}
// expected-error@-1{{variable 'x' is not bound by any pattern}}
}
}

Expand Down