Skip to content

[Diagnostics] Improve diagnostics for implicit (un)tupling. #28076

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
Feb 14, 2020
Merged

[Diagnostics] Improve diagnostics for implicit (un)tupling. #28076

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
20 changes: 8 additions & 12 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if I've paged something out, but why is this the only one of the three diagnostics which states that doing this is deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the other two cases, we most likely have a fix-it that either adds or removes parens (for example, you can look at the test cases), so I felt that providing an additional "this is deprecated" was not so useful. I can add that though, it is not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brentdax what do you think of my response? Ok to not have the deprecated note because we are supplying fix-its? Or should I still add it?

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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