From 8a7a1d98f5f97419cf1848917c1838ffb0e68e72 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Tue, 21 Jul 2020 12:25:43 -0700 Subject: [PATCH] Teach the VarDeclUsageChecker About Variables Bound in Patterns The VDUC was missing a class of AST nodes that can bind variables: patterns in switch statements. For these, it was falling back to requesting a simple replacement of the bound variable name with _. But for patterns, this means there's a two-step dance the user has to go through where the first fixit does this: .pattern(let x) -> .pattern(let _) Then a second round of compilation would emit a fixit to do this: .pattern(let _) -> .pattern(_) Instead, detect "simple" variable bindings - for now, variable patterns that are immediately preceded by a `let` or `var` binding pattern - and collapse two steps into one. Resolves rdar://47240768 --- lib/Sema/MiscDiagnostics.cpp | 29 ++++++++++++++++-- test/Constraints/patterns.swift | 2 +- test/Parse/switch.swift | 2 +- test/decl/var/usage.swift | 52 +++++++++++++++++++++++++++++++-- 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 32a9300dfa77e..2592c6069ae1e 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -2743,7 +2743,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { // let x = foo() // -> // _ = foo() - if (auto *pbd = var->getParentPatternBinding()) + if (auto *pbd = var->getParentPatternBinding()) { if (pbd->getSingleVar() == var && pbd->getInit(0) != nullptr && !isa(pbd->getPattern(0))) { unsigned varKind = var->isLet(); @@ -2755,7 +2755,8 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { .fixItReplace(replaceRange, "_"); continue; } - + } + // If the variable is defined in a pattern in an if/while/guard statement, // see if we can produce a tuned fixit. When we have something like: // @@ -2808,6 +2809,30 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { } } } + + // If the variable is defined in a pattern that isn't one of the usual + // conditional statements, try to detect and rewrite "simple" binding + // patterns: + // case .pattern(let x): + // -> + // case .pattern(_): + if (auto *pattern = var->getParentPattern()) { + BindingPattern *foundVP = nullptr; + pattern->forEachNode([&](Pattern *P) { + if (auto *VP = dyn_cast(P)) + if (VP->getSingleVar() == var) + foundVP = VP; + }); + + if (foundVP) { + unsigned varKind = var->isLet(); + Diags + .diagnose(var->getLoc(), diag::variable_never_used, + var->getName(), varKind) + .fixItReplace(foundVP->getSourceRange(), "_"); + continue; + } + } // Otherwise, this is something more complex, perhaps // let (a,b) = foo() diff --git a/test/Constraints/patterns.swift b/test/Constraints/patterns.swift index 51de1f54ddfd4..3c44d72073ed2 100644 --- a/test/Constraints/patterns.swift +++ b/test/Constraints/patterns.swift @@ -131,7 +131,7 @@ default: // Introduce new x? pattern switch Optional(42) { -case let x?: break // expected-warning{{immutable value 'x' was never used; consider replacing with '_' or removing it}} +case let x?: break // expected-warning{{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{10-11=_}} case nil: break } diff --git a/test/Parse/switch.swift b/test/Parse/switch.swift index 2b42e837965c8..d44fcacd457cb 100644 --- a/test/Parse/switch.swift +++ b/test/Parse/switch.swift @@ -28,7 +28,7 @@ func parseError4(x: Int) { func parseError5(x: Int) { switch x { - case let z // expected-error {{expected ':' after 'case'}} expected-warning {{immutable value 'z' was never used}} {{12-13=_}} + case let z // expected-error {{expected ':' after 'case'}} expected-warning {{immutable value 'z' was never used}} {{8-13=_}} } } diff --git a/test/decl/var/usage.swift b/test/decl/var/usage.swift index 38ad16b2a84d1..76d145b314e56 100644 --- a/test/decl/var/usage.swift +++ b/test/decl/var/usage.swift @@ -304,9 +304,9 @@ func testFixitsInStatementsWithPatterns(_ a : Int?) { // QoI: "never used" in an "if let" should rewrite expression to use != nil func test(_ a : Int?, b : Any) { - if true == true, let x = a { // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{24-25=_}} + if true == true, let x = a { // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{20-25=_}} } - if let x = a, let y = a { // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{10-11=_}} + if let x = a, let y = a { // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{6-11=_}} _ = y } @@ -447,3 +447,51 @@ func testForwardReferenceCapture() { innerFunc() print(x) } + +// rdar://47240768 Expand the definition of "simple" pattern to variables bound in patterns +func testVariablesBoundInPatterns() { + enum StringB { + case simple(b: Bool) + case tuple(b: (Bool, Bool)) + case optional(b: Bool?) + } + + // Because Swift enables all kinds of creative binding forms, make sure that + // variable patterns occuring directly under a `let` or `var` have that + // introducer stripped by the fixit. All other cases are currently too + // complicated for the VarDeclUsageChecker. + switch StringB.simple(b: true) { + case .simple(b: let b) where false: // expected-warning {{immutable value 'b' was never used; consider replacing with '_' or removing it}} {{19-24=_}} + break + case .simple(b: var b) where false: // expected-warning {{variable 'b' was never used; consider replacing with '_' or removing it}} {{19-24=_}} + break + case var .simple(b: b): // expected-warning {{variable 'b' was never used; consider replacing with '_' or removing it}} {{23-24=_}} + break + case .tuple(b: let (b1, b2)) where false: + // expected-warning@-1 {{immutable value 'b1' was never used; consider replacing with '_' or removing it}} {{23-25=_}} + // expected-warning@-2 {{immutable value 'b2' was never used; consider replacing with '_' or removing it}} {{27-29=_}} + break + case .tuple(b: (let b1, let b2)) where false: + // expected-warning@-1 {{immutable value 'b1' was never used; consider replacing with '_' or removing it}} {{19-25=_}} + // expected-warning@-2 {{immutable value 'b2' was never used; consider replacing with '_' or removing it}} {{27-33=_}} + break + case .tuple(b: (var b1, var b2)) where false: + // expected-warning@-1 {{variable 'b1' was never used; consider replacing with '_' or removing it}} {{19-25=_}} + // expected-warning@-2 {{variable 'b2' was never used; consider replacing with '_' or removing it}} {{27-33=_}} + break + case var .tuple(b: (b1, b2)) where false: + // expected-warning@-1 {{variable 'b1' was never used; consider replacing with '_' or removing it}} {{23-25=_}} + // expected-warning@-2 {{variable 'b2' was never used; consider replacing with '_' or removing it}} {{27-29=_}} + break + case .tuple(b: let b): // expected-warning {{immutable value 'b' was never used; consider replacing with '_' or removing it}} {{18-23=_}} + break + case .optional(b: let x?) where false: // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{25-26=_}} + break + case .optional(b: let .some(x)) where false: // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{31-32=_}} + break + case let .optional(b: x?): // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{25-26=_}} + break + case let .optional(b: .none): // expected-warning {{'let' pattern has no effect; sub-pattern didn't bind any variables}} {{8-12=}} + break + } +}