Sema: several improvements on missing switch cases diagnostics. (#8026)

nkcsgexi · web-flow · commit b4cf37bf7dc9 · 2017-03-10T19:32:37.000-08:00
1. Make sure the actions taken by fixits are reflected in diagnostics messages.
2. Issue missing cases diagnostics at the start of the switch statement instead of its end.
3. Use &lt;#code#&gt; instead of &lt;#Code#&gt; in the stub.
diff --git a/include/swift/AST/ASTPrinter.h b/include/swift/AST/ASTPrinter.h
@@ -310,6 +310,9 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, tok keyword);
 /// Get the length of a keyword or punctuator by its kind.
 uint8_t getKeywordLen(tok keyword);
 
+/// Get <#code#>;
+StringRef getCodePlaceholder();
+
 } // namespace swift
 
 #endif // LLVM_SWIFT_AST_ASTPRINTER_H
diff --git a/include/swift/AST/DiagnosticsSIL.def b/include/swift/AST/DiagnosticsSIL.def
@@ -186,8 +186,6 @@ ERROR(missing_never_call,none,
       "%select{function|closure}1 with uninhabited return type %0 is missing "
       "call to another never-returning function on all paths",
       (Type, unsigned))
-ERROR(non_exhaustive_switch,none,
-      "switch must be exhaustive, consider adding a default clause", ())
 ERROR(guard_body_must_not_fallthrough,none,
       "'guard' body may not fall through, consider using a 'return' or 'throw'"
       " to exit the scope", ())
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2670,7 +2670,12 @@ ERROR(trailing_closure_requires_parens,none,
       " context", ())
 
 ERROR(empty_switch_stmt,none,
-"'switch' statement body must have at least one 'case' or 'default' block",())
+      "'switch' statement body must have at least one 'case' or 'default' block;"
+      " do you want to add "
+      "%select{missing cases?|a default case?}0",(bool))
+ERROR(non_exhaustive_switch,none,
+      "switch must be exhaustive, consider adding "
+      "%select{missing cases|a default clause}0", (bool))
 //------------------------------------------------------------------------------
 // Type Check Patterns
 //------------------------------------------------------------------------------
diff --git a/include/swift/AST/DiagnosticsSema.h b/include/swift/AST/DiagnosticsSema.h
@@ -37,8 +37,7 @@ namespace swift {
     extern detail::DiagWithArguments<void Signature>::type ID;
 #include "DiagnosticsSema.def"
   }
-  void diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS,
-                            Diagnostic Id);
+  void diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS);
 }
 
 #endif
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -605,6 +605,8 @@ uint8_t swift::getKeywordLen(tok keyword) {
   }
 }
 
