Emit special warning when = is used and user might want to use <- (#1115

)

* Emit special warning when = is used and user might want to use <-

* Don't warn if there is no setter

* Move IsPropertyGetter and IsPropertySetter to tast.fs

* Show property name in error message

* Use IsPropertyGetterMethod/IsPropertySetterMethod in Symbols.fs

* Only warn on last expression in a sequence - fixes #326

* Extract ReportImplicitlyIgnoredBoolExpression as separate function

* Detect "in" expressions

src/fsharp/CompileOps.fs

@@ -142,7 +142,10 @@ let GetRangeOfError(err:PhasedError) =
       | TypeIsImplicitlyAbstract m 
       | RequiredButNotSpecified (_,_,_,_,m) 
       | FunctionValueUnexpected (_,_,m)
-      | UnitTypeExpected (_,_,_,m )
+      | UnitTypeExpected (_,_,m)
+      | UnitTypeExpectedWithEquality (_,_,m)
+      | UnitTypeExpectedWithPossiblePropertySetter (_,_,_,_,m)
+      | UnitTypeExpectedWithPossibleAssignment (_,_,_,_,m)
       | UseOfAddressOfOperator m 
       | DeprecatedThreadStaticBindingWarning(m) 
       | NonUniqueInferredAbstractSlot (_,_,_,_,_,m) 
@@ -266,6 +269,9 @@ let GetErrorNumber(err:PhasedError) =
       | UnionPatternsBindDifferentNames _  -> 18
       | UnionCaseWrongArguments  _ -> 19
       | UnitTypeExpected _  -> 20
+      | UnitTypeExpectedWithEquality _  -> 20
+      | UnitTypeExpectedWithPossiblePropertySetter _  -> 20
+      | UnitTypeExpectedWithPossibleAssignment _  -> 20
       | RecursiveUseCheckedAtRuntime  _ -> 21
       | LetRecEvaluatedOutOfOrder  _ -> 22
       | NameClash _ -> 23
