Fix misleading diagnostics for reordered record fields

azure-pipelines-PR.yml

@@ -298,46 +298,29 @@ stages:
             name: $(DncEngPublicBuildPool)
             demands: ImageOverride -equals $(_WindowsMachineQueueName)
           timeoutInMinutes: 120
+          strategy:
+            matrix:
+              Batch1:
+                batchNumber: 1
+              Batch2:
+                batchNumber: 2
+              Batch3:
+                batchNumber: 3
           steps:
-          - checkout: self
-            clean: true
-
-          - script: eng\CIBuildNoPublish.cmd -compressallmetadata -buildnorealsig -testDesktop -configuration Release
-            env:
-              FSharp_CacheEvictionImmediate: true
-              DOTNET_DbgEnableMiniDump: 1
-              DOTNET_DbgMiniDumpType: 2 # 1=mini, 2=heap, 3=triage, 4=full. Heap dumps include managed object data for debugging.
-              DOTNET_DbgMiniDumpName: $(Build.SourcesDirectory)\artifacts\log\Release\$(Build.BuildId)-%e-%p-%t.dmp
-              NativeToolsOnMachine: true
-            displayName: Build
-
-          - task: PublishTestResults@2
-            displayName: Publish Test Results
-            inputs:
-              testResultsFormat: 'XUnit'
-              testRunTitle: WindowsNoRealsig_testDesktop
-              mergeTestResults: true
-              testResultsFiles: '*.xml'
-              searchFolder: '$(Build.SourcesDirectory)/artifacts/TestResults/Release'
-            condition: succeededOrFailed()
-            continueOnError: true
-          - task: PublishBuildArtifacts@1
-            displayName: Publish Build BinLog
-            continueOnError: true
-            inputs:
-                PathToPublish: '$(Build.SourcesDirectory)\artifacts\log/Release\Build.VisualFSharp.slnx.binlog'
-                ArtifactName: 'Windows Release build binlogs'
-                ArtifactType: Container
-                parallel: true
-          - task: PublishBuildArtifacts@1
-            displayName: Publish Dumps
-            condition: failed()
-            continueOnError: true
-            inputs:
-                PathToPublish: '$(Build.SourcesDirectory)\artifacts\log\Release'
-                ArtifactName: 'Windows Release WindowsNoRealsig_testDesktop process dumps'
-                ArtifactType: Container
-                parallel: true
+          - template: /eng/templates/batched-test-steps.yml
+            parameters:
+              buildCommand: eng\CIBuildNoPublish.cmd -compressallmetadata -buildnorealsig -testDesktopBatch $(batchNumber) -configuration Release
+              buildEnv:
+                FSharp_CacheEvictionImmediate: true
+                DOTNET_DbgEnableMiniDump: 1
+                DOTNET_DbgMiniDumpType: 2
+                DOTNET_DbgMiniDumpName: $(Build.SourcesDirectory)\artifacts\log\Release\$(Build.BuildId)-%e-%p-%t.dmp
+                NativeToolsOnMachine: true
+              testRunTitlePrefix: 'WindowsNoRealsig_testDesktop'
+              artifactNamePrefix: 'WindowsNoRealsig testDesktop'
+              publishBinLog: true
+              binLogPath: '$(Build.SourcesDirectory)\artifacts\log/Release\Build.VisualFSharp.slnx.binlog'
+              publishDumps: true
 
         # Windows With Compressed Metadata
         - job: WindowsCompressedMetadata

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -29,6 +29,7 @@
 * Fix `MethodAccessException` under `--realsig+` when a closure (inner `let rec`, `task`/`async` state machine, or quotation splice) inside a member defined in an intrinsic type augmentation (`type C with member ...`) accesses a `private` member of `C`. The synthesized closure is now nested inside the declaring type instead of beside it in the module class. ([Issue #19933](https://github.com/dotnet/fsharp/issues/19933), [PR #19955](https://github.com/dotnet/fsharp/pull/19955))
 * Preserve source range for type errors on empty-bodied computation expressions (e.g. `foo {}`) in pipelines, function arguments, and type-annotated contexts, instead of reporting `unknown(1,1)`. ([Issue #19550](https://github.com/dotnet/fsharp/issues/19550), [PR #19849](https://github.com/dotnet/fsharp/pull/19849))
 * Fix multiline nested type arguments failing to parse when the closing `>` aligns with the opening type name's column. ([Issue #15171](https://github.com/dotnet/fsharp/issues/15171))
+* Remove misleading FS0193 when record fields differ in order between a signature and its implementation, while retaining FS0312. ([Issue #20410](https://github.com/dotnet/fsharp/issues/20410), [PR #20559](https://github.com/dotnet/fsharp/pull/20559))
 * Tooltip "Full name" now shows demangled companion module names (e.g. `MyType.func` instead of `MyTypeModule.func`). ([Issue #17335](https://github.com/dotnet/fsharp/issues/17335), [PR #19867](https://github.com/dotnet/fsharp/pull/19867))
 * Fix spurious FS0410 accessibility error when tuple-deconstructing bindings use private types in the same module scope. ([Issue #4161](https://github.com/dotnet/fsharp/issues/4161), [PR #19947](https://github.com/dotnet/fsharp/pull/19947))
 * Fix internal error (FS0193) when calling an indexed property setter with a named argument that matches an indexer parameter. ([Issue #16034](https://github.com/dotnet/fsharp/issues/16034), [PR #19851](https://github.com/dotnet/fsharp/pull/19851))

src/Compiler/Checking/SignatureConformance.fs

@@ -637,7 +637,9 @@ type Checker(g, amap, denv, remapInfo: SignatureRepackageInfo, checkingSig) =
 
             // This check is required because constructors etc. are externally visible
             // and thus compiled representations do pick up dependencies on the field order
-            (if List.forall2 (checkField aenv infoReader implTycon sigTycon)  implFields sigFields
+            (if List.forall2 (fun (implField: RecdField) (sigField: RecdField) ->
+                    implField.LogicalName = sigField.LogicalName &&
+                    checkField aenv infoReader implTycon sigTycon implField sigField) implFields sigFields
              then true
              else (errorR(Error (FSComp.SR.DefinitionsInSigAndImplNotCompatibleFieldOrderDiffer(kindText, implTyconName), m)); false))
 

tests/FSharp.Compiler.ComponentTests/Conformance/Signatures/Signatures.fs

@@ -166,3 +166,149 @@ type B(a: A) = class end
         |> compile
         |> shouldFail
         |> withErrorCode 0410
+
+    let private recordSignaturePair signature implementation =
+        Fsi ("module M\n" + signature)
+        |> withAdditionalSourceFile (FsSource ("module M\n" + implementation))
+        |> asLibrary
+
+    let private assertRecordDiagnostics expected (result: CompilationResult) =
+        // Raw diagnostic columns are zero-based; keep repeated warnings and their order.
+        let actual =
+            result.Output.Diagnostics
+            |> List.map (fun d ->
+                d.Error, System.IO.Path.GetFileName d.NativeRange.FileName,
+                (d.Range.StartLine, d.Range.StartColumn, d.Range.EndLine, d.Range.EndColumn),
+                d.Message.Replace("\r\n", "\n"))
+        Assert.True((expected = actual), sprintf "Expected:\n%A\nActual:\n%A" expected actual)
+
+    let private recordOrderDiagnostic typeName line column =
+        ErrorType.Error 312, "test.fs", (line, column, line, column + String.length typeName),
+        $"The type definitions for type '{typeName}' in the signature and implementation are not compatible because the order of the fields is different in the signature and implementation"
+
+    let private fieldMismatch column (implementation: string) (signature: string) (reason: string) =
+        ErrorType.Error 193, "test.fs", (2, column, 2, column + 1),
+        $"The module contains the field\n    {implementation}    \nbut its signature specifies\n    {signature}    \n{reason}"
+
+    [<Theory>]
+    [<InlineData(
+        """type FormatConfig = FormatConfig of int
+type ResolvedSetting = ResolvedSetting of int
+type EditorConfigProblem = EditorConfigProblem of int
+type ResolvedConfig =
+    {
+        Config: FormatConfig
+        Settings: ResolvedSetting list
+        EditorConfigFiles: string list
+        Problems: EditorConfigProblem list
+    }""",
+        """type FormatConfig = FormatConfig of int
+type ResolvedSetting = ResolvedSetting of int
+type EditorConfigProblem = EditorConfigProblem of int
+type ResolvedConfig =
+    {
+        Config: FormatConfig
+        EditorConfigFiles: string list
+        Problems: EditorConfigProblem list
+        Settings: ResolvedSetting list
+    }""", "ResolvedConfig", 5, false)>]
+    [<InlineData("type R = { A: int; B: string }", "type R = { B: string; A: int }", "R", 2, false)>]
+    [<InlineData("type R = { Prefix: bool; A: int; B: string; C: decimal }", "type R = { Prefix: bool; B: string; C: decimal; A: int }", "R", 2, false)>]
+    [<InlineData("type R = { A: int; B: int }", "type R = { B: int; A: int }", "R", 2, false)>]
+    [<InlineData("[<Struct>]\ntype R<'T> = { A: 'T; B: 'T list }", "[<Struct>]\ntype R<'T> = { B: 'T list; A: 'T }", "R", 3, false)>]
+    [<InlineData("type R = { [<System.Obsolete(\"sig\")>] A: int; B: string }", "type R = { B: string; [<System.Obsolete(\"impl\")>] A: int }", "R", 2, true)>]
+    let ``Issue 20410 - record field permutations retain only genuine diagnostics`` signature implementation typeName line attributeConflict =
+        let result = recordSignaturePair signature implementation |> compile |> shouldFail
+        Assert.Contains(result.Output.Diagnostics, fun d -> d.Error = ErrorType.Error 312)
+        result |> assertRecordDiagnostics [
+            if attributeConflict then
+                Warning 1200, "test.fs", (2, 24, 2, 47),
+                "The attribute 'ObsoleteAttribute' appears in both the implementation and the signature, but the attribute arguments differ. Only the attribute from the signature will be included in the compiled code."
+            recordOrderDiagnostic typeName line 5
+        ]
+
+    [<Fact>]
+    let ``Issue 20410 - aligned record fields compile`` () =
+        let declaration = "type R = { A: int; B: string }"
+        recordSignaturePair declaration declaration
+        |> compile
+        |> shouldSucceed
+        |> assertRecordDiagnostics []
+
+    [<Theory>]
+    [<InlineData("type R = { A: string; B: string }", "A: string", 11, "The types differ")>]
+    [<InlineData("type R = { mutable A: int; B: string }", "mutable A: int", 19, "The 'mutable' modifiers differ")>]
+    [<InlineData("type R = internal { A: int; B: string }", "A: int", 20, "the accessibility specified in the signature is more than that specified in the implementation")>]
+    let ``Issue 20410 - same-name field mismatches are preserved`` implementation field column reason =
+        recordSignaturePair "type R = { A: int; B: string }" implementation
+        |> compile
+        |> shouldFail
+        |> assertRecordDiagnostics [
+            if field = "A: int" then
+                fieldMismatch 28 "B: string" "B: string" reason
+            fieldMismatch column field "A: int" reason
+        ]
+
+    [<Theory>]
+    [<InlineData("type R = { A: int }", false)>]
+    [<InlineData("type R = { A: int; B: string; C: bool }", true)>]
+    [<InlineData("type R = { A: int; C: string }", false)>]
+    let ``Issue 20410 - different record field name sets are preserved`` implementation extra =
+        let code, reason =
+            if extra then 311, "C was present in the implementation but not in the signature"
+            else 313, "B was required by the signature but was not specified by the implementation"
+        recordSignaturePair "type R = { A: int; B: string }" implementation
+        |> compile
+        |> shouldFail
+        |> assertRecordDiagnostics [
+            ErrorType.Error code, "test.fs", (2, 5, 2, 6),
+            $"The type definitions for type 'R' in the signature and implementation are not compatible because the field {reason}"
+        ]
+
+    [<Theory>]
+    [<InlineData(false)>]
+    [<InlineData(true)>]
+    let ``Issue 20410 - matching prefix retains repeated nullness warnings`` swapped =
+        let fields = if swapped then "B: bool; A: int" else "A: int; B: bool"
+        let result =
+            recordSignaturePair
+                "type R = { Prefix: string; A: int; B: bool }"
+                $"type R = {{ Prefix: string | null; {fields} }}"
+            |> withLangVersionPreview
+            |> withCheckNulls
+            |> withWarnOn 3261
+            |> compile
+        if swapped then
+            result |> shouldFail |> ignore
+            Assert.Contains(result.Output.Diagnostics, fun d -> d.Error = ErrorType.Error 312)
+        result |> assertRecordDiagnostics [
+            for _ in 1..3 do
+                Warning 3261, "test.fs", (2, 11, 2, 17),
+                "Nullness warning: The module contains the field\n    Prefix: string | null    \nbut its signature specifies\n    Prefix: string    \nThe types differ in their nullness annotations"
+            if swapped then
+                recordOrderDiagnostic "R" 2 5
+        ]
+
+    [<Theory>]
+    [<InlineData("type U = Case of A: int * B: string", "type U = Case of B: string * A: int", 36)>]
+    [<InlineData("exception E of A: int * B: string", "exception E of B: string * A: int", 63)>]
+    [<InlineData("type C =\n    val A: int\n    val B: string", "type C =\n    val B: string\n    val A: int", 0)>]
+    [<InlineData("type R = { A: int; B: string }", "type R = { A: int; A: int }", 37)>]
+    let ``Issue 20410 - non-record and duplicate-field behavior is preserved`` signature implementation code =
+        let expected =
+            match code with
+            | 36 -> [
+                fieldMismatch 17 "B: string" "A: int" "The names differ"
+                ErrorType.Error 36, "test.fs", (2, 9, 2, 13),
+                "The module contains the constructor\n    | Case of B: string * A: int    \nbut its signature specifies\n    | Case of A: int * B: string    \nThe types of the fields differ"
+              ]
+            | 63 -> [
+                fieldMismatch 15 "B: string" "A: int" "The names differ"
+                ErrorType.Error 63, "test.fs", (2, 10, 2, 11),
+                "The exception definitions are not compatible because the order of the fields is different in the signature and implementation. The module contains the exception definition\n    exception E of B: string * A: int    \nbut its signature specifies\n\texception E of A: int * B: string."
+              ]
+            | 37 -> [ErrorType.Error 37, "test.fs", (2, 19, 2, 20), "Duplicate definition of field 'A'"]
+            | 0 -> []
+            | _ -> failwithf "Unexpected control diagnostic: %d" code
+        let result = recordSignaturePair signature implementation |> compile
+        result |> (if code = 0 then shouldSucceed else shouldFail) |> assertRecordDiagnostics expected