+StringRef swift::getCodePlaceholder() { return "<#code#>"; }
+
 ASTPrinter &operator<<(ASTPrinter &printer, tok keyword) {
   SmallString<16> Buffer;
   llvm::raw_svector_ostream OS(Buffer);
diff --git a/lib/SILOptimizer/Mandatory/DataflowDiagnostics.cpp b/lib/SILOptimizer/Mandatory/DataflowDiagnostics.cpp
@@ -84,8 +84,7 @@ static void diagnoseUnreachable(const SILInstruction *I,
 
     // A non-exhaustive switch would also produce an unreachable instruction.
     if (L.isASTNode<SwitchStmt>()) {
-      diagnoseMissingCases(Context, L.getAsASTNode<SwitchStmt>(),
-                           diag::non_exhaustive_switch);
+      diagnoseMissingCases(Context, L.getAsASTNode<SwitchStmt>());
       return;
     }
 
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -2632,7 +2632,7 @@ printRequirementStub(ValueDecl *Requirement, DeclContext *Adopter,
     Options.PrintDocumentationComments = false;
     Options.AccessibilityFilter = Accessibility::Private;
     Options.PrintAccessibility = false;
-    Options.FunctionBody = [](const ValueDecl *VD) { return "<#code#>"; };
+    Options.FunctionBody = [](const ValueDecl *VD) { return getCodePlaceholder(); };
     Options.setBaseType(AdopterTy);
     Options.CurrentModule = Adopter->getParentModule();
     if (!Adopter->isExtensionContext()) {
diff --git a/lib/Sema/TypeCheckStmt.cpp b/lib/Sema/TypeCheckStmt.cpp
@@ -212,16 +212,19 @@ removedHandledEnumElements(Pattern *P,
   return false;
 };
 
-void swift::diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS,
-                                 Diagnostic Id) {
+void swift::
+diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS) {
+  bool Empty = SwitchS->getCases().empty();
+  SourceLoc StartLoc = SwitchS->getStartLoc();
   SourceLoc EndLoc = SwitchS->getEndLoc();
-  StringRef Placeholder = "<#Code#>";
+  StringRef Placeholder = getCodePlaceholder();
   SmallString<32> Buffer;
   llvm::raw_svector_ostream OS(Buffer);
 
   auto DefaultDiag = [&]() {
     OS << tok::kw_default << ": " << Placeholder << "\n";
-    Context.Diags.diagnose(EndLoc, Id).fixItInsert(EndLoc, Buffer.str());
+    Context.Diags.diagnose(StartLoc, Empty ? diag::empty_switch_stmt :
+      diag::non_exhaustive_switch, true).fixItInsert(EndLoc, Buffer.str());
   };
   // To find the subject enum decl for this switch statement.
   EnumDecl *SubjectED = nullptr;
@@ -307,7 +310,8 @@ void swift::diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS,
       printPayloads(EE, OS);
       OS <<": " << Placeholder << "\n";
   });
-  Context.Diags.diagnose(EndLoc, Id).fixItInsert(EndLoc, Buffer.str());
+  Context.Diags.diagnose(StartLoc, Empty ? diag::empty_switch_stmt :
+    diag::non_exhaustive_switch, false).fixItInsert(EndLoc, Buffer.str());
 }
 
 static void setAutoClosureDiscriminators(DeclContext *DC, Stmt *S) {
@@ -981,7 +985,7 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
 
     // Reject switch statements with empty blocks.
     if (S->getCases().empty())
-      diagnoseMissingCases(TC.Context, S, diag::empty_switch_stmt);
+      diagnoseMissingCases(TC.Context, S);
     for (unsigned i = 0, e = S->getCases().size(); i < e; ++i) {
       auto *caseBlock = S->getCases()[i];
       // Fallthrough transfers control to the next case block. In the
diff --git a/test/ClangImporter/enum-dataflow.swift b/test/ClangImporter/enum-dataflow.swift
@@ -7,12 +7,12 @@ import user_objc
 
 let aliasOriginal = NSAliasesEnum.byName
 
-switch aliasOriginal {
+switch aliasOriginal { // expected-error {{switch must be exhaustive, consider adding missing cases}}
 case .original:
   break
-} // expected-error {{switch must be exhaustive, consider adding a default clause}}
+}
 
-switch aliasOriginal {
+switch aliasOriginal { // expected-error {{switch must be exhaustive, consider adding missing cases}}
 case .bySameValue:
   break
-} // expected-error {{switch must be exhaustive, consider adding a default clause}}
+}
diff --git a/test/ClangImporter/enum-error.swift b/test/ClangImporter/enum-error.swift
@@ -82,7 +82,7 @@ func testError() {
   switch (terr) { case .TENone, .TEOne, .TETwo: break } // ok
 
   switch (terr) { case .TENone, .TEOne: break }
-    // expected-error@-1 {{switch must be exhaustive, consider adding a default clause}}
+    // expected-error@-1 {{switch must be exhaustive, consider adding missing cases}}
 
   let _ = TestError.Code(rawValue: 2)!
 
diff --git a/test/ClangImporter/enum-new.swift b/test/ClangImporter/enum-new.swift
@@ -13,12 +13,12 @@ func test() {
   case .Yellow, .Magenta, .Black, .Cyan: break
   } // no-error
 
-  switch getColorOptions() {
+  switch getColorOptions() { // expected-error {{switch must be exhaustive, consider adding a default clause}}
   case ColorOptions.Pastel: break
   case ColorOptions.Swift: break
-  } // expected-error {{switch must be exhaustive, consider adding a default clause}}
+  }
   
-  switch 5 as Int16 {
+  switch 5 as Int16 { // expected-error {{switch must be exhaustive, consider adding a default clause}}
   case Zero: break // no-error
-  } // expected-error {{switch must be exhaustive, consider adding a default clause}}
+  }
 }
diff --git a/test/FixCode/fixits-empty-switch.swift.result b/test/FixCode/fixits-empty-switch.swift.result
@@ -9,14 +9,14 @@ enum E1 {
 
 func foo1(_ e: E1) {
   switch e {
-  case .e1: <#Code#>
-case .e2: <#Code#>
-case .e3: <#Code#>
+  case .e1: <#code#>
+case .e2: <#code#>
+case .e3: <#code#>
 }
 }
 
 func foo1 (_ i : Int) {
   switch i {
-  default: <#Code#>
+  default: <#code#>
 }
 }
diff --git a/test/FixCode/fixits-switch.swift.result b/test/FixCode/fixits-switch.swift.result
@@ -12,25 +12,25 @@ func foo1(_ e : E1) -> Int {
   switch(e) {
   case .e1:
     return 1
-  case .e2: <#Code#>
-case .e3: <#Code#>
-case .e4: <#Code#>
+  case .e2: <#code#>
+case .e3: <#code#>
+case .e4: <#code#>
 }
 }
 
 func foo2(_ i : Int) -> Int {
   switch i {
   case 1:
     return 1
-  default: <#Code#>
+  default: <#code#>
 }
 }
 
 func foo3(_ c : Character) -> Character {
   switch c {
   case "a":
     return "a"
-  default: <#Code#>
+  default: <#code#>
 }
 }
 
@@ -50,37 +50,37 @@ func foo4(_ e : E2) -> Int {
   switch e {
   case .e2:
     return 1
-  case .e1(let a, let s): <#Code#>
-case .e3(let a): <#Code#>
-case .e4(_): <#Code#>
-case .e5(_, _): <#Code#>
-case .e6(let a, _): <#Code#>
-case .e7: <#Code#>
-case .e8(let a, _, _): <#Code#>
-case .e9(_, _, _): <#Code#>
+  case .e1(let a, let s): <#code#>
+case .e3(let a): <#code#>
+case .e4(_): <#code#>
+case .e5(_, _): <#code#>
+case .e6(let a, _): <#code#>
+case .e7: <#code#>
+case .e8(let a, _, _): <#code#>
+case .e9(_, _, _): <#code#>
 }
 }
 
 func foo5(_ e : E1) -> Int {
   switch e {
   case _ where e.rawValue > 0:
     return 1
-  default: <#Code#>
+  default: <#code#>
 }
 }
 
 func foo6(_ e : E2) -> Int {
   switch e {
   case let .e1(x, y):
     return x + y
-  case .e2(let a): <#Code#>
-case .e3(let a): <#Code#>
-case .e4(_): <#Code#>
-case .e5(_, _): <#Code#>
-case .e6(let a, _): <#Code#>
-case .e7: <#Code#>
-case .e8(let a, _, _): <#Code#>
-case .e9(_, _, _): <#Code#>
+  case .e2(let a): <#code#>
+case .e3(let a): <#code#>
+case .e4(_): <#code#>
+case .e5(_, _): <#code#>
+case .e6(let a, _): <#code#>
+case .e7: <#code#>
+case .e8(let a, _, _): <#code#>
+case .e9(_, _, _): <#code#>
 }
 }
 
@@ -89,11 +89,11 @@ func foo7(_ e : E2) -> Int {
   case .e2(1): return 0
   case .e1: return 0
   case .e3: return 0
-  case .e4(_): <#Code#>
-case .e5(_, _): <#Code#>
-case .e6(let a, _): <#Code#>
-case .e7: <#Code#>
-case .e8(let a, _, _): <#Code#>
-case .e9(_, _, _): <#Code#>
+  case .e4(_): <#code#>
+case .e5(_, _): <#code#>
+case .e6(let a, _): <#code#>
+case .e7: <#code#>
+case .e8(let a, _, _): <#code#>
+case .e9(_, _, _): <#code#>
 }
 }
diff --git a/test/Parse/implicit_getter_incomplete.swift b/test/Parse/implicit_getter_incomplete.swift
@@ -13,6 +13,6 @@ func test1() {
 // Would trigger assertion when AST verifier checks source ranges ("child source range not contained within its parent")
 func test2() { // expected-note {{match}}
   var a : Int { // expected-note {{match}}
-    switch i { // expected-error {{unresolved identifier}}
-} // expected-error {{'switch' statement body must have at least one}}
+    switch i { // expected-error {{unresolved identifier}} expected-error{{'switch' statement body must have at least one 'case'}}
+}
 // expected-error@+1 2 {{expected '}'}}
diff --git a/test/Parse/invalid.swift b/test/Parse/invalid.swift
@@ -51,7 +51,7 @@ func testNotCoveredCase(x: Int) {
     break
   }
 
-  switch x {
+  switch x { // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
 #if true // expected-error {{all statements inside a switch must be covered by a 'case' or 'default'}}
   case 1:
     break
@@ -60,7 +60,7 @@ func testNotCoveredCase(x: Int) {
   default:
     break
 #endif
-  } // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
+  }
 }
 
 // rdar://18926814
diff --git a/test/Parse/recovery.swift b/test/Parse/recovery.swift
@@ -203,12 +203,12 @@ func missingControllingExprInForEach() {
 func missingControllingExprInSwitch() {
   switch // expected-error {{expected expression in 'switch' statement}} expected-error {{expected '{' after 'switch' subject expression}}
 
-  switch { // expected-error {{expected expression in 'switch' statement}}
-  } // expected-error {{'switch' statement body must have at least one 'case' or 'default' block}}
+  switch { // expected-error {{expected expression in 'switch' statement}} expected-error {{'switch' statement body must have at least one 'case' or 'default' block}}
+  }
 
-  switch // expected-error {{expected expression in 'switch' statement}}
+  switch // expected-error {{expected expression in 'switch' statement}} expected-error {{'switch' statement body must have at least one 'case' or 'default' block}}
   {
-  } // expected-error {{'switch' statement body must have at least one 'case' or 'default' block}}
+  }
 
   switch { // expected-error {{expected expression in 'switch' statement}}
     case _: return
diff --git a/test/Parse/switch.swift b/test/Parse/switch.swift
@@ -121,14 +121,14 @@ default: // expected-error{{additional 'case' blocks cannot appear after the 'de
   x = 0
 }
 
-switch x {
+switch x { // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
   x = 1 // expected-error{{all statements inside a switch must be covered by a 'case' or 'default'}}
-} // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
+}
 
-switch x {
+switch x { // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
   x = 1 // expected-error{{all statements inside a switch must be covered by a 'case' or 'default'}}
   x = 2
-} // expected-error{{'switch' statement body must have at least one 'case' or 'default' block}}
+}
 
 switch x {
 default: // expected-error{{'default' label in a 'switch' should have at least one executable statement}} {{9-9= break}}
diff --git a/test/SILOptimizer/switch.swift b/test/SILOptimizer/switch.swift
@@ -2,12 +2,12 @@
 
 func non_fully_covered_switch(x: Int) -> Int {
   var x = x
-  switch x {
+  switch x { // expected-error{{switch must be exhaustive}}
     case 0:
      x += 1
     case 3:
      x -= 1
-  } // expected-error{{switch must be exhaustive}}
+  }
   return x
 }
 
diff --git a/test/SILOptimizer/unreachable_code.swift b/test/SILOptimizer/unreachable_code.swift
diff --git a/test/stmt/switch_stmt1.swift b/test/stmt/switch_stmt1.swift
diff --git a/test/stmt/switch_stmt2.swift b/test/stmt/switch_stmt2.swift
diff --git a/tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp b/tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp
diff --git a/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp b/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp

Original file line number	Diff line number	Diff line change
`@@ -37,8 +37,7 @@ namespace swift {`
`37`	`37`	`extern detail::DiagWithArguments<void Signature>::type ID;`
`38`	`38`	`#include "DiagnosticsSema.def"`
`39`	`39`	`}`
`40`		`- void diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS,`
`41`		`- Diagnostic Id);`
	`40`	`+ void diagnoseMissingCases(ASTContext &Context, const SwitchStmt *SwitchS);`
`42`	`41`	`}`
`43`	`42`
`44`	`43`	`#endif`
Original file line number	Diff line number	Diff line change
`@@ -605,6 +605,8 @@ uint8_t swift::getKeywordLen(tok keyword) {`
`605`	`605`	`}`
`606`	`606`	`}`
`607`	`607`
	`608`	`+StringRef swift::getCodePlaceholder() { return "<#code#>"; }`
	`609`	`+`
`608`	`610`	`ASTPrinter &operator<<(ASTPrinter &printer, tok keyword) {`
`609`	`611`	`SmallString<16> Buffer;`
`610`	`612`	`llvm::raw_svector_ostream OS(Buffer);`
Original file line number	Diff line number	Diff line change
`@@ -84,8 +84,7 @@ static void diagnoseUnreachable(const SILInstruction *I,`
`84`	`84`
`85`	`85`	`// A non-exhaustive switch would also produce an unreachable instruction.`
`86`	`86`	`if (L.isASTNode<SwitchStmt>()) {`
`87`		`- diagnoseMissingCases(Context, L.getAsASTNode<SwitchStmt>(),`
`88`		`- diag::non_exhaustive_switch);`
	`87`	`+ diagnoseMissingCases(Context, L.getAsASTNode<SwitchStmt>());`
`89`	`88`	`return;`
`90`	`89`	`}`
`91`	`90`
Original file line number	Diff line number	Diff line change
`@@ -9,14 +9,14 @@ enum E1 {`
`9`	`9`
`10`	`10`	`func foo1(_ e: E1) {`
`11`	`11`	`switch e {`
`12`		`- case .e1: <#Code#>`
`13`		`-case .e2: <#Code#>`
`14`		`-case .e3: <#Code#>`
	`12`	`+ case .e1: <#code#>`
	`13`	`+case .e2: <#code#>`
	`14`	`+case .e3: <#code#>`
`15`	`15`	`}`
`16`	`16`	`}`
`17`	`17`
`18`	`18`	`func foo1 (_ i : Int) {`
`19`	`19`	`switch i {`
`20`		`- default: <#Code#>`
	`20`	`+ default: <#code#>`
`21`	`21`	`}`
`22`	`22`	`}`