Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reporting unnamed fields in an FCS helper function #15668

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions src/Compiler/Service/ServiceAnalysis.fs
Original file line number Diff line number Diff line change
Expand Up @@ -451,3 +451,74 @@ module UnusedDeclarations =
let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile isScriptFile
return unusedRanges
}

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

[<TailCall>]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a good idea until we move to a new released FSharp.Core which has this attribute already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks & understood, fresh paint stuff in FSharp.Core needs at least one official release, is it how to look at it?

I used it because I was unsure my code was TailCall or StackOverflow (due to yield!), I don't know how deeply nested code is going to be passed, but I'm hopeful there is headroom in case I missed something making it non TCR-Optimized.

Copy link
Member

@vzarytovskii vzarytovskii Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks & understood, fresh paint stuff in FSharp.Core needs at least one official release, is it how to look at it?

Yeah, pretty much. Because some "flavours" of FCS are built with FSharp.Core from nuget. I know @auduchinok uses it.

Copy link
Contributor Author

@smoothdeveloper smoothdeveloper Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there CI empowered people that would put a hard check on this?

I don't know how that would play, beside having a "public surface area" diff and analysis to see which members are getting exposed in the FCS codebase.

Maybe there are other ways?

Just speculating this could be a removed burden for code reviewers (if we find a robust way of doing).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar team!

let rec checkDeclaration (options: Set<UnnamedFieldsLocations>) declaration =
match declaration with
| FSharpImplementationFileDeclaration.MemberOrFunctionOrValue _ -> Seq.empty
| FSharpImplementationFileDeclaration.InitAction _ -> Seq.empty
| FSharpImplementationFileDeclaration.Entity(entity, _declarations) ->
seq {
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 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
| None -> return Seq.empty
| Some checkResults ->
return (seq {
for d in checkResults.Declarations do
yield! checkDeclaration options d
}
|> Seq.distinct // not comparable but distinct works :)
//|> Seq.sort // not comparable
)
}
4 changes: 4 additions & 0 deletions src/Compiler/Service/ServiceAnalysis.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,7 @@ module public UnusedDeclarations =

/// Get all unused declarations in a file
val getUnusedDeclarations: checkFileResults: FSharpCheckFileResults * isScriptFile: bool -> Async<seq<range>>


module public Naming =
val getUnnamedDiscriminatedUnionAndExceptionFields : checkFileResults: FSharpCheckFileResults * isScriptFile : bool -> Async<seq<range>>
38 changes: 38 additions & 0 deletions tests/service/ProjectAnalysisTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -5765,3 +5765,41 @@ let ``References from #r nuget are included in script project options`` () =
|> Seq.distinct
printfn "%s" (assemblyNames |> String.concat "\n")
assemblyNames |> should contain "Dapper.dll"


[<Test>]
let ``detects unnamed fields in DU and exceptions`` () =
let file = __SOURCE_DIRECTORY__ + "/data/analysis/unammed-fields/unnamed.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)
let message =
$"expected:\n{expected}\ngot:\n{warns}\n"
+ $"please compare\n{baseline}\nwith\n{postmortem}"
failwith message

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
(4,19--4,22)
(10,13--10,16)
(14,26--14,29)
(20,20--20,23)
(24,33--24,36)
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
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

type PrivateDU = private PrivateCase1 of int * string
exception private PrivateException of int * string