[QoI] make several improvements to the unused expression diagnostics,… (#2559)

* [QoI] make several improvements to the unused expression diagnostics, to go
along with recent policy changes:

- For expression types that are not specifically handled, make sure to
  produce a general "unused value" warning, catching a bunch of unused
  values in the testsuite.

- For unused operator results, diagnose them as uses of the operator
  instead of "calls".

- For calls, mutter the type of the result for greater specificity.

- For initializers, mutter the type of the initialized value.

- Look through OpenExistentialExpr's so we can handle protocol member
  references propertly.

- Look through several other expressions so we handle @discardableResult
  better.

* Fix validation-test/Sema/OverridesAndOverloads.swift after compiler started produced more warnings

Author: tkremenek

include/swift/AST/DiagnosticsSema.def

@@ -2192,10 +2192,16 @@ ERROR(expression_unused_function,none,
       "expression resolves to an unused function", ())
 ERROR(expression_unused_lvalue,none,
       "expression resolves to an unused l-value", ())
-WARNING(expression_unused_result,none,
+WARNING(expression_unused_result_call,none,
         "result of call to %0 is unused", (DeclName))
+WARNING(expression_unused_result_operator,none,
+        "result of operator %0 is unused", (DeclName))
+WARNING(expression_unused_result_unknown, none,
+        "result of call is unused, but produces %0", (Type))
+WARNING(expression_unused_result, none,
+        "expression of type %0 is unused", (Type))
 WARNING(expression_unused_init_result,none,
-        "result of initializer is unused", ())
+        "result of %0 initializer is unused", (Type))
 WARNING(expression_unused_optional_try,none,
         "result of 'try?' is unused", ())
 WARNING(expression_unused_selector_result, none,

lib/Sema/TypeCheckStmt.cpp

@@ -1015,6 +1015,17 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {
     return;
   }
 
+  // Drill through noop expressions we don't care about, like ParanExprs.
+  auto valueE = E;
+  while (1) {
+    valueE = valueE->getValueProvidingExpr();
+    
+    if (auto *OEE = dyn_cast<OpenExistentialExpr>(valueE))
+      valueE = OEE->getSubExpr();
+    else
+      break;
+  }
+  
   // Complain about functions that aren't called.
   // TODO: What about tuples which contain functions by-value that are
   // dead?
@@ -1024,7 +1035,11 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {
     return;
   }
 
-  auto valueE = E->getValueProvidingExpr();
+  // If the result of this expression is of type "()", then it is safe to
+  // ignore.
+  if (valueE->getType()->isVoid() ||
+      valueE->getType()->is<ErrorType>())
+    return;
 
   // Complain about '#selector'.
   if (auto *ObjCSE = dyn_cast<ObjCSelectorExpr>(valueE)) {
@@ -1047,15 +1062,20 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {
     if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(OEE->getSubExpr()))
       return checkIgnoredExpr(IIO->getSubExpr());
 
-  // FIXME: Complain about literals
-
   // Check if we have a call to a function not marked with
   // '@discardableResult'.
   if (auto call = dyn_cast<ApplyExpr>(valueE)) {
     // Dig through all levels of calls.
-    Expr *fn = call->getFn()->getSemanticsProvidingExpr();
-    while (auto applyFn = dyn_cast<ApplyExpr>(fn)) {
-      fn = applyFn->getFn()->getSemanticsProvidingExpr();
+    Expr *fn = call->getFn();
+    while (true) {
+      fn = fn->getSemanticsProvidingExpr();
+      if (auto applyFn = dyn_cast<ApplyExpr>(fn)) {
+        fn = applyFn->getFn();
+      } else if (auto FVE = dyn_cast<ForceValueExpr>(fn)) {
+        fn = FVE->getSubExpr();
+      } else {
+        break;
+      }
     }
 
     // Find the callee.
@@ -1069,19 +1089,45 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {
     else if (auto dynMemberRef = dyn_cast<DynamicMemberRefExpr>(fn))
       callee = dyn_cast<AbstractFunctionDecl>(
                  dynMemberRef->getMember().getDecl());
-
-    if (callee) {
-      if (!callee->getAttrs().getAttribute<DiscardableResultAttr>() &&
-          !valueE->getType()->isVoid()) {
-        diagnose(fn->getLoc(), diag::expression_unused_result,
-                 callee->getFullName());
-        return;
-      }
-      if (isa<ConstructorDecl>(callee) && !call->isImplicit()) {
-        diagnose(fn->getLoc(), diag::expression_unused_init_result);
-      }
+    
+    // If the callee explicitly allows its result to be ignored, then don't
+    // complain.
+    if (callee && callee->getAttrs().getAttribute<DiscardableResultAttr>())
+      return;
+
+    // Otherwise, complain.  Start with more specific diagnostics.
+    if (callee && isa<ConstructorDecl>(callee) && !call->isImplicit()) {
+      diagnose(fn->getLoc(), diag::expression_unused_init_result,
+               callee->getDeclContext()->getDeclaredTypeOfContext())
+        .highlight(call->getArg()->getSourceRange());
+      return;
     }
+    
+    SourceRange SR1 = call->getArg()->getSourceRange(), SR2;
+    if (auto *BO = dyn_cast<BinaryExpr>(call)) {
+      SR1 = BO->getArg()->getElement(0)->getSourceRange();
+      SR2 = BO->getArg()->getElement(1)->getSourceRange();
+    }
+    
+    // Otherwise, produce a generic diagnostic.
+    if (callee) {
+      auto diagID = diag::expression_unused_result_call;
+      if (callee->getFullName().isOperator())
+        diagID = diag::expression_unused_result_operator;
+      
+      diagnose(fn->getLoc(), diagID, callee->getFullName())
+        .highlight(SR1).highlight(SR2);
+    } else
+      diagnose(fn->getLoc(), diag::expression_unused_result_unknown,
+               valueE->getType())
+        .highlight(SR1).highlight(SR2);
+
+    return;
   }
+
+  // Produce a generic diagnostic.
+  diagnose(valueE->getLoc(), diag::expression_unused_result, valueE->getType())
+    .highlight(valueE->getSourceRange());
 }
 
 Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {

test/ClangModules/AppKit_test.swift

@@ -15,8 +15,8 @@ class MyDocument : NSDocument {
 }
 
 func test(_ URL: NSURL, controller: NSDocumentController) {
-  try! NSDocument(contentsOf: URL, ofType: "") // expected-warning{{unused}}
-  try! MyDocument(contentsOf: URL, ofType: "")
+  try! NSDocument(contentsOf: URL, ofType: "") // expected-warning{{result of 'NSDocument' initializer is unused}}
+  try! MyDocument(contentsOf: URL, ofType: "") // expected-warning {{expression of type 'MyDocument' is unused}}
 
   try! controller.makeDocument(withContentsOf: URL, ofType: "")
 }

test/ClangModules/adapter.swift

@@ -15,7 +15,7 @@ import Redeclaration
 let encoding: UInt = NSUTF8StringEncoding
 
 let viaTypedef: Redeclaration.NSPoint = AppKit.NSPoint(x: 0, y: 0)
-Redeclaration.NSStringToNSString(AppKit.NSStringToNSString("abc"))
+Redeclaration.NSStringToNSString(AppKit.NSStringToNSString("abc")) // expected-warning {{result of call is unused}}
 
 let viaStruct: Redeclaration.FooStruct1 = AppKit.FooStruct1()
 let forwardDecl: Redeclaration.Tribool = AppKit.Tribool() // expected-error {{no type named 'Tribool' in module 'Redeclaration'}}

test/ClangModules/availability.swift

@@ -14,6 +14,7 @@ func test_unavailable_instance_method(_ x : NSObject) -> Bool {
 }
 
 func test_unavailable_method_in_protocol(_ x : NSObjectProtocol) {
+  // expected-warning @+1 {{expression of type 'NSObjectProtocol' is unused}}
   x.retain() // expected-error {{'retain()' is unavailable}}
 }
 func test_unavailable_method_in_protocol_use_class_instance(_ x : NSObject) {
@@ -34,8 +35,8 @@ func test_NSInvocation(_ x: NSInvocation,         // expected-error {{'NSInvocat
 
 func test_class_avail(_ x:NSObject, obj: AnyObject) {
   x.`class`() // expected-error {{'class()' is unavailable in Swift: use 'dynamicType' instead}} expected-warning {{result of call to 'class()' is unused}}
-  NSObject.`class`() // expected-error {{'class()' is unavailable in Swift: use 'self' instead}} expected-warning {{result of call to 'class()' is unused}}
-  obj.`class`!() // expected-error {{'class()' is unavailable in Swift: use 'dynamicType' instead}}
+  _ = NSObject.`class`() // expected-error {{'class()' is unavailable in Swift: use 'self' instead}}
+  _ = obj.`class`!() // expected-error {{'class()' is unavailable in Swift: use 'dynamicType' instead}}
 }
 
 func test_unavailable_app_extension() {
@@ -100,7 +101,7 @@ func test_DistributedObjects(_ o: NSObject,
 
   let ca = NSConnectionDidDieNotification // expected-error {{'NSConnectionDidDieNotification' is unavailable in Swift: Use NSXPCConnection instead}}
   let cc = NSConnectionReplyMode // expected-error {{'NSConnectionReplyMode' is unavailable in Swift: Use NSXPCConnection instead}}
-  o.classForPortCoder // expected-error {{'classForPortCoder' is unavailable in Swift: Use NSXPCConnection instead}}
+  _ = o.classForPortCoder // expected-error {{'classForPortCoder' is unavailable in Swift: Use NSXPCConnection instead}}
 }
 
 func test_NSCalendarDate(_ o: NSCalendarDate) {} // expected-error {{'NSCalendarDate' is unavailable in Swift: Use NSCalendar and NSDateComponents and NSDateFormatter instead}}

test/ClangModules/foreign_errors.swift

@@ -104,7 +104,7 @@ func testNSErrorExhaustive() {
     do {
       try ErrorProne.fail()
     } catch let e as NSError {
-      e
+      e // expected-warning {{expression of type 'NSError' is unused}}
     }
   }
 }

test/ClangModules/objc_parse.swift

@@ -49,7 +49,7 @@ func instanceMethods(_ b: B) {
   b.performAdd(1, 2, 3, 4) // expected-error{{missing argument labels 'withValue:withValue:withValue2:' in call}} {{19-19=withValue: }} {{22-22=withValue: }} {{25-25=withValue2: }}
 
   // Both class and instance methods exist.
-  b.description
+  _ = b.description
   b.instanceTakesObjectClassTakesFloat(b)
   b.instanceTakesObjectClassTakesFloat(2.0) // expected-error{{cannot convert value of type 'Double' to expected argument type 'AnyObject!'}}
 
@@ -86,21 +86,21 @@ func instanceMethodsInExtensions(_ b: B) {
   b.method(1, separateExtMethod:3.5)
 
   let m1 = b.method(_:onCat1:)
-  m1(1, onCat1: 2.5)
+  _ = m1(1, onCat1: 2.5)
 
   let m2 = b.method(_:onExtA:)
-  m2(1, onExtA: 2.5)
+  _ = m2(1, onExtA: 2.5)
 
   let m3 = b.method(_:onExtB:)
-  m3(1, onExtB: 2.5)
+  _ = m3(1, onExtB: 2.5)
 
   let m4 = b.method(_:separateExtMethod:)
-  m4(1, separateExtMethod: 2.5)
+  _ = m4(1, separateExtMethod: 2.5)
 }
 
 func dynamicLookupMethod(_ b: AnyObject) {
   if let m5 = b.method(_:separateExtMethod:) {
-    m5(1, separateExtMethod: 2.5)
+    _ = m5(1, separateExtMethod: 2.5)
   }
 }
 
@@ -204,15 +204,16 @@ func testProtocolMethods(_ b: B, p2m: P2.Type) {
   // Imported constructor.
   var b2 = B(viaP2: 3.14159, second: 3.14159)
 
-  p2m.init(viaP2:3.14159, second: 3.14159)
+  _ = p2m.init(viaP2:3.14159, second: 3.14159)
 }
 
 func testId(_ x: AnyObject) {
   x.perform!("foo:", with: x) // expected-warning{{no method declared with Objective-C selector 'foo:'}}
+  // expected-warning @-1 {{result of call is unused, but produces 'Unmanaged<AnyObject>!'}}
 
-  x.performAdd(1, withValue: 2, withValue: 3, withValue2: 4)
-  x.performAdd!(1, withValue: 2, withValue: 3, withValue2: 4)
-  x.performAdd?(1, withValue: 2, withValue: 3, withValue2: 4)
+  _ = x.performAdd(1, withValue: 2, withValue: 3, withValue2: 4)
+  _ = x.performAdd!(1, withValue: 2, withValue: 3, withValue2: 4)
+  _ = x.performAdd?(1, withValue: 2, withValue: 3, withValue2: 4)
 }
 
 class MySubclass : B {
@@ -224,7 +225,7 @@ class MySubclass : B {
 }
 
 func getDescription(_ array: NSArray) {
-  array.description
+  _ = array.description
 }
 
 // Method overriding with unfortunate ordering.
@@ -257,7 +258,7 @@ func almostSubscriptableKeyMismatchInherited(_ roc: ReadOnlyCollectionChild,
 
 // Use of 'Class' via dynamic lookup.
 func classAnyObject(_ obj: NSObject) {
-  obj.myClass().description!()
+  _ = obj.myClass().description!()
 }
 
 // Protocol conformances
@@ -307,8 +308,8 @@ func customAccessors(_ hive: Hive, bee: Bee) {
   markUsed(hive.makingHoney()) // expected-error{{cannot call value of non-function type 'Bool'}}
   hive.setMakingHoney(true) // expected-error{{value of type 'Hive' has no member 'setMakingHoney'}}
 
-  hive.`guard`.description // okay
-  hive.`guard`.description! // no-warning
+  _ = hive.`guard`.description // okay
+  _ = hive.`guard`.description! // no-warning
   hive.`guard` = bee // no-warning
 }
 
@@ -339,7 +340,7 @@ func testDynamicSelf(_ queen: Bee, wobbler: NSWobbling) {
 }
 
 func testRepeatedProtocolAdoption(_ w: NSWindow) {
-  w.description
+  _ = w.description
 }
 
 class ProtocolAdopter1 : FooProto {

test/Constraints/construction.swift

@@ -41,7 +41,7 @@ var z : Z = .none
 func acceptZ(_ z: Z) {}
 func acceptString(_ s: String) {}
 
-Point(1, 2)
+Point(1, 2) // expected-warning {{expression of type '(x: Int, y: Int)' is unused}}
 var db : Base = d
 X(i: 1, j: 2) // expected-warning{{unused}}
 Y(1, 2, "hello") // expected-warning{{unused}}
@@ -60,7 +60,7 @@ _ = .none as Optional<Int>
 Optional(.none) // expected-error{{generic parameter 'T' could not be inferred}}
 
 // Interpolation
-"\(hello), \(world) #\(i)!"
+_ = "\(hello), \(world) #\(i)!"
 
 class File {
   init() { 
@@ -86,21 +86,21 @@ extension Foo {
 
 // Downcasting
 var b : Base
-b as! Derived
+_ = b as! Derived
 
 // Construction doesn't permit conversion.
 // NOTE: Int and other integer-literal convertible types
 //  are special cased in the library.
 Int(i) // expected-warning{{unused}}
-i as Int
+_ = i as Int
 Z(z) // expected-error{{cannot invoke initializer for type 'Z' with an argument list of type '(Z)'}}
 // expected-note @-1 {{overloads for 'Z' exist with these partially matching parameter lists: (UnicodeScalar), (String)}}
 
 Z.init(z)  // expected-error {{cannot invoke 'Z.Type.init' with an argument list of type '(Z)'}}
 // expected-note @-1 {{overloads for 'Z.Type.init' exist with these partially matching parameter lists: (UnicodeScalar), (String)}}
 
 
-z as Z
+_ = z as Z
 
 // Construction from inouts.
 struct FooRef { }

test/Constraints/diagnostics.swift

@@ -353,7 +353,7 @@ f7(1)(1.0)       // expected-error {{missing argument label 'b:' in call}}
 f7(1)(b: 1.0)       // expected-error {{cannot convert value of type 'Double' to expected argument type 'Int'}}
 
 let f8 = f7(2)
-f8(b: 1)
+_ = f8(b: 1)
 f8(10)          // expected-error {{missing argument label 'b:' in call}} {{4-4=b: }}
 f8(1.0)         // expected-error {{missing argument label 'b:' in call}}
 f8(b: 1.0)         // expected-error {{cannot convert value of type 'Double' to expected argument type 'Int'}}

test/Constraints/dynamic_lookup.swift

@@ -117,7 +117,7 @@ obj.foo!(5)
 obj.foo!("hello")
 obj.wibble!()
 obj.wobble!() // expected-error{{value of type 'Id' (aka 'AnyObject') has no member 'wobble'}}
-obj.ext1!()
+obj.ext1!()  // expected-warning {{result of call is unused}}
 obj.wonka!()
 
 // Same as above but without the '!'
@@ -126,7 +126,7 @@ obj.foo(5)
 obj.foo("hello")
 obj.wibble()
 obj.wobble() // expected-error{{value of type 'Id' (aka 'AnyObject') has no member 'wobble'}}
-obj.ext1()
+obj.ext1()  // expected-warning {{result of call is unused}}
 obj.wonka()
 
 // Find class methods via dynamic method lookup.
@@ -144,7 +144,7 @@ var ovl1Result = obj.ovl1!()
 ovl1Result = A() // verify that we got an A, not a B
 
 // Same as above but without the '!'
-obj.ovl1()
+obj.ovl1()  // expected-warning {{result of call is unused}}
 
 // Don't allow overload resolution between declarations from different
 // classes.
@@ -163,7 +163,7 @@ var ovl4Result = obj.ovl4!()
 var ovl5Result = obj.ovl5!() // expected-error{{ambiguous use of 'ovl5()'}}
 
 // Same as above but without the '!'
-obj.ovl4()
+obj.ovl4()  // expected-warning {{result of call is unused}}
 
 // Generics