Skip to content

Commit

Permalink
[Diagnostics] Improve diagnostics for implicit (un)tupling.
Browse files Browse the repository at this point in the history 
Fixes rdar://problem/56436226.
  • Loading branch information
@varungandhi-apple
varungandhi-apple committed Feb 13, 2020
1 parent 518509d commit a98d20f
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 48 deletions.
20 changes: 8 additions & 12 deletions include/swift/AST/DiagnosticsSema.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3712,18 +3712,14 @@ ERROR(pattern_type_mismatch_context,none,

ERROR(tuple_pattern_in_non_tuple_context,none,
"tuple pattern cannot match values of the non-tuple type %0", (Type))
WARNING(matching_pattern_with_many_assoc_values, none,
"cannot match several associated values at once, "
"implicitly tupling the associated values and trying to match that "
"instead", ())
WARNING(matching_tuple_pattern_with_many_assoc_values,none,
"a tuple pattern cannot match several associated values at once, "
"implicitly tupling the associated values and trying to match "
"that instead", ())
WARNING(matching_many_patterns_with_tupled_assoc_value,none,
"the enum case has a single tuple as an associated value, but "
"there are several patterns here, implicitly tupling the patterns "
"and trying to match that instead", ())
WARNING(found_one_pattern_for_several_associated_values,none,
"enum case '%0' has %1 associated values; matching them as a tuple "
"is deprecated", (StringRef, unsigned))
WARNING(converting_tuple_into_several_associated_values,none,
"enum case '%0' has %1 associated values", (StringRef, unsigned))
WARNING(converting_several_associated_values_into_tuple,none,
"enum case '%0' has one associated value that is a tuple of %1 "
"elements",(StringRef, unsigned))
ERROR(closure_argument_list_tuple,none,
"contextual closure type %0 expects %1 argument%s1, "
"but %2 %select{were|was}3 used in closure body", (Type, unsigned, unsigned, bool))
Expand Down
86 changes: 63 additions & 23 deletions lib/Sema/TypeCheckPattern.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -826,50 +826,89 @@ llvm::Expected<Type> PatternTypeRequest::evaluate(
llvm_unreachable("bad pattern kind!");
}

namespace {

/// Potentially tuple/untuple a pattern before passing it to the pattern engine.
///
/// We need to allow particular matches for backwards compatibility, so we
/// "repair" the pattern if needed, so that the exhaustiveness checker receives
/// well-formed input. Also emit diagnostics warning the user to fix their code.
/// "repair" the pattern if needed. This ensures that the pattern engine
/// receives well-formed input, avoiding the need to implement an additional
/// compatibility hack there, as doing that is lot more tricky due to the
/// different cases that need to handled.
///
/// We also emit diagnostics and potentially a fix-it to help the user.
///
/// See SR-11160 and SR-11212 for more discussion.
//
// type ~ (T1, ..., Tn) (n >= 2)
// 1a. pat ~ ((P1, ..., Pm)) (m >= 2)
// 1b. pat
// 1a. pat ~ ((P1, ..., Pm)) (m >= 2) -> untuple the pattern
// 1b. pat (a single pattern, not a tuple) -> handled by pattern engine
// type ~ ((T1, ..., Tn)) (n >= 2)
// 2. pat ~ (P1, ..., Pm) (m >= 2)
void implicitlyUntuplePatternIfApplicable(ASTContext &Ctx,
Pattern *&enumElementInnerPat,
Type enumPayloadType) {
// 2. pat ~ (P1, ..., Pm) (m >= 2) -> tuple the pattern
static
void repairTupleOrAssociatedValuePatternIfApplicable(
ASTContext &Ctx,
Pattern *&enumElementInnerPat,
Type enumPayloadType,
const EnumElementDecl *enumCase) {
auto &DE = Ctx.Diags;
bool addDeclNote = false;
if (auto *tupleType = dyn_cast<TupleType>(enumPayloadType.getPointer())) {
if (tupleType->getNumElements() >= 2
&& enumElementInnerPat->getKind() == PatternKind::Paren) {
auto *semantic = enumElementInnerPat->getSemanticsProvidingPattern();
if (auto *tuplePattern = dyn_cast<TuplePattern>(semantic)) {
if (tuplePattern->getNumElements() >= 2) {
DE.diagnose(tuplePattern->getLoc(),
diag::matching_tuple_pattern_with_many_assoc_values);
auto diag = DE.diagnose(tuplePattern->getLoc(),
diag::converting_tuple_into_several_associated_values,
enumCase->getNameStr(), tupleType->getNumElements());
auto subPattern =
dyn_cast<ParenPattern>(enumElementInnerPat)->getSubPattern();

// We might also have code like
//
// enum Upair { case upair(Int, Int) }
// func f(u: Upair) { switch u { case .upair(let (x, y)): () } }
//
// This needs a more complex rearrangement to fix the code. So only
// apply the fix-it if we have a tuple immediately inside.
if (subPattern->getKind() == PatternKind::Tuple) {
auto leadingParen = SourceRange(enumElementInnerPat->getStartLoc());
auto trailingParen = SourceRange(enumElementInnerPat->getEndLoc());
diag.fixItRemove(leadingParen)
.fixItRemove(trailingParen);
}

addDeclNote = true;
enumElementInnerPat = semantic;
}
} else {
DE.diagnose(enumElementInnerPat->getLoc(),
diag::matching_pattern_with_many_assoc_values);
diag::found_one_pattern_for_several_associated_values,
enumCase->getNameStr(),
tupleType->getNumElements());
addDeclNote = true;
}
}
} else if (auto *tupleType = enumPayloadType->getAs<TupleType>()) {
if (tupleType->getNumElements() >= 2
&& enumElementInnerPat->getKind() == PatternKind::Tuple) {
DE.diagnose(enumElementInnerPat->getLoc(),
diag::matching_many_patterns_with_tupled_assoc_value);
enumElementInnerPat =
new (Ctx) ParenPattern(enumElementInnerPat->getStartLoc(),
enumElementInnerPat,
enumElementInnerPat->getEndLoc());
if (tupleType->getNumElements() >= 2) {
if (auto *tuplePattern = dyn_cast<TuplePattern>(enumElementInnerPat)) {
DE.diagnose(enumElementInnerPat->getLoc(),
diag::converting_several_associated_values_into_tuple,
enumCase->getNameStr(),
tupleType->getNumElements())
.fixItInsert(enumElementInnerPat->getStartLoc(), "(")
.fixItInsertAfter(enumElementInnerPat->getEndLoc(), ")");
addDeclNote = true;
enumElementInnerPat =
new (Ctx) ParenPattern(enumElementInnerPat->getStartLoc(),
enumElementInnerPat,
enumElementInnerPat->getEndLoc());
}
}
}
}

if (addDeclNote)
DE.diagnose(enumCase->getStartLoc(), diag::decl_declared_here,
enumCase->getFullName());
}

/// Perform top-down type coercion on the given pattern.
Expand Down Expand Up @@ -1449,7 +1488,8 @@ Pattern *TypeChecker::coercePatternToType(ContextualPattern pattern,
newSubOptions.setContext(TypeResolverContext::EnumPatternPayload);
newSubOptions |= TypeResolutionFlags::FromNonInferredPattern;

::implicitlyUntuplePatternIfApplicable(Context, sub, elementType);
::repairTupleOrAssociatedValuePatternIfApplicable(
Context, sub, elementType, elt);

sub = coercePatternToType(
pattern.forSubPattern(sub, /*retainTopLevel=*/false), elementType,
Expand Down
6 changes: 4 additions & 2 deletions test/Parse/matching_patterns.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ enum Voluntary<T> : Equatable {
()

case .Twain(), // expected-error{{tuple pattern has the wrong length for tuple type '(T, T)'}}
.Twain(_), // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
.Twain(_), // expected-warning {{enum case 'Twain' has 2 associated values; matching them as a tuple is deprecated}}
// expected-note@-25 {{'Twain' declared here}}
.Twain(_, _),
.Twain(_, _, _): // expected-error{{tuple pattern has the wrong length for tuple type '(T, T)'}}
()
Expand Down Expand Up @@ -143,7 +144,8 @@ case Voluntary<Int>.Mere,
.Mere(_):
()
case .Twain,
.Twain(_), // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
.Twain(_), // expected-warning {{enum case 'Twain' has 2 associated values; matching them as a tuple is deprecated}}
// expected-note@-69 {{'Twain' declared here}}
.Twain(_, _),
.Twain(_, _, _): // expected-error{{tuple pattern has the wrong length for tuple type '(Int, Int)'}}
()
Expand Down
30 changes: 20 additions & 10 deletions test/Sema/exhaustive_switch.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1279,21 +1279,24 @@ enum SR11212Tests {
func sr11212_content_untupled_pattern_tupled1(u: Untupled) -> (Int, Int) {
switch u {
case .upair((let x, let y)): return (x, y)
// expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
// expected-warning@-1 {{enum case 'upair' has 2 associated values}}{{16-17=}}{{31-32=}}
// expected-note@-7 {{'upair' declared here}}
}
}

func sr11212_content_untupled_pattern_tupled2(u: Untupled) -> (Int, Int) {
switch u {
case .upair(let (x, y)): return (x, y)
// expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
// expected-warning@-1 {{enum case 'upair' has 2 associated values}} // No fix-it as that would require us to peek inside the 'let' :-/
// expected-note@-15 {{'upair' declared here}}
}
}

func sr11212_content_untupled_pattern_tupled3(u: Untupled) -> (Int, Int) {
switch u {
case let .upair((x, y)): return (x, y)
// expected-warning@-1 {{a tuple pattern cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
// expected-warning@-1 {{enum case 'upair' has 2 associated values}}{{20-21=}}{{27-28=}}
// expected-note@-23 {{'upair' declared here}}
}
}

Expand All @@ -1312,14 +1315,16 @@ enum SR11212Tests {
func sr11212_content_untupled_pattern_ambiguous1(u: Untupled) -> (Int, Int) {
switch u {
case .upair(let u_): return u_
// expected-warning@-1 {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
// expected-warning@-1 {{enum case 'upair' has 2 associated values; matching them as a tuple is deprecated}}
// expected-note@-43 {{'upair' declared here}}
}
}

func sr11212_content_untupled_pattern_ambiguous2(u: Untupled) -> (Int, Int) {
switch u {
case let .upair(u_): return u_
// expected-warning@-1 {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
// expected-warning@-1 {{enum case 'upair' has 2 associated values; matching them as a tuple is deprecated}}
// expected-note@-51 {{'upair' declared here}}
}
}

Expand Down Expand Up @@ -1348,14 +1353,16 @@ enum SR11212Tests {
func sr11212_content_tupled_pattern_untupled1(t: Tupled) -> (Int, Int) {
switch t {
case .tpair(let x, let y): return (x, y)
// expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
// expected-warning@-1 {{enum case 'tpair' has one associated value that is a tuple of 2 elements}}{{16-16=(}}{{30-30=)}}
// expected-note@-25 {{'tpair' declared here}}
}
}

func sr11212_content_tupled_pattern_untupled2(t: Tupled) -> (Int, Int) {
switch t {
case let .tpair(x, y): return (x, y)
// expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
// expected-warning@-1 {{enum case 'tpair' has one associated value that is a tuple of 2 elements}}{{20-20=(}}{{26-26=)}}
// expected-note@-33 {{'tpair' declared here}}
}
}

Expand Down Expand Up @@ -1396,22 +1403,25 @@ enum SR11212Tests {
func sr11212_content_generic_pattern_untupled1(b: Box<(Int, Int)>) -> (Int, Int) {
switch b {
case .box(let x, let y): return (x, y)
// expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
// expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{14-14=(}}{{28-28=)}}
// expected-note@-25 {{'box' declared here}}
}
}

func sr11212_content_generic_pattern_untupled2(b: Box<(Int, Int)>) -> (Int, Int) {
switch b {
case let .box(x, y): return (x, y)
// expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
// expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{18-18=(}}{{24-24=)}}
// expected-note@-33 {{'box' declared here}}
}
}

// rdar://problem/58578342
func sr11212_content_generic_pattern_untupled3(b: Box<((Int, Int), Int)>) -> (Int, Int, Int) {
switch b {
case let .box((x, y), z): return (x, y, z)
// expected-warning@-1 {{the enum case has a single tuple as an associated value, but there are several patterns here, implicitly tupling the patterns and trying to match that instead}}
// expected-warning@-1 {{enum case 'box' has one associated value that is a tuple of 2 elements}}{{18-18=(}}{{29-29=)}}
// expected-note@-42 {{'box' declared here}}
}
}

Expand Down
3 changes: 2 additions & 1 deletion test/decl/enum/enumtest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ func testDirection() {
i = x
break

case .NorthEast(let x): // expected-warning {{cannot match several associated values at once, implicitly tupling the associated values and trying to match that instead}}
case .NorthEast(let x): // expected-warning {{enum case 'NorthEast' has 2 associated values; matching them as a tuple is deprecated}}
// expected-note@-14 {{'NorthEast(distanceNorth:distanceEast:)' declared here}}
i = x.distanceEast
break
}
Expand Down

0 comments on commit a98d20f

Please sign in to comment.