From 0878403dcc39ba30c43ce208b1770cbc6b8de580 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Mon, 12 Aug 2024 11:53:18 +0100 Subject: [PATCH 1/3] Better error reporting for unions duplicated fields --- src/Compiler/Checking/CheckDeclarations.fs | 18 ++++-- .../UnionTypes/E_UnionFieldConflictingName.fs | 2 +- .../Types/UnionTypes/UnionStructTypes.fs | 9 ++- .../Types/UnionTypes/UnionTypes.fs | 59 ++++++++++++++++++- 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 9ddff1e7585..00a60c73dbc 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/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.") + ] From d9367424008208b06abf1e740f0dcfdb76ddf1d6 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Mon, 12 Aug 2024 13:30:54 +0100 Subject: [PATCH 2/3] update tests --- .../ExceptionDefinitions/E_ExnFieldConflictingName.fs | 4 +++- .../ExceptionDefinitions/ExceptionDefinitions.fs | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) 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 From 0cd3717e25cb2b420b34b33f7c81e5ec6b8ace99 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Mon, 12 Aug 2024 16:29:31 +0100 Subject: [PATCH 3/3] release notes --- docs/release-notes/.FSharp.Compiler.Service/9.0.100.md | 1 + 1 file changed, 1 insertion(+) 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 e2562e5a4c0..5c68533014d 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md @@ -26,4 +26,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