From c09b303a4aeae3e846c2181a9896e7290d28aedd Mon Sep 17 00:00:00 2001 From: Gauthier Segay Date: Sat, 22 Jul 2023 17:35:31 +0200 Subject: [PATCH 1/4] testing waters for reporting unnamed fields in an FCS helper function --- src/Compiler/Service/ServiceAnalysis.fs | 41 +++++++++++++++++++ src/Compiler/Service/ServiceAnalysis.fsi | 4 ++ tests/service/ProjectAnalysisTests.fs | 36 ++++++++++++++++ .../unammed.members.diagnostics.bsl | 3 ++ .../unammed.members.diagnostics.fsx | 24 +++++++++++ 5 files changed, 108 insertions(+) create mode 100644 tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl create mode 100644 tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 38af8f7156c..6b4867b7543 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -451,3 +451,44 @@ module UnusedDeclarations = let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile isScriptFile return unusedRanges } + +module public Naming = + [] + let rec checkDeclaration declaration = + match declaration with + | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue _ -> Seq.empty + | FSharpImplementationFileDeclaration.InitAction _ -> Seq.empty + | FSharpImplementationFileDeclaration.Entity(entity, _declarations) -> + seq { + for case in entity.UnionCases do + // lenient on non public members + if not case.Accessibility.IsPublic then () else + // lenient on single field cases + if case.Fields.Count <= 1 then () else + for field in case.Fields do + if field.IsNameGenerated || isNull field.Name || System.String.Empty = field.Name then + field.DeclarationLocation + match field.ImplementationLocation with + | None -> () + | Some range -> range + match field.SignatureLocation with + | None -> () + | Some range -> range + for declaration in _declarations do + yield! checkDeclaration declaration + } + let getUnnamedDiscriminatedUnionAndExceptionFields (checkFileResults: FSharpCheckFileResults, isScriptFile: bool) = + async { + // be lenient in case of script + if isScriptFile then return Seq.empty else + match checkFileResults.ImplementationFile with + | None -> return Seq.empty + | Some checkResults -> + return (seq { + for d in checkResults.Declarations do + yield! checkDeclaration d + } + |> Seq.distinct + //|> Seq.sort // not comparable + ) + } diff --git a/src/Compiler/Service/ServiceAnalysis.fsi b/src/Compiler/Service/ServiceAnalysis.fsi index 672cf088759..d20b8ab0f78 100644 --- a/src/Compiler/Service/ServiceAnalysis.fsi +++ b/src/Compiler/Service/ServiceAnalysis.fsi @@ -31,3 +31,7 @@ module public UnusedDeclarations = /// Get all unused declarations in a file val getUnusedDeclarations: checkFileResults: FSharpCheckFileResults * isScriptFile: bool -> Async> + + +module public Naming = + val getUnnamedDiscriminatedUnionAndExceptionFields : checkFileResults: FSharpCheckFileResults * isScriptFile : bool -> Async> \ No newline at end of file diff --git a/tests/service/ProjectAnalysisTests.fs b/tests/service/ProjectAnalysisTests.fs index f7cb93317cc..e6c26806450 100644 --- a/tests/service/ProjectAnalysisTests.fs +++ b/tests/service/ProjectAnalysisTests.fs @@ -5765,3 +5765,39 @@ let ``References from #r nuget are included in script project options`` () = |> Seq.distinct printfn "%s" (assemblyNames |> String.concat "\n") assemblyNames |> should contain "Dapper.dll" + + +[] +let ``detects unnamed fields in DU and exceptions`` () = + let file = __SOURCE_DIRECTORY__ + "/data/analysis/unammed-fields/unammed.members.diagnostics.fsx" + let baseline = System.IO.Path.ChangeExtension(file, ".bsl") + let input = + file + |> System.IO.File.ReadAllText + |> SourceText.ofString + let checker = FSharpChecker.Create(keepAssemblyContents=true) + + let projOptions, diagnostics = + checker.GetProjectOptionsFromScript(file, input) + |> Async.RunSynchronously + + let parseResults, checkFileAnswer = + checker.ParseAndCheckFileInProject(file, 0, input, projOptions) + |> Async.RunSynchronously + + match checkFileAnswer with + | FSharpCheckFileAnswer.Aborted -> failwith $"unexpected check file results for {file}" + | FSharpCheckFileAnswer.Succeeded results -> + let warns = + Naming.getUnnamedDiscriminatedUnionAndExceptionFields(results, false) + |> Async.RunSynchronously + |> Seq.toArray + |> Seq.map string + |> String.concat "\n" + let expected = System.IO.File.ReadAllText baseline + if expected <> warns then + let postmortem = System.IO.Path.ChangeExtension(file, ".err") + System.IO.File.WriteAllText(postmortem, warns) + printfn $"expected:\n{expected}\ngot:\n{warns}" + printfn $"please compare\n{baseline}\nwith\n{postmortem}" + \ No newline at end of file diff --git a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl new file mode 100644 index 00000000000..5d9abcb8198 --- /dev/null +++ b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl @@ -0,0 +1,3 @@ +(4,19--4,22) +(10,13--10,16) +(20,20--20,23) \ No newline at end of file diff --git a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx new file mode 100644 index 00000000000..4280c4221f7 --- /dev/null +++ b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx @@ -0,0 +1,24 @@ +type TopLevelDU = +| TopLevelCase1 of int +| TopLevelCase2 of ok: int * okok: string +| TopLevelCase3 of int * noOkCase: string + +module Outer = + type DU = + | Case1 of int + | Case2 of ok: int * okok: string + | Case3 of int * noOkCase: string + + exception Exception1 of int + exception Exception2 of ok: int * okok: string + exception Exception3 of int * noOkCase: string + + module Inner = + type InnerDU = + | InnerCase1 of int + | InnerCase2 of ok: int * okok: string + | InnerCase3 of int * noOkCase: string + + exception InnerException1 of int + exception InnerException2 of ok: int * okok: string + exception InnerException3 of int * noOkCase: string \ No newline at end of file From a6a1c1ef8aef8e9c48e1e289d1408ab03066b85b Mon Sep 17 00:00:00 2001 From: Gauthier Segay Date: Sat, 22 Jul 2023 17:41:40 +0200 Subject: [PATCH 2/4] assertion in case of failure --- tests/service/ProjectAnalysisTests.fs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/service/ProjectAnalysisTests.fs b/tests/service/ProjectAnalysisTests.fs index e6c26806450..6f65ad771db 100644 --- a/tests/service/ProjectAnalysisTests.fs +++ b/tests/service/ProjectAnalysisTests.fs @@ -5798,6 +5798,8 @@ let ``detects unnamed fields in DU and exceptions`` () = if expected <> warns then let postmortem = System.IO.Path.ChangeExtension(file, ".err") System.IO.File.WriteAllText(postmortem, warns) - printfn $"expected:\n{expected}\ngot:\n{warns}" - printfn $"please compare\n{baseline}\nwith\n{postmortem}" + let message = + $"expected:\n{expected}\ngot:\n{warns}\n" + + $"please compare\n{baseline}\nwith\n{postmortem}" + failwith message \ No newline at end of file From e18e30e0ad788a3d67f29916ab7c9e9c796772e5 Mon Sep 17 00:00:00 2001 From: Gauthier Segay Date: Sat, 22 Jul 2023 17:49:35 +0200 Subject: [PATCH 3/4] adding private du and exception --- .../analysis/unammed-fields/unammed.members.diagnostics.fsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx index 4280c4221f7..35b685cdbb1 100644 --- a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx +++ b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx @@ -21,4 +21,7 @@ module Outer = exception InnerException1 of int exception InnerException2 of ok: int * okok: string - exception InnerException3 of int * noOkCase: string \ No newline at end of file + exception InnerException3 of int * noOkCase: string + + type PrivateDU = private PrivateCase1 of int * string + exception private PrivateException of int * string \ No newline at end of file From fdd2d1a26f2f3b6fcf335e0a24393ffbcf3c03c8 Mon Sep 17 00:00:00 2001 From: Gauthier Segay Date: Tue, 25 Jul 2023 22:34:32 +0200 Subject: [PATCH 4/4] fixing typo, handling exception fields, define options for where to check (just as internal implementation detail for now) --- src/Compiler/Service/ServiceAnalysis.fs | 66 ++++++++++++++----- tests/service/ProjectAnalysisTests.fs | 2 +- .../unammed.members.diagnostics.bsl | 3 - .../unnamed.members.diagnostics.bsl | 5 ++ ...cs.fsx => unnamed.members.diagnostics.fsx} | 0 5 files changed, 54 insertions(+), 22 deletions(-) delete mode 100644 tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl create mode 100644 tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.bsl rename tests/service/data/analysis/unammed-fields/{unammed.members.diagnostics.fsx => unnamed.members.diagnostics.fsx} (100%) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 6b4867b7543..fa728886d12 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -453,32 +453,62 @@ module UnusedDeclarations = } module public Naming = + + type UnnamedFieldsLocations = + | InDiscriminatedUnionCases + | InExceptions + + let defaultOptions = Set.ofArray [|InDiscriminatedUnionCases; InExceptions|] + + let fieldNotOk (field: FSharpField) = + field.IsNameGenerated + || isNull field.Name + || System.String.Empty = field.Name + [] - let rec checkDeclaration declaration = + let rec checkDeclaration (options: Set) declaration = match declaration with | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue _ -> Seq.empty | FSharpImplementationFileDeclaration.InitAction _ -> Seq.empty | FSharpImplementationFileDeclaration.Entity(entity, _declarations) -> seq { - for case in entity.UnionCases do - // lenient on non public members - if not case.Accessibility.IsPublic then () else - // lenient on single field cases - if case.Fields.Count <= 1 then () else - for field in case.Fields do - if field.IsNameGenerated || isNull field.Name || System.String.Empty = field.Name then - field.DeclarationLocation - match field.ImplementationLocation with - | None -> () - | Some range -> range - match field.SignatureLocation with - | None -> () - | Some range -> range + if options.Contains InDiscriminatedUnionCases then + for case in entity.UnionCases do + // lenient on non public members + if not case.Accessibility.IsPublic then () else + // lenient on single field cases + if case.Fields.Count <= 1 then () else + for field in case.Fields do + if fieldNotOk field then + field.DeclarationLocation + match field.ImplementationLocation with + | None -> () + | Some range -> range + match field.SignatureLocation with + | None -> () + | Some range -> range + if options.Contains InExceptions then + if entity.IsFSharpExceptionDeclaration then + // lenient on non public members + if not entity.Accessibility.IsPublic then () else + // lenient on single field cases + if entity.FSharpFields.Count <= 1 then () else + for field in entity.FSharpFields do + if fieldNotOk field then + field.DeclarationLocation + match field.ImplementationLocation with + | None -> () + | Some range -> range + match field.SignatureLocation with + | None -> () + | Some range -> range for declaration in _declarations do - yield! checkDeclaration declaration + yield! checkDeclaration options declaration } + let getUnnamedDiscriminatedUnionAndExceptionFields (checkFileResults: FSharpCheckFileResults, isScriptFile: bool) = async { + let options = defaultOptions // be lenient in case of script if isScriptFile then return Seq.empty else match checkFileResults.ImplementationFile with @@ -486,9 +516,9 @@ module public Naming = | Some checkResults -> return (seq { for d in checkResults.Declarations do - yield! checkDeclaration d + yield! checkDeclaration options d } - |> Seq.distinct + |> Seq.distinct // not comparable but distinct works :) //|> Seq.sort // not comparable ) } diff --git a/tests/service/ProjectAnalysisTests.fs b/tests/service/ProjectAnalysisTests.fs index 6f65ad771db..05c5e74be25 100644 --- a/tests/service/ProjectAnalysisTests.fs +++ b/tests/service/ProjectAnalysisTests.fs @@ -5769,7 +5769,7 @@ let ``References from #r nuget are included in script project options`` () = [] let ``detects unnamed fields in DU and exceptions`` () = - let file = __SOURCE_DIRECTORY__ + "/data/analysis/unammed-fields/unammed.members.diagnostics.fsx" + let file = __SOURCE_DIRECTORY__ + "/data/analysis/unammed-fields/unnamed.members.diagnostics.fsx" let baseline = System.IO.Path.ChangeExtension(file, ".bsl") let input = file diff --git a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl b/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl deleted file mode 100644 index 5d9abcb8198..00000000000 --- a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.bsl +++ /dev/null @@ -1,3 +0,0 @@ -(4,19--4,22) -(10,13--10,16) -(20,20--20,23) \ No newline at end of file diff --git a/tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.bsl b/tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.bsl new file mode 100644 index 00000000000..ae5471e5eef --- /dev/null +++ b/tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.bsl @@ -0,0 +1,5 @@ +(4,19--4,22) +(10,13--10,16) +(14,26--14,29) +(20,20--20,23) +(24,33--24,36) \ No newline at end of file diff --git a/tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx b/tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.fsx similarity index 100% rename from tests/service/data/analysis/unammed-fields/unammed.members.diagnostics.fsx rename to tests/service/data/analysis/unammed-fields/unnamed.members.diagnostics.fsx