@@ -532,8 +538,11 @@ let UseOfAddressOfOperatorE() = DeclareResourceString("UseOfAddressOfOperator","
 let DefensiveCopyWarningE() = DeclareResourceString("DefensiveCopyWarning","%s")
 let DeprecatedThreadStaticBindingWarningE() = DeclareResourceString("DeprecatedThreadStaticBindingWarning","")
 let FunctionValueUnexpectedE() = DeclareResourceString("FunctionValueUnexpected","%s")
-let UnitTypeExpected1E() = DeclareResourceString("UnitTypeExpected1","")
-let UnitTypeExpected2E() = DeclareResourceString("UnitTypeExpected2","%s")
+let UnitTypeExpectedE() = DeclareResourceString("UnitTypeExpected","")
+let UnitTypeExpectedWithEqualityE() = DeclareResourceString("UnitTypeExpectedWithEquality","")
+let UnitTypeExpectedWithPossiblePropertySetterE() = DeclareResourceString("UnitTypeExpectedWithPossiblePropertySetter","%s%s")
+let UnitTypeExpectedWithPossibleAssignmentE() = DeclareResourceString("UnitTypeExpectedWithPossibleAssignment","%s")
+let UnitTypeExpectedWithPossibleAssignmentToMutableE() = DeclareResourceString("UnitTypeExpectedWithPossibleAssignmentToMutable","%s")
 let RecursiveUseCheckedAtRuntimeE() = DeclareResourceString("RecursiveUseCheckedAtRuntime","")
 let LetRecUnsound1E() = DeclareResourceString("LetRecUnsound1","%s")
 let LetRecUnsound2E() = DeclareResourceString("LetRecUnsound2","%s%s")
@@ -1206,16 +1215,25 @@ let OutputPhasedErrorR (os:System.Text.StringBuilder) (err:PhasedError) =
       | DeprecatedThreadStaticBindingWarning(_) -> 
           os.Append(DeprecatedThreadStaticBindingWarningE().Format) |> ignore
       | FunctionValueUnexpected (denv,ty,_) ->
-          // REVIEW: consider if we need to show _cxs (the type parameter constrants)
+          // REVIEW: consider if we need to show _cxs (the type parameter constraints)
           let _, ty, _cxs = PrettyTypes.PrettifyTypes1 denv.g ty
           os.Append(FunctionValueUnexpectedE().Format (NicePrint.stringOfTy denv ty)) |> ignore
-      | UnitTypeExpected (denv,ty,perhapsProp,_) ->
-          // REVIEW: consider if we need to show _cxs (the type parameter constrants)
-          let _, ty, _cxs = PrettyTypes.PrettifyTypes1 denv.g ty
-          if perhapsProp then 
-            os.Append(UnitTypeExpected2E().Format (NicePrint.stringOfTy denv ty)) |> ignore
-          else
-            os.Append(UnitTypeExpected1E().Format) |> ignore
+      | UnitTypeExpected (_,_,_) ->
+          let warningText = UnitTypeExpectedE().Format
+          os.Append warningText |> ignore
+      | UnitTypeExpectedWithEquality (_) ->
+          let warningText = UnitTypeExpectedWithEqualityE().Format
+          os.Append warningText |> ignore
+      | UnitTypeExpectedWithPossiblePropertySetter (_,_,bindingName,propertyName,_) ->
+          let warningText = UnitTypeExpectedWithPossiblePropertySetterE().Format bindingName propertyName
+          os.Append warningText |> ignore
+      | UnitTypeExpectedWithPossibleAssignment (_,_,isAlreadyMutable,bindingName,_) ->
+          let warningText = 
+            if isAlreadyMutable then
+                UnitTypeExpectedWithPossibleAssignmentToMutableE().Format bindingName
+            else
+                UnitTypeExpectedWithPossibleAssignmentE().Format bindingName
+          os.Append warningText |> ignore
       | RecursiveUseCheckedAtRuntime  _ -> 
           os.Append(RecursiveUseCheckedAtRuntimeE().Format) |> ignore
       | LetRecUnsound (_,[v],_) ->  

src/fsharp/FSStrings.resx

@@ -897,11 +897,20 @@
   <data name="FunctionValueUnexpected" xml:space="preserve">
     <value>This expression is a function value, i.e. is missing arguments. Its type is {0}.</value>
   </data>
-  <data name="UnitTypeExpected1" xml:space="preserve">
+  <data name="UnitTypeExpected" xml:space="preserve">
     <value>The result of this expression is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.</value>
   </data>
-  <data name="UnitTypeExpected2" xml:space="preserve">
-    <value>This expression should have type 'unit', but has type '{0}'. If assigning to a property use the syntax 'obj.Prop &lt;- expr'.</value>
+  <data name="UnitTypeExpectedWithEquality" xml:space="preserve">
+    <value>The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'.</value>
+  </data>
+  <data name="UnitTypeExpectedWithPossiblePropertySetter" xml:space="preserve">
+    <value>The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'. If you intended to set a value to a property, then use the '&lt;-' operator e.g. '{0}.{1} &lt;- expression'.</value>
+  </data>
+  <data name="UnitTypeExpectedWithPossibleAssignment" xml:space="preserve">
+    <value>The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'. If you intended to mutate a value, then mark the value 'mutable' and use the '&lt;-' operator e.g. '{0} &lt;- expression'.</value>
+  </data>
+  <data name="UnitTypeExpectedWithPossibleAssignmentToMutable" xml:space="preserve">
+    <value>The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'. If you intended to mutate a value, then use the '&lt;-' operator e.g. '{0} &lt;- expression'.</value>
   </data>
   <data name="RecursiveUseCheckedAtRuntime" xml:space="preserve">
     <value>This recursive use will be checked for initialization-soundness at runtime. This warning is usually harmless, and may be suppressed by using '#nowarn "21"' or '--nowarn:21'.</value>
@@ -1083,4 +1092,4 @@
   <data name="TargetInvocationExceptionWrapper" xml:space="preserve">
     <value>internal error: {0}</value>
   </data>
-</root>
+</root>

src/fsharp/TypeChecker.fs

@@ -83,7 +83,10 @@ exception FieldsFromDifferentTypes of DisplayEnv * RecdFieldRef * RecdFieldRef *
 exception FieldGivenTwice of DisplayEnv * Tast.RecdFieldRef * range
 exception MissingFields of string list * range
 exception FunctionValueUnexpected of DisplayEnv * TType * range
-exception UnitTypeExpected of DisplayEnv * TType * bool * range
+exception UnitTypeExpected of DisplayEnv * TType * range
+exception UnitTypeExpectedWithEquality of DisplayEnv * TType * range
+exception UnitTypeExpectedWithPossibleAssignment of DisplayEnv * TType * bool * string * range
+exception UnitTypeExpectedWithPossiblePropertySetter of DisplayEnv * TType * string * string * range
 exception UnionPatternsBindDifferentNames of range
 exception VarBoundTwice of Ident
 exception ValueRestriction of DisplayEnv * bool * Val * Typar * range
@@ -718,23 +721,59 @@ let UnifyFunctionType extraInfo cenv denv mFunExpr ty =
         | Some argm -> error (NotAFunction(denv,ty,mFunExpr,argm))
         | None ->    error (FunctionExpected(denv,ty,mFunExpr))
 
+let ReportImplicitlyIgnoredBoolExpression denv m ty expr =
+    let checkExpr m exprOpt =
+        match exprOpt with 
+        | Expr.App(Expr.Val(vf,_,_),_,_,exprs,_) when vf.LogicalName = opNameEquals ->
+            match exprs with 
+            | Expr.App(Expr.Val(propRef,_,_),_,_,Expr.Val(vf,_,_) :: _,_) :: _ ->
+                if propRef.IsPropertyGetterMethod then
+                    let propertyName = propRef.PropertyName
+                    let hasCorrespondingSetter =
+                        match propRef.ActualParent with
+                        | Parent entityRef ->
+                            entityRef.MembersOfFSharpTyconSorted
+                            |> List.exists (fun valRef -> valRef.IsPropertySetterMethod && valRef.PropertyName = propertyName)
+                        | _ -> false
+
+                    if hasCorrespondingSetter then
+                        UnitTypeExpectedWithPossiblePropertySetter (denv,ty,vf.DisplayName,propertyName,m)
+                    else
+                        UnitTypeExpectedWithEquality (denv,ty,m)
+                else
+                    UnitTypeExpectedWithEquality (denv,ty,m)
+            | Expr.Op(TOp.ILCall(_,_,_,_,_,_,_,methodRef,_,_,_),_,Expr.Val(vf,_,_) :: _,_) :: _ when methodRef.Name.StartsWith "get_"->
+                UnitTypeExpectedWithPossiblePropertySetter (denv,ty,vf.DisplayName,PrettyNaming.ChopPropertyName(methodRef.Name),m)
+            | Expr.Val(vf,_,_) :: _ -> 
+                UnitTypeExpectedWithPossibleAssignment (denv,ty,vf.IsMutable,vf.DisplayName,m)
+            | _ -> UnitTypeExpectedWithEquality (denv,ty,m)
+        | _ -> UnitTypeExpected (denv,ty,m)
+
+    match expr with 
+    | Some(Expr.Let(_,Expr.Sequential(_,inner,_,_,_),_,_))
+    | Some(Expr.Sequential(_,inner,_,_,_)) ->
+        let rec extractNext expr =
+            match expr with
+            | Expr.Sequential(_,inner,_,_,_) -> extractNext inner
+            | _ -> checkExpr expr.Range expr
+        extractNext inner
+    | Some expr -> checkExpr m expr
+    | _ -> UnitTypeExpected (denv,ty,m)
 
 let UnifyUnitType cenv denv m ty exprOpt =
-    if not (AddCxTypeEqualsTypeUndoIfFailed denv cenv.css m ty cenv.g.unit_ty) then 
+    if AddCxTypeEqualsTypeUndoIfFailed denv cenv.css m ty cenv.g.unit_ty then 
+        true
+    else
         let domainTy = NewInferenceType ()
         let resultTy = NewInferenceType ()
         if AddCxTypeEqualsTypeUndoIfFailed denv cenv.css m ty (domainTy --> resultTy) then 
             warning (FunctionValueUnexpected(denv,ty,m))
-        else
-            let perhapsProp = 
-                typeEquiv cenv.g cenv.g.bool_ty ty &&
-                match exprOpt with 
-                | Some(Expr.App(Expr.Val(vf,_,_),_,_,[__],_)) when vf.LogicalName = opNameEquals -> true
-                | _ -> false
-            warning (UnitTypeExpected (denv,ty,perhapsProp,m)) 
+        else        
+            if not (typeEquiv cenv.g cenv.g.bool_ty ty) then 
+                warning (UnitTypeExpected (denv,ty,m)) 
+            else
+                warning (ReportImplicitlyIgnoredBoolExpression denv m ty exprOpt)
         false
-    else
-        true
 
 //-------------------------------------------------------------------------
 // Attribute target flags
@@ -2249,7 +2288,8 @@ module GeneralizationHelpers =
         | Some memberFlags -> 
             match memberFlags.MemberKind with 
             // can't infer extra polymorphism for properties 
-            | MemberKind.PropertyGet  | MemberKind.PropertySet  -> 
+            | MemberKind.PropertyGet 
+            | MemberKind.PropertySet -> 
                  if not (isNil declaredTypars) then 
                      errorR(Error(FSComp.SR.tcPropertyRequiresExplicitTypeParameters(),m))
             | MemberKind.Constructor -> 

src/fsharp/TypeChecker.fsi

@@ -75,7 +75,10 @@ exception UnionCaseWrongNumberOfArgs of DisplayEnv * int * int * range
 exception FieldsFromDifferentTypes of DisplayEnv * RecdFieldRef * RecdFieldRef * range
 exception FieldGivenTwice of DisplayEnv * RecdFieldRef * range
 exception MissingFields of string list * range
-exception UnitTypeExpected of DisplayEnv * TType * bool * range
+exception UnitTypeExpected of DisplayEnv * TType * range
+exception UnitTypeExpectedWithEquality of DisplayEnv * TType * range
+exception UnitTypeExpectedWithPossiblePropertySetter of DisplayEnv * TType * string * string * range
+exception UnitTypeExpectedWithPossibleAssignment of DisplayEnv * TType * bool * string * range
 exception FunctionValueUnexpected of DisplayEnv * TType * range
 exception UnionPatternsBindDifferentNames of range
 exception VarBoundTwice of Ast.Ident

src/fsharp/tast.fs

@@ -3100,6 +3100,18 @@ and
     ///   - If this is an implementation of an abstract slot then this is the name of the property implemented by the abstract slot
     member x.PropertyName               = x.Deref.PropertyName
 
+    /// Indicates whether this value represents a property getter.
+    member x.IsPropertyGetterMethod = 
+        match x.MemberInfo with
+        | None -> false
+        | Some (memInfo:ValMemberInfo) -> memInfo.MemberFlags.MemberKind = MemberKind.PropertyGet || memInfo.MemberFlags.MemberKind = MemberKind.PropertyGetSet
+
+    /// Indicates whether this value represents a property setter.
+    member x.IsPropertySetterMethod = 
+        match x.MemberInfo with
+        | None -> false
+        | Some (memInfo:ValMemberInfo) -> memInfo.MemberFlags.MemberKind = MemberKind.PropertySet || memInfo.MemberFlags.MemberKind = MemberKind.PropertyGetSet
+
     /// A unique stamp within the context of this invocation of the compiler process 
     member x.Stamp                      = x.Deref.Stamp
 

src/fsharp/vs/Symbols.fs

@@ -1357,31 +1357,22 @@ and FSharpMemberOrFunctionOrValue(cenv, d:FSharpMemberOrValData, item) =
         x.IsPropertyGetterMethod ||
         match fsharpInfo() with 
         | None -> false
-        | Some v -> 
-        match v.MemberInfo with 
-        | None -> false 
-        | Some memInfo -> memInfo.MemberFlags.MemberKind = MemberKind.PropertyGet
+        | Some v -> v.IsPropertyGetterMethod
 
     member x.IsSetterMethod =  
         if isUnresolved() then false else 
         x.IsPropertySetterMethod ||
         match fsharpInfo() with 
         | None -> false
-        | Some v -> 
-        match v.MemberInfo with 
-        | None -> false 
-        | Some memInfo -> memInfo.MemberFlags.MemberKind = MemberKind.PropertySet
+        | Some v -> v.IsPropertySetterMethod
 
     member __.IsPropertyGetterMethod = 
         if isUnresolved() then false else 
         match d with 
         | M m when m.LogicalName.StartsWith("get_") -> 
             let propName = PrettyNaming.ChopPropertyName(m.LogicalName) 
             not (isNil (GetImmediateIntrinsicPropInfosOfType(Some propName, AccessibleFromSomeFSharpCode) cenv.g cenv.amap range0 (generalizedTyconRef m.DeclaringEntityRef)))
-        | V v -> 
-            match v.MemberInfo with 
-            | None -> false 
-            | Some memInfo -> memInfo.MemberFlags.MemberKind = MemberKind.PropertyGet
+        | V v -> v.IsPropertyGetterMethod
         | _ -> false
 
     member __.IsPropertySetterMethod = 
@@ -1391,10 +1382,7 @@ and FSharpMemberOrFunctionOrValue(cenv, d:FSharpMemberOrValData, item) =
         | M m when m.LogicalName.StartsWith("set_") -> 
             let propName = PrettyNaming.ChopPropertyName(m.LogicalName) 
             not (isNil (GetImmediateIntrinsicPropInfosOfType(Some propName, AccessibleFromSomeFSharpCode) cenv.g cenv.amap range0 (generalizedTyconRef m.DeclaringEntityRef)))
-        | V v -> 
-            match v.MemberInfo with 
-            | None -> false 
-            | Some memInfo -> memInfo.MemberFlags.MemberKind = MemberKind.PropertySet
+        | V v -> v.IsPropertySetterMethod
         | _ -> false
 
     member __.IsInstanceMember = 

tests/fsharpqa/Source/Conformance/Expressions/BindingExpressions/Binding/in02.fs

@@ -4,7 +4,7 @@
 // I'm adding these cases to make sure we do not accidentally change the behavior from version to version
 // Eventually, we will deprecated them - and the specs will be updated.
 //
-//<Expects status="error" span="(11,5-12,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
+//<Expects status="error" span="(12,5-12,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
 //
 
 module B =

tests/fsharpqa/Source/Conformance/Expressions/BindingExpressions/Binding/in04.fs

@@ -4,7 +4,7 @@
 // I'm adding these cases to make sure we do not accidentally change the behavior from version to version
 // Eventually, we will deprecated them - and the specs will be updated.
 //
-//<Expects status="error" span="(11,5-13,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
+//<Expects status="error" span="(13,5-13,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
 //
 
 module D =

tests/fsharpqa/Source/Conformance/Expressions/BindingExpressions/Binding/in05.fs

@@ -5,7 +5,7 @@
 // Eventually, we will deprecated them - and the specs will be updated.
 //<Expects status="error" span="(12,13-12,14)" id="FS0001">The type 'int' does not match the type 'unit'$</Expects>
 //<Expects status="error" span="(12,18-12,24)" id="FS0001">Type mismatch\. Expecting a.    ''a -> 'b'    .but given a.    ''a -> unit'    .The type 'int' does not match the type 'unit'$</Expects>
-//<Expects status="error" span="(10,5-13,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
+//<Expects status="error" span="(13,5-13,10)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
 module E = 
     let a = 3 in
         a + 1 |> ignore

tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/QueryExpressions/E_WhereRequiresParens01.fs

@@ -1,7 +1,7 @@
 // #Conformance #DataExpressions #Query
 // Where expressions require parenthesis
+//<Expects status="error" span="(8,21-8,22)" id="FS3145">This is not a known query operator. Query operators are identifiers such as 'select', 'where', 'sortBy', 'thenBy', 'groupBy', 'groupValBy', 'join', 'groupJoin', 'sumBy' and 'averageBy', defined using corresponding methods on the 'QueryBuilder' type.</Expects>
 //<Expects status="error" span="(8,9-8,14)" id="FS3095">'where' is not used correctly\. This is a custom operation in this query or computation expression\.$</Expects>
-//<Expects status="error" span="(8,9-8,24)" id="FS0020">The result of this expression is implicitly ignored\. Consider using 'ignore' to discard this value explicitly, e\.g\. 'expr \|> ignore', or 'let' to bind the result to a name, e\.g\. 'let result = expr'.$</Expects>
 let query =
     query {
         for i in [1..10] do

tests/fsharpqa/Source/Warnings/DontWarnIfPropertyWithoutSetter.fs

@@ -0,0 +1,14 @@
+// #Warnings
+//<Expects status="Warning" span="(11,5)" id="FS0020">The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'.</Expects>
+
+type MyClass(property1 : int) =
+    member val Property2 = "" with get
+
+let x = MyClass(1)
+let y = "hello"
+
+let changeProperty() =
+    x.Property2 = "22"
+    y = "test"
+
+exit 0

tests/fsharpqa/Source/Warnings/WarnIfImplicitlyDiscarded.fs

@@ -0,0 +1,11 @@
+// #Warnings
+//<Expects status="Warning" span="(8,5)" id="FS0020">The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'.</Expects>
+
+let x = 10
+let y = 20
+
+let changeX() =
+    y * x = 20
+    y = 30
+
+exit 0