`Symbol.IsPrivateToFile` when symbol is not in signature file (#14274)

* Try fix Symbol.PrivateToFile

* fantomas

* More tests

src/Compiler/Service/FSharpCheckerResults.fs

@@ -261,11 +261,25 @@ type FSharpSymbolUse(denv: DisplayEnv, symbol: FSharpSymbol, inst: TyparInstanti
     member this.IsPrivateToFile =
         let isPrivate =
             match this.Symbol with
-            | :? FSharpMemberOrFunctionOrValue as m -> not m.IsModuleValueOrMember || m.Accessibility.IsPrivate
+            | :? FSharpMemberOrFunctionOrValue as m ->
+                let fileSignatureLocation =
+                    m.DeclaringEntity |> Option.bind (fun e -> e.SignatureLocation)
+
+                let fileDeclarationLocation =
+                    m.DeclaringEntity |> Option.map (fun e -> e.DeclarationLocation)
+
+                let fileHasSignatureFile = fileSignatureLocation <> fileDeclarationLocation
+
+                let symbolIsNotInSignatureFile = m.SignatureLocation = Some m.DeclarationLocation
+
+                fileHasSignatureFile && symbolIsNotInSignatureFile
+                || not m.IsModuleValueOrMember
+                || m.Accessibility.IsPrivate
             | :? FSharpEntity as m -> m.Accessibility.IsPrivate
             | :? FSharpGenericParameter -> true
             | :? FSharpUnionCase as m -> m.Accessibility.IsPrivate
             | :? FSharpField as m -> m.Accessibility.IsPrivate
+            | :? FSharpActivePatternCase as m -> m.Accessibility.IsPrivate
             | _ -> false
 
         let declarationLocation =

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

@@ -207,12 +207,13 @@
     <Compile Include="Signatures\ArrayTests.fs" />
     <Compile Include="Signatures\TypeTests.fs" />
     <Compile Include="FSharpChecker\CommonWorkflows.fs" />
+    <Compile Include="FSharpChecker\SymbolUse.fs" />
+    <None Include="**\*.cs;**\*.fs;**\*.fsx;**\*.fsi" Exclude="@(Compile)">
+       <Link>%(RelativeDir)\TestSource\%(Filename)%(Extension)</Link>
+    </None>
   </ItemGroup>
 
   <ItemGroup>
-    <None Include="**\*.cs;**\*.fs;**\*.fsx;**\*.fsi" Exclude="@(Compile)">
-      <Link>%(RelativeDir)\TestSource\%(Filename)%(Extension)</Link>
-    </None>
     <None Include="**\*.bsl">
       <Link>%(RelativeDir)\BaseLine\%(Filename)%(Extension)</Link>
     </None>

tests/FSharp.Compiler.ComponentTests/FSharpChecker/CommonWorkflows.fs

@@ -7,26 +7,16 @@ open Xunit
 
 open FSharp.Test.ProjectGeneration
 
-let projectDir = "test-projects"
-
 let makeTestProject () =
-    let name = $"testProject{Guid.NewGuid().ToString()[..7]}"
-    let dir = Path.GetFullPath projectDir
-    {
-        Name = name
-        ProjectDir = dir ++ name
-        SourceFiles = [
-            sourceFile "First" []
-            sourceFile "Second" ["First"]
-            sourceFile "Third" ["First"]
-            { sourceFile "Last" ["Second"; "Third"] with EntryPoint = true }
-        ]
-        DependsOn = []
-    }
+    SyntheticProject.Create(
+        sourceFile "First" [],
+        sourceFile "Second" ["First"],
+        sourceFile "Third" ["First"],
+        { sourceFile "Last" ["Second"; "Third"] with EntryPoint = true })
 
 [<Fact>]
 let ``Edit file, check it, then check dependent file`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         updateFile "First" breakDependentFiles
         checkFile "First" expectSignatureChanged
         saveFile "First"
@@ -35,7 +25,7 @@ let ``Edit file, check it, then check dependent file`` () =
 
 [<Fact>]
 let ``Edit file, don't check it, check dependent file`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         updateFile "First" breakDependentFiles
         saveFile "First"
         checkFile "Second" expectErrors
@@ -43,7 +33,7 @@ let ``Edit file, don't check it, check dependent file`` () =
 
 [<Fact>]
 let ``Check transitive dependency`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         updateFile "First" breakDependentFiles
         saveFile "First"
         checkFile "Last" expectSignatureChanged
@@ -51,7 +41,7 @@ let ``Check transitive dependency`` () =
 
 [<Fact>]
 let ``Change multiple files at once`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         updateFile "First" (setPublicVersion 2)
         updateFile "Second" (setPublicVersion 2)
         updateFile "Third" (setPublicVersion 2)
@@ -62,7 +52,7 @@ let ``Change multiple files at once`` () =
 [<Fact>]
 let ``Files depend on signature file if present`` () =
     (makeTestProject()
-    |> updateFile "First" (fun f -> { f with HasSignatureFile = true })
+    |> updateFile "First" addSignatureFile
     |> projectWorkflow) {
         updateFile "First" breakDependentFiles
         saveFile "First"
@@ -71,7 +61,7 @@ let ``Files depend on signature file if present`` () =
 
 [<Fact>]
 let ``Adding a file`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         addFileAbove "Second" (sourceFile "New" [])
         updateFile "Second" (addDependency "New")
         saveAll
@@ -80,7 +70,7 @@ let ``Adding a file`` () =
 
 [<Fact>]
 let ``Removing a file`` () =
-    projectWorkflow (makeTestProject()) {
+    makeTestProject().Workflow {
         removeFile "Second"
         saveAll
         checkFile "Last" expectErrors
@@ -88,20 +78,13 @@ let ``Removing a file`` () =
 
 [<Fact>]
 let ``Changes in a referenced project`` () =
-    let name = $"library{Guid.NewGuid().ToString()[..7]}"
-    let dir = Path.GetFullPath projectDir
-    let library = {
-        Name = name
-        ProjectDir = dir ++ name
-        SourceFiles = [ sourceFile "Library" [] ]
-        DependsOn = []
-    }
+    let library = SyntheticProject.Create("library", sourceFile "Library" [])
 
     let project =
         { makeTestProject() with DependsOn = [library] }
         |> updateFile "First" (addDependency "Library")
 
-    projectWorkflow project {
+    project.Workflow {
         updateFile "Library" updatePublicSurface
         saveFile "Library"
         checkFile "Last" expectSignatureChanged

tests/FSharp.Compiler.ComponentTests/FSharpChecker/SymbolUse.fs

+module FSharp.Compiler.ComponentTests.FSharpChecker.SymbolUse
+
+open FSharp.Compiler.CodeAnalysis
+open Xunit
+open FSharp.Test.ProjectGeneration
+
+
+module IsPrivateToFile =
+
+    [<Fact>]
+    let ``Function definition in signature file`` () =
+        let project = SyntheticProject.Create(
+            sourceFile "First" [] |> addSignatureFile,
+            sourceFile "Second" ["First"])
+
+        project.Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(5, 6, "let f2 x = x + 1", ["f2"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                Assert.False(symbolUse.IsPrivateToFile))
+        }
+
+    [<Fact>]
+    let ``Function definition, no signature file`` () =
+        let project = SyntheticProject.Create(
+            sourceFile "First" [],
+            sourceFile "Second" ["First"])
+
+        project.Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(5, 6, "let f2 x = x + 1", ["f2"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                Assert.False(symbolUse.IsPrivateToFile))
+        }
+
+    [<Fact>]
+    let ``Function definition not in signature file`` () =
+        let projectName = "IsPrivateToFileTest1"
+        let signature = $"""
+module {projectName}.ModuleFirst
+type TFirstV_1<'a> = | TFirst of 'a
+val f: x: 'a -> TFirstV_1<'a>
+// no f2 here
+"""
+        let project = SyntheticProject.Create(projectName,
+            { sourceFile "First" [] with SignatureFile = Custom signature },
+            sourceFile "Second" ["First"])
+
+        project.Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(5, 6, "let f2 x = x + 1", ["f2"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                Assert.True(symbolUse.IsPrivateToFile))
+        }
+
+    [<Fact>]
+    let ``Function parameter, no signature file`` () =
+        SyntheticProject.Create(sourceFile "First" []).Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(5, 8, "let f2 x = x + 1", ["x"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                Assert.True(symbolUse.IsPrivateToFile))
+        }
+
+    /// This is a bug: https://github.com/dotnet/fsharp/issues/14277
+    [<Fact>]
+    let ``Function parameter, with signature file`` () =
+        SyntheticProject.Create(sourceFile "First" [] |> addSignatureFile).Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(5, 8, "let f2 x = x + 1", ["x"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                // This should be false, because it's also in the signature file
+                Assert.True(symbolUse.IsPrivateToFile))
+        }
+
+    [<Fact>]
+    let ``Private function, with signature file`` () =
+        SyntheticProject.Create(
+            { sourceFile "First" [] with ExtraSource = "let private f3 x = x + 1" } 
+            |> addSignatureFile).Workflow {
+            checkFile "First" (fun (typeCheckResult: FSharpCheckFileResults) ->
+                let symbolUse = typeCheckResult.GetSymbolUseAtLocation(6, 14, "let private f3 x = x + 1", ["f3"]) |> Option.defaultWith (fun () -> failwith "no symbol use found")
+                Assert.False(symbolUse.IsPrivateToFile))
+        }
\ No newline at end of file

tests/FSharp.Test.Utilities/ProjectGeneration.fs

@@ -23,11 +23,14 @@ open FSharp.Compiler.Text
 open Xunit
 
 
-let private projectRoot = __SOURCE_DIRECTORY__
+let private projectRoot = "test-projects"
 
 let private defaultFunctionName = "f"
 
 
+type SignatureFile = No | AutoGenerated | Custom of string
+
+
 type SyntheticSourceFile =
     {
         Id: string
@@ -37,29 +40,55 @@ type SyntheticSourceFile =
         DependsOn: string list
         /// Changing this makes dependent files' code invalid
         FunctionName: string
-        HasSignatureFile: bool
+        SignatureFile: SignatureFile
         HasErrors: bool
+        ExtraSource: string
         EntryPoint: bool
     }
 
     member this.FileName = $"File{this.Id}.fs"
     member this.SignatureFileName = $"{this.FileName}i"
 
+    member this.HasSignatureFile =
+        match this.SignatureFile with
+        | No -> false
+        | _ -> true
+
+
 let sourceFile fileId deps =
     { Id = fileId
       PublicVersion = 1
       InternalVersion = 1
       DependsOn = deps
       FunctionName = defaultFunctionName
-      HasSignatureFile = false
+      SignatureFile = No
       HasErrors = false
+      ExtraSource = ""
       EntryPoint = false }
 
 type SyntheticProject =
     { Name: string
       ProjectDir: string
       SourceFiles: SyntheticSourceFile list
-      DependsOn: SyntheticProject list }
+      DependsOn: SyntheticProject list
+      RecursiveNamespace: bool }
+
+    static member Create(?name: string) =
+        let name = defaultArg name $"TestProject_{Guid.NewGuid().ToString()[..7]}"
+        let dir = Path.GetFullPath projectRoot
+        {
+            Name = name
+            ProjectDir = dir ++ name
+            SourceFiles = []
+            DependsOn = []
+            RecursiveNamespace = false
+        }
+
+    static member Create([<ParamArray>] sourceFiles: SyntheticSourceFile[]) =
+        { SyntheticProject.Create() with SourceFiles = sourceFiles |> List.ofArray }
+
+    static member Create(name: string, [<ParamArray>] sourceFiles: SyntheticSourceFile[]) =
+        { SyntheticProject.Create(name) with SourceFiles = sourceFiles |> List.ofArray }
 
     member this.Find fileId =
         this.SourceFiles
@@ -120,7 +149,11 @@ module Internal =
 
     let renderSourceFile (project: SyntheticProject) (f: SyntheticSourceFile) =
         seq {
-            $"module %s{project.Name}.Module{f.Id}"
+            if project.RecursiveNamespace then
+                $"namespace rec {project.Name}"
+                $"module Module{f.Id}"
+            else
+                $"module %s{project.Name}.Module{f.Id}"
 
             for p in project.DependsOn do
                 $"open {p.Name}"
@@ -136,6 +169,8 @@ module Internal =
 
             $"let f2 x = x + {f.InternalVersion}"
 
+            f.ExtraSource
+
             if f.HasErrors then
                 "let wrong = 1 + 'a'"
 
@@ -239,6 +274,8 @@ module ProjectOperations =
     let addDependency fileId f : SyntheticSourceFile =
         { f with DependsOn = fileId :: f.DependsOn }
 
+    let addSignatureFile f = { f with SignatureFile = AutoGenerated }
+
     let checkFile fileId (project: SyntheticProject) (checker: FSharpChecker) =
         let file = project.Find fileId
         let contents = renderSourceFile project file
@@ -302,12 +339,17 @@ module ProjectOperations =
                 let file = p.SourceFiles[i]
                 writeFile p file
 
-                if file.HasSignatureFile && generateSignatureFiles then
+                let signatureFileName = p.ProjectDir ++ file.SignatureFileName
+
+                match file.SignatureFile with
+                | AutoGenerated when generateSignatureFiles ->
                     let project = { p with SourceFiles = p.SourceFiles[0..i] }
                     let! results = checkFile file.Id project checker
                     let signature = getSignature results
-                    let signatureFileName = p.ProjectDir ++ file.SignatureFileName
                     writeFileIfChanged signatureFileName signature
+                | Custom signature ->
+                    writeFileIfChanged signatureFileName signature
+                | _ -> ()
 
             writeFileIfChanged (p.ProjectDir ++ $"{p.Name}.fsproj") (renderFsProj p)
         }
@@ -400,6 +442,20 @@ type ProjectWorkflowBuilder(initialProject: SyntheticProject, ?checker: FSharpCh
             return { ctx with Signatures = ctx.Signatures.Add(fileId, newSignature) }
         }
 
+    /// Parse and type check given file and process the results using `processResults` function.
+    [<CustomOperation "checkFile">]
+    member this.CheckFile(workflow: Async<WorkflowContext>, fileId: string, processResults) =
+        async {
+            let! ctx = workflow
+            let! results = checkFile fileId ctx.Project checker
+            let typeCheckResults = getTypeCheckResult results
+
+            let newSignature = getSignature results
+
+            processResults typeCheckResults
+
+            return { ctx with Signatures = ctx.Signatures.Add(fileId, newSignature) }
+        }
     /// Save given file to disk.
     [<CustomOperation "saveFile">]
     member this.SaveFile(workflow: Async<WorkflowContext>, fileId: string) =
@@ -422,3 +478,9 @@ type ProjectWorkflowBuilder(initialProject: SyntheticProject, ?checker: FSharpCh
 /// Execute a set of operations on a given synthetic project.
 /// The project is saved to disk and type checked at the start.
 let projectWorkflow project = ProjectWorkflowBuilder project
+
+
+type SyntheticProject with
+    /// Execute a set of operations on this project.
+    /// The project is saved to disk and type checked at the start.
+    member this.Workflow = projectWorkflow this