Fix nested copy+updates; update tests

Author: brianrourkeboll

src/Compiler/Checking/CheckPatterns.fs

@@ -457,13 +457,22 @@ and TcPatArrayOrList warnOnUpper cenv env vFlags patEnv ty isArray args m =
     phase2, acc
 
 and TcRecordPat warnOnUpper (cenv: cenv) env vFlags patEnv ty fieldPats m =
+    let idents =
+        fieldPats
+        |> List.map (fun fieldPat ->
+            let (|FrontAndBack|) = List.frontAndBack
+            match fieldPat with
+            | NamePatPairField (fieldName = SynLongIdent (id = [fieldId]))
+            | NamePatPairField (fieldName = SynLongIdent (id = FrontAndBack (_, fieldId))) -> fieldId)
+
     let fieldPats =
         fieldPats 
         |> List.map (fun (NamePatPairField(fieldName = fieldLid; pat = pat)) -> 
             match fieldLid.LongIdent with
             | [id] -> ExplicitOrSpread.Explicit (([], id), pat)
             | lid -> ExplicitOrSpread.Explicit (List.frontAndBack lid, pat))
 
+    CheckRecdExprDuplicateFields idents
     match BuildFieldMap cenv env false ty fieldPats m with
     | None -> (fun _ -> TPat_error m), patEnv
     | Some(tinst, tcref, fldsmap, _fldsList) ->

src/Compiler/Checking/CheckRecordSyntaxHelpers.fs

