From 14f4369191af0c1d1d151556925c91b8ecdbde68 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Tue, 13 Aug 2024 12:24:38 +0100 Subject: [PATCH] Better error reporting for unions duplicated fields (#17521) * Better error reporting for unions duplicated fields * update tests * release notes --- .../.FSharp.Compiler.Service/9.0.100.md | 1 + src/Compiler/Checking/CheckDeclarations.fs | 18 ++++-- .../E_ExnFieldConflictingName.fs | 4 +- .../ExceptionDefinitions.fs | 6 +- .../UnionTypes/E_UnionFieldConflictingName.fs | 2 +- .../Types/UnionTypes/UnionStructTypes.fs | 9 ++- .../Types/UnionTypes/UnionTypes.fs | 59 ++++++++++++++++++- 7 files changed, 87 insertions(+), 12 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md index f01e40cafc0..a11ec24aba8 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md @@ -27,4 +27,5 @@ * Enforce `AttributeTargets` on unions. ([PR #17389](https://github.com/dotnet/fsharp/pull/17389)) * Ensure that isinteractive multi-emit backing fields are not public. ([Issue #17439](https://github.com/dotnet/fsharp/issues/17438)), ([PR #17439](https://github.com/dotnet/fsharp/pull/17439)) * Enable FSharp 9.0 Language Version ([Issue #17497](https://github.com/dotnet/fsharp/issues/17438)), [PR](https://github.com/dotnet/fsharp/pull/17500))) +* Better error reporting for unions with duplicated fields. ([PR #17521](https://github.com/dotnet/fsharp/pull/17521)) ### Breaking Changes diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index b1fdc7f3bfe..b1ed720c973 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -500,18 +500,26 @@ module TcRecdUnionAndEnumDeclarations = if not (String.isLeadingIdentifierCharacterUpperCase name) && name <> opNameCons && name <> opNameNil then errorR(NotUpperCaseConstructor(id.idRange)) - let ValidateFieldNames (synFields: SynField list, tastFields: RecdField list) = + let private CheckUnionDuplicateFields (elems: Ident list) = + elems |> List.iteri (fun i (uc1: Ident) -> + elems |> List.iteri (fun j (uc2: Ident) -> + if j > i && uc1.idText = uc2.idText then + errorR(Error(FSComp.SR.tcFieldNameIsUsedModeThanOnce(uc1.idText), uc1.idRange)))) + + let ValidateFieldNames (synFields: SynField list, tastFields: RecdField list) = + let fields = synFields |> List.choose (function SynField(idOpt = Some ident) -> Some ident | _ -> None) + if fields.Length > 1 then + CheckUnionDuplicateFields fields + let seen = Dictionary() (synFields, tastFields) ||> List.iter2 (fun sf f -> match seen.TryGetValue f.LogicalName with | true, synField -> match sf, synField with - | SynField(idOpt = Some id), SynField(idOpt = Some _) -> - error(Error(FSComp.SR.tcFieldNameIsUsedModeThanOnce(id.idText), id.idRange)) | SynField(idOpt = Some id), SynField(idOpt = None) | SynField(idOpt = None), SynField(idOpt = Some id) -> - error(Error(FSComp.SR.tcFieldNameConflictsWithGeneratedNameForAnonymousField(id.idText), id.idRange)) - | _ -> assert false + errorR(Error(FSComp.SR.tcFieldNameConflictsWithGeneratedNameForAnonymousField(id.idText), id.idRange)) + | _ -> () | _ -> seen.Add(f.LogicalName, sf)) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/E_ExnFieldConflictingName.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/E_ExnFieldConflictingName.fs index db51a6e7dda..d6921cd1b85 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/E_ExnFieldConflictingName.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/E_ExnFieldConflictingName.fs @@ -5,4 +5,6 @@ exception AAA of Data1 : int * string -exception BBB of A : int * A : string \ No newline at end of file +exception BBB of A : int * A : string + +exception CCC of A : int * A : string * A : int \ No newline at end of file diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/ExceptionDefinitions.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/ExceptionDefinitions.fs index 9e39ffa3233..70e113cf929 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/ExceptionDefinitions.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ExceptionDefinitions/ExceptionDefinitions.fs @@ -226,8 +226,10 @@ module ExceptionDefinition = |> compile |> shouldFail |> withDiagnostics [ - (Error 3176, Line 6, Col 18, Line 6, Col 23, "Named field 'Data1' conflicts with autogenerated name for anonymous field.") - (Error 3176, Line 8, Col 28, Line 8, Col 29, "Named field 'A' is used more than once.") + (Error 3176, Line 6, Col 18, Line 6, Col 23, "Named field 'Data1' conflicts with autogenerated name for anonymous field."); + (Error 3176, Line 8, Col 18, Line 8, Col 19, "Named field 'A' is used more than once."); + (Error 3176, Line 10, Col 18, Line 10, Col 19, "Named field 'A' is used more than once."); + (Error 3176, Line 10, Col 28, Line 10, Col 29, "Named field 'A' is used more than once.") ] // SOURCE=E_FieldNameUsedMulti.fs SCFLAGS="--test:ErrorRanges" # E_FieldNameUsedMulti.fs diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/E_UnionFieldConflictingName.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/E_UnionFieldConflictingName.fs index 828b1f5c1d2..d33eea473fd 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/E_UnionFieldConflictingName.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/E_UnionFieldConflictingName.fs @@ -7,4 +7,4 @@ type MyDU = | Case1 of Item2 : int * string type MyDU2 = - | Case1 of A : int * A : string \ No newline at end of file + | Case1 of A : int * A : string * A : int \ No newline at end of file diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionStructTypes.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionStructTypes.fs index e480a484524..57f639540c3 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionStructTypes.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionStructTypes.fs @@ -402,7 +402,9 @@ type StructUnion = |> typecheck |> shouldFail |> withDiagnostics [ - (Error 3176, Line 5, Col 27, Line 5, Col 31, "Named field 'item' is used more than once.") + (Error 3176, Line 5, Col 12, Line 5, Col 16, "Named field 'item' is used more than once."); + (Error 3585, Line 5, Col 12, Line 5, Col 16, "If a multicase union type is a struct, then all fields with the same name must be of the same type. This rule applies also to the generated 'Item' name in case of unnamed fields."); + (Error 3585, Line 5, Col 27, Line 5, Col 31, "If a multicase union type is a struct, then all fields with the same name must be of the same type. This rule applies also to the generated 'Item' name in case of unnamed fields.") ] [] @@ -417,7 +419,10 @@ type StructUnion = |> typecheck |> shouldFail |> withDiagnostics [ - (Error 3176, Line 5, Col 27, Line 5, Col 31, "Named field 'Item' is used more than once.") + (Error 3176, Line 5, Col 12, Line 5, Col 16, "Named field 'Item' is used more than once."); + (Error 3585, Line 5, Col 12, Line 5, Col 16, "If a multicase union type is a struct, then all fields with the same name must be of the same type. This rule applies also to the generated 'Item' name in case of unnamed fields."); + (Error 3585, Line 5, Col 27, Line 5, Col 31, "If a multicase union type is a struct, then all fields with the same name must be of the same type. This rule applies also to the generated 'Item' name in case of unnamed fields."); + (Error 3585, Line 6, Col 12, Line 6, Col 18, "If a multicase union type is a struct, then all fields with the same name must be of the same type. This rule applies also to the generated 'Item' name in case of unnamed fields.") ] [] diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionTypes.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionTypes.fs index 4611fef33ea..2f21fe3bcfc 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionTypes.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Types/UnionTypes/UnionTypes.fs @@ -272,7 +272,8 @@ module UnionTypes = |> verifyCompile |> shouldFail |> withDiagnostics [ - (Error 3176, Line 7, Col 16, Line 7, Col 21, "Named field 'Item2' conflicts with autogenerated name for anonymous field.") + (Error 3176, Line 7, Col 16, Line 7, Col 21, "Named field 'Item2' conflicts with autogenerated name for anonymous field."); + (Error 3176, Line 10, Col 16, Line 10, Col 17, "Named field 'A' is used more than once."); (Error 3176, Line 10, Col 26, Line 10, Col 27, "Named field 'A' is used more than once.") ] @@ -750,3 +751,59 @@ type MyId = (Error 23, Line 7, Col 17, Line 7, Col 20, "The member 'IdA' can not be defined because the name 'IdA' clashes with the union case 'IdA' in this type or module") (Error 23, Line 17, Col 17, Line 17, Col 20, "The member 'IdC' can not be defined because the name 'IdC' clashes with the union case 'IdC' in this type or module") ] + + + [] + let ``Union field appears multiple times in union declaration`` () = + Fsx """ +type X = + | A of a: int * a: int + """ + |> typecheck + |> shouldFail + |> withDiagnostics [ + (Error 3176, Line 3, Col 12, Line 3, Col 13, "Named field 'a' is used more than once.") + ] + + [] + let ``Union field appears multiple times in union declaration 2`` () = + Fsx """ +type X = + | A of a: int * a: int + | B of a: int * a: int + """ + |> typecheck + |> shouldFail + |> withDiagnostics [ + (Error 3176, Line 3, Col 12, Line 3, Col 13, "Named field 'a' is used more than once.") + (Error 3176, Line 4, Col 12, Line 4, Col 13, "Named field 'a' is used more than once.") + ] + + [] + let ``Union field appears multiple times in union declaration 3`` () = + Fsx """ +type X = + | A of a: int * a: int * a: int + """ + |> typecheck + |> shouldFail + |> withDiagnostics [ + (Error 3176, Line 3, Col 12, Line 3, Col 13, "Named field 'a' is used more than once.") + (Error 3176, Line 3, Col 21, Line 3, Col 22, "Named field 'a' is used more than once.") + ] + + [] + let ``Union field appears multiple times in union declaration 4`` () = + Fsx """ +type X = + | A of a: int * a: int +let x = A (1, 2) +match x with +| A(a = 1) -> () +| _ -> () + """ + |> typecheck + |> shouldFail + |> withDiagnostics [ + (Error 3176, Line 3, Col 12, Line 3, Col 13, "Named field 'a' is used more than once.") + ]