@@ -151,25 +151,16 @@ let TransformAstForNestedUpdates (cenv: TcFileState) (env: TcEnv) overallTy (lid
 
     match access, fields with
     | _, [] -> failwith "unreachable"
-    | accessIds, [ (fieldId, _) ] ->
-        match exprBeingAssigned with
-        | ExplicitOrSpread.Explicit exprBeingAssigned -> ExplicitOrSpread.Explicit((accessIds, fieldId), Some exprBeingAssigned)
-        | ExplicitOrSpread.Spread exprBeingAssigned -> ExplicitOrSpread.Spread((accessIds, fieldId), Some exprBeingAssigned)
-
+    | accessIds, [ (fieldId, _) ] -> (accessIds, fieldId), Some exprBeingAssigned
     | accessIds, (outerFieldId, item) :: rest ->
         checkLanguageFeatureAndRecover cenv.g.langVersion LanguageFeature.NestedCopyAndUpdate (rangeOfLid lid)
 
         CallNameResolutionSink cenv.tcSink (outerFieldId.idRange, env.NameEnv, item, [], ItemOccurrence.Use, env.AccessRights)
 
         let outerFieldId = ident (outerFieldId.idText, outerFieldId.idRange.MakeSynthetic())
 
-        match exprBeingAssigned with
-        | ExplicitOrSpread.Explicit synExpr ->
-            ExplicitOrSpread.Explicit(
-                (accessIds, outerFieldId),
-                Some(synExprRecd (recdExprCopyInfo (fields |> List.map fst) withExpr) outerFieldId rest synExpr)
-            )
-        | ExplicitOrSpread.Spread exprBeingAssigned -> ExplicitOrSpread.Spread((accessIds, outerFieldId), Some exprBeingAssigned)
+        (accessIds, outerFieldId),
+        Some(synExprRecd (recdExprCopyInfo (fields |> List.map fst) withExpr) outerFieldId rest exprBeingAssigned)
 
 /// When the original expression in copy-and-update is more complex than `{ x with ... }`, like `{ f () with ... }`,
 /// we bind it first, so that it's not evaluated multiple times during a nested update

src/Compiler/Checking/CheckRecordSyntaxHelpers.fsi

@@ -17,9 +17,9 @@ val TransformAstForNestedUpdates:
     env: TcEnv ->
     overallTy: TType ->
     lid: LongIdent ->
-    exprBeingAssigned: ExplicitOrSpread<SynExpr, 'Spread> ->
+    exprBeingAssigned: SynExpr ->
     withExpr: SynExpr * BlockSeparator ->
-        ExplicitOrSpread<(Ident list * Ident) * SynExpr option, (Ident list * Ident) * 'Spread option>
+        (Ident list * Ident) * SynExpr option
 
 val BindOriginalRecdExpr:
     withExpr: SynExpr * BlockSeparator -> mkRecdExpr: ((SynExpr * BlockSeparator) option -> SynExpr) -> SynExpr

src/Compiler/Checking/Expressions/CheckExpressions.fs

@@ -1918,11 +1918,6 @@ let BuildFieldMap (cenv: cenv) env isPartial ty (flds: ExplicitOrSpread<(Ident l
     let ad = env.eAccessRights
 
     let allFields = flds |> List.map (fun (ExplicitOrSpread.Explicit ((_, ident), _) | ExplicitOrSpread.Spread ((_, ident), _)) -> ident)
-    if allFields.Length > 1 then
-        // In the case of nested record fields on the same level in record copy-and-update.
-        // We need to reverse the list to get the correct order of fields.
-        let idents = if isPartial then allFields |> List.rev else allFields
-        CheckRecdExprDuplicateFields idents
 
     let fldResolutions =
         flds
@@ -7804,11 +7799,15 @@ and TcRecdExpr cenv overallTy env tpenv (inherits, withExprOpt, synRecdFields, m
                         // we assume that parse errors were already reported
                         raise (ReportedError None)
 
-                    let ExplicitOrSpread.Explicit ((_, fieldId), _) | ExplicitOrSpread.Spread ((_, fieldId), _) as field =
+                    let isFromNestedUpdate, fieldId, field =
                         match withExprOpt, synLongId.LongIdent, exprBeingAssigned with
-                        | _, [ id ], _ -> ExplicitOrSpread.Explicit (([], id), exprBeingAssigned)
-                        | Some (origExpr, blockSep), lid, Some exprBeingAssigned -> TransformAstForNestedUpdates cenv env overallTy lid (ExplicitOrSpread.Explicit exprBeingAssigned) (origExpr, blockSep)
-                        | _ -> ExplicitOrSpread.Explicit (List.frontAndBack synLongId.LongIdent, exprBeingAssigned)
+                        | _, [ id ], _ -> false, id, ExplicitOrSpread.Explicit (([], id), exprBeingAssigned)
+                        | Some (origExpr, blockSep), lid, Some exprBeingAssigned ->
+                            let _, id as longIdent, exprBeingAssigned = TransformAstForNestedUpdates cenv env overallTy lid exprBeingAssigned (origExpr, blockSep)
+                            true, id, ExplicitOrSpread.Explicit (longIdent, exprBeingAssigned)
+                        | _ ->
+                            let _, id as longIdent = List.frontAndBack synLongId.LongIdent
+                            false, id, ExplicitOrSpread.Explicit (longIdent, exprBeingAssigned)
 
                     let flds =
                         flds |> Map.change fieldId.idText (function
@@ -7827,7 +7826,8 @@ and TcRecdExpr cenv overallTy env tpenv (inherits, withExprOpt, synRecdFields, m
                             //
                             // Keep both, but error.
                             | Some (LeftwardExplicit, dupes) ->
-                                errorR (Duplicate ("field", fieldId.idText, m))
+                                if not isFromNestedUpdate then
+                                    errorR (Error (FSComp.SR.tcMultipleFieldsInRecord fieldId.idText, m))
                                 Some (LeftwardExplicit, (i, field) :: dupes)
 
                             // Rightward explicit field shadowing leftward spread field.
@@ -8359,11 +8359,13 @@ and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, (or
                 List.rev spreadSrcs, fieldsInOriginalOrder, tpenv
 
             | SynExprAnonRecordFieldOrSpread.Field (SynExprAnonRecordField (fieldName = synLongIdent; expr = exprBeingAssigned; range = m), _) :: fieldsAndSpreads ->
-                let ExplicitOrSpread.Explicit ((_, fieldId), _) | ExplicitOrSpread.Spread ((_, fieldId), _) as field =
+                let isFromNestedUpdate, fieldId, field =
                     match synLongIdent.LongIdent with
                     | [] -> error(Error(FSComp.SR.nrUnexpectedEmptyLongId(), mWholeExpr))
-                    | [ id ] -> ExplicitOrSpread.Explicit (([], id), Some exprBeingAssigned)
-                    | lid -> TransformAstForNestedUpdates cenv env origExprTy lid (ExplicitOrSpread.Explicit exprBeingAssigned) (origExpr, blockSeparator)
+                    | [ id ] -> false, id, ExplicitOrSpread.Explicit (([], id), Some exprBeingAssigned)
+                    | lid ->
+                        let _, id as longIdent, exprBeingAssigned = TransformAstForNestedUpdates cenv env origExprTy lid exprBeingAssigned (origExpr, blockSeparator)
+                        true, id, ExplicitOrSpread.Explicit (longIdent, exprBeingAssigned)
 
                 let flds =
                     flds |> Map.change fieldId.idText (function
@@ -8382,7 +8384,8 @@ and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, (or
                         //
                         // Keep both, but error.
                         | Some (LeftwardExplicit, dupes) ->
-                            errorR (Error (FSComp.SR.tcAnonRecdDuplicateFieldId fieldId.idText, m))
+                            if not isFromNestedUpdate then
+                                errorR (Error (FSComp.SR.tcAnonRecdDuplicateFieldId fieldId.idText, m))
                             Some (LeftwardExplicit, (i, field) :: dupes)
 
                         // Rightward explicit field shadowing leftward spread field.

src/Compiler/Checking/Expressions/CheckExpressions.fsi

@@ -895,6 +895,9 @@ val UnifyTupleTypeAndInferCharacteristics:
     'T list ->
         TupInfo * TTypes
 
+/// Helper used to check for duplicate fields in records.
+val CheckRecdExprDuplicateFields: elems: Ident list -> unit
+
 /// Helper used to check both record expressions and record patterns
 val BuildFieldMap:
     cenv: TcFileState ->

tests/FSharp.Compiler.ComponentTests/Conformance/Constraints/Unmanaged.fs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
+// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
 
 namespace Conformance.Constraints
 
@@ -95,7 +95,7 @@ test (ValueSome (struct {|Field = Some 42|}))
         |> withDiagnostics [
                 Error 1, Line 3, Col 17, Line 3, Col 22, "A generic construct requires that the type 'string' is an unmanaged type"
                 Error 1, Line 4, Col 28, Line 4, Col 33, "A generic construct requires that the type 'string' is an unmanaged type"
-                Error 1, Line 5, Col 35, Line 5, Col 42, "A generic construct requires that the type ''a option' is an unmanaged type" ]
+                Error 1, Line 5, Col 35, Line 5, Col 42, "A generic construct requires that the type 'int option' is an unmanaged type" ]
 
     [<Fact>]
     let ``Option not considered unmanaged`` () = 

tests/FSharp.Compiler.ComponentTests/Conformance/Types/RecordTypes/AnonymousRecords.fs

@@ -446,7 +446,7 @@ let v = {| A = 1; A = 2 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 13, "The field 'A' appears multiple times in this record expression.")
+            (Error 3522, Line 2, Col 19, Line 2, Col 24, "The field 'A' appears multiple times in this record expression.")
         ]
 
     [<Fact>]
@@ -457,8 +457,8 @@ let v = {| A = 1; A = 2; A = 3 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 13, "The field 'A' appears multiple times in this record expression.")
-            (Error 3522, Line 2, Col 19, Line 2, Col 20, "The field 'A' appears multiple times in this record expression.")
+            Error 3522, Line 2, Col 19, Line 2, Col 24, "The field 'A' appears multiple times in this record expression."
+            Error 3522, Line 2, Col 26, Line 2, Col 31, "The field 'A' appears multiple times in this record expression."
         ]
 
     [<Fact>]
@@ -469,8 +469,8 @@ let v = {| A = 0; B = 2; A = 5; B = 6 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 13, "The field 'A' appears multiple times in this record expression.")
-            (Error 3522, Line 2, Col 19, Line 2, Col 20, "The field 'B' appears multiple times in this record expression.")
+            Error 3522, Line 2, Col 26, Line 2, Col 31, "The field 'A' appears multiple times in this record expression."
+            Error 3522, Line 2, Col 33, Line 2, Col 38, "The field 'B' appears multiple times in this record expression."
         ]
 
     [<Fact>]
@@ -481,7 +481,7 @@ let v = {| A = 2; C = "W"; A = 8; B = 6 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 13, "The field 'A' appears multiple times in this record expression.")
+            Error 3522, Line 2, Col 28, Line 2, Col 33, "The field 'A' appears multiple times in this record expression."
         ]
 
     [<Fact>]
@@ -492,8 +492,8 @@ let v = {| A = 0; C = ""; A = 1; B = 2; A = 5 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 13, "The field 'A' appears multiple times in this record expression.")
-            (Error 3522, Line 2, Col 27, Line 2, Col 28, "The field 'A' appears multiple times in this record expression.")
+            Error 3522, Line 2, Col 27, Line 2, Col 32, "The field 'A' appears multiple times in this record expression."
+            Error 3522, Line 2, Col 41, Line 2, Col 46, "The field 'A' appears multiple times in this record expression."
         ]
 
     [<Fact>]
@@ -504,8 +504,8 @@ let v = {| ``A`` = 0; B = 5; A = ""; B = 0 |}
         |> compile
         |> shouldFail
         |> withDiagnostics [
-            (Error 3522, Line 2, Col 12, Line 2, Col 17, "The field 'A' appears multiple times in this record expression.")
-            (Error 3522, Line 2, Col 23, Line 2, Col 24, "The field 'B' appears multiple times in this record expression.")
+            Error 3522, Line 2, Col 30, Line 2, Col 36, "The field 'A' appears multiple times in this record expression."
+            Error 3522, Line 2, Col 38, Line 2, Col 43, "The field 'B' appears multiple times in this record expression."
         ]
 
     [<Fact>]

tests/FSharp.Compiler.ComponentTests/Conformance/Types/RecordTypes/RecordTypes.fs

@@ -441,7 +441,7 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 17, "The field 'B' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 25, Line 4, Col 32, "The field 'B' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]
@@ -454,8 +454,8 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 17, "The field 'B' appears multiple times in this record expression or pattern")
-            (Error 668, Line 4, Col 25, Line 4, Col 26, "The field 'B' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 25, Line 4, Col 32, "The field 'B' appears multiple times in this record expression or pattern"
+            Error 668, Line 4, Col 34, Line 4, Col 41, "The field 'B' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]
@@ -468,8 +468,8 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 17, "The field 'A' appears multiple times in this record expression or pattern")
-            (Error 668, Line 4, Col 23, Line 4, Col 24, "The field 'B' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 30, Line 4, Col 35, "The field 'A' appears multiple times in this record expression or pattern"
+            Error 668, Line 4, Col 37, Line 4, Col 42, "The field 'B' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]
@@ -482,7 +482,7 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 17, "The field 'A' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 31, Line 4, Col 36, "The field 'A' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]
@@ -495,8 +495,8 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 17, "The field 'A' appears multiple times in this record expression or pattern")
-            (Error 668, Line 4, Col 31, Line 4, Col 32, "The field 'A' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 31, Line 4, Col 36, "The field 'A' appears multiple times in this record expression or pattern"
+            Error 668, Line 4, Col 45, Line 4, Col 50, "The field 'A' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]
@@ -509,8 +509,8 @@ module RecordTypes =
         |> typecheck
         |> shouldFail
         |> withDiagnostics [
-            (Error 668, Line 4, Col 16, Line 4, Col 21, "The field 'A' appears multiple times in this record expression or pattern")
-            (Error 668, Line 4, Col 27, Line 4, Col 28, "The field 'B' appears multiple times in this record expression or pattern")
+            Error 668, Line 4, Col 34, Line 4, Col 39, "The field 'A' appears multiple times in this record expression or pattern"
+            Error 668, Line 4, Col 41, Line 4, Col 46, "The field 'B' appears multiple times in this record expression or pattern"
         ]
 
     [<Fact>]

tests/FSharp.Compiler.ComponentTests/Language/CopyAndUpdateTests.fs

@@ -1,4 +1,4 @@
-module Language.CopyAndUpdateTests
+module Language.CopyAndUpdateTests
 
 open Xunit
 open FSharp.Test.Compiler
@@ -17,7 +17,7 @@ let t2 x = { x with D.B = "a"; D.B = "b" }
     |> typecheck
     |> shouldFail
     |> withDiagnostics [
-        (Error 668, Line 6, Col 23, Line 6, Col 24, "The field 'B' appears multiple times in this record expression or pattern")
+        Error 668, Line 6, Col 34, Line 6, Col 41, "The field 'B' appears multiple times in this record expression or pattern"
     ]
 
 [<Fact>]
@@ -32,8 +32,8 @@ let t2 x = { x with D.B = "a"; D.B = "b"; D.B = "c" }
     |> typecheck
     |> shouldFail
     |> withDiagnostics [
-        (Error 668, Line 6, Col 23, Line 6, Col 24, "The field 'B' appears multiple times in this record expression or pattern")
-        (Error 668, Line 6, Col 34, Line 6, Col 35, "The field 'B' appears multiple times in this record expression or pattern")
+        Error 668, Line 6, Col 34, Line 6, Col 41, "The field 'B' appears multiple times in this record expression or pattern"
+        Error 668, Line 6, Col 45, Line 6, Col 52, "The field 'B' appears multiple times in this record expression or pattern"
     ]
 
 [<Fact>]
@@ -48,8 +48,8 @@ let t2 x = { x with D.B = "a"; D.C = ""; D.B = "c" ; D.C = "d" }
     |> typecheck
     |> shouldFail
     |> withDiagnostics [
-        (Error 668, Line 6, Col 34, Line 6, Col 35, "The field 'C' appears multiple times in this record expression or pattern")
-        (Error 668, Line 6, Col 23, Line 6, Col 24, "The field 'B' appears multiple times in this record expression or pattern")
+        Error 668, Line 6, Col 44, Line 6, Col 51, "The field 'B' appears multiple times in this record expression or pattern"
+        Error 668, Line 6, Col 56, Line 6, Col 63, "The field 'C' appears multiple times in this record expression or pattern"
     ]
 
 [<Fact>]

tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs

@@ -1012,7 +1012,7 @@ myNullReturningFunction myValOfY                     |> ignore
     |> withDiagnostics     
                 [Error 3261, Line 17, Col 25, Line 17, Col 34, "Nullness warning: The type 'string' does not support 'null'."
                  Error 193, Line 19, Col 26, Line 19, Col 45, "The type 'System.DateTime' does not have 'null' as a proper value"
-                 Error 1, Line 20, Col 25, Line 20, Col 36, "The type '{| Anon: 'a |}' does not have 'null' as a proper value"
+                 Error 1, Line 20, Col 25, Line 20, Col 36, "The type '{| Anon: int |}' does not have 'null' as a proper value"
                  Error 1, Line 21, Col 26, Line 21, Col 31, "The type ''a * 'b * 'c' does not have 'null' as a proper value"
                  Error 1, Line 23, Col 25, Line 23, Col 33, "The type 'Y' does not have 'null' as a proper value"]