From 4fca3be018ddb6e4fa53e4f4c5ac4e9998ad6dd1 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Fri, 16 Aug 2024 19:21:16 +0200 Subject: [PATCH 01/11] Added test --- .../TypeChecks/Graph/CompilationTests.fs | 14 ++++---- .../TypeChecks/Graph/Scenarios.fs | 35 ++++++++++++++++--- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs index 829810f3eea..14244f46ad8 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs @@ -2,7 +2,7 @@ open FSharp.Test open FSharp.Test.Compiler -open NUnit.Framework +open Xunit; open Scenarios [] @@ -44,12 +44,14 @@ let compileAValidScenario (scenario: Scenario) (method: Method) = |> shouldSucceed |> ignore -let scenarios = codebases +let scenarios = codebases |> List.map (fun c -> [| box c |]) |> Array.ofList -[] -let ``Compile a valid scenario using graph-based type-checking`` (scenario: Scenario) = +[] +[] +let ``Compile a valid scenario using graph-based type-checking`` (scenario) = compileAValidScenario scenario Method.Graph -[] -let ``Compile a valid scenario using sequential type-checking`` (scenario: Scenario) = +[] +[] +let ``Compile a valid scenario using sequential type-checking`` (scenario) = compileAValidScenario scenario Method.Sequential diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index fe0c270edc5..19e3cdb3a40 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -256,7 +256,7 @@ module Y.C // This open statement does not do anything. // It can safely be removed, but because of its presence we need to link it to something that exposes the namespace X. -// We try and pick the file with the lowest index +// We try and pick the file with the lowest index open X let c = 0 @@ -572,7 +572,7 @@ type Bar() = static member Foo () : unit = failwith "" -let Foo () : unit = +let Foo () : unit = Bar.Foo () """ (set [| 0 |]) @@ -792,7 +792,7 @@ open My.Great.Namespace type Foo = class end """ (set [| 0 |]) - + sourceFile "Program" """ @@ -897,7 +897,7 @@ do """ (set [| 0 |]) ] - + scenario "parentheses around module name in nameof expression" [ @@ -1042,4 +1042,31 @@ type Bar(foo: MyRootNamespace.A.Foo, s: string) = class end """ (set [| 0 |]) ] + scenario + "Library with using global namespace" + [ + sourceFile + "Library.fs" + """ +namespace Lib +module File1 = + let mutable discState = System.DateTime.Now + +module File2 = + [] + type DiscState(rep : int) = + member this.Rep = rep + + let mutable discState = DiscState(0) + """ + Set.empty + sourceFile + "App.fs" + """ +module X +let v = global.Lib.File1.discState.Second +let v2 = global.Lib.File2.discState.Rep + """ + Set.empty + ] ] \ No newline at end of file From ba9153a91ffa7a6cb0c98fff50abf09f70c1c932 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 12:43:04 +0200 Subject: [PATCH 02/11] Skip the global namespace when transforming longident to path --- .../Driver/GraphChecking/DependencyResolution.fs | 2 +- .../Driver/GraphChecking/FileContentMapping.fs | 10 +++++++--- src/Compiler/Driver/GraphChecking/Types.fs | 1 + src/Compiler/Driver/GraphChecking/Types.fsi | 1 + .../TypeChecks/Graph/QueryTrieTests.fs | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index 11ba984ca51..1425ec0f9ed 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -117,7 +117,7 @@ let rec processStateEntry (trie: TrieNode) (state: FileContentQueryState) (entry FoundDependencies = foundDependencies } - | ModuleName name -> + | FileContentEntry.ModuleName name -> // We need to check if the module name is a hit in the Trie. let state' = let queryResult = queryTrie trie [ name ] diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 13ee0c312f9..ef0a6ca296d 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -8,10 +8,14 @@ type Continuations = ((FileContentEntry list -> FileContentEntry list) -> FileCo /// Collect a list of 'U from option 'T via a mapping function. let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.collect mapping (Option.toList t) -let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = +let rec longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = match skipLast, longId with - | true, _ :: _ -> List.take (longId.Length - 1) longId - | _ -> longId + | true, h :: t when h.idText = "`global`" -> + List.take (t.Length - 1) t + | true, _ :: _ -> + List.take (longId.Length - 1) longId + | _ -> + longId |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index c667a573f69..7443df58204 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -61,6 +61,7 @@ type internal TrieNode = /// A significant construct found in the syntax tree of a file. /// This construct needs to be processed in order to deduce potential links to other files in the project. +[] type internal FileContentEntry = /// Any toplevel namespace a file might have. /// In case a file has `module X.Y.Z`, then `X.Y` is considered to be the toplevel namespace diff --git a/src/Compiler/Driver/GraphChecking/Types.fsi b/src/Compiler/Driver/GraphChecking/Types.fsi index 096719b6be7..5f075a1bad7 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fsi +++ b/src/Compiler/Driver/GraphChecking/Types.fsi @@ -55,6 +55,7 @@ type internal TrieNode = /// A significant construct found in the syntax tree of a file. /// This construct needs to be processed in order to deduce potential links to other files in the project. +[] type internal FileContentEntry = /// Any toplevel namespace a file might have. /// In case a file has `module X.Y.Z`, then `X.Y` is considered to be the toplevel namespace diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs index e6fd44a3505..8a3b903e16c 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs @@ -36,7 +36,7 @@ let private nestedModule name content = let private prefIdent (lid: string) = let parts = lid.Split(".") - Array.take (parts.Length - 1) parts |> List.ofArray |> PrefixedIdentifier + Array.take (parts.Length - 1) parts |> List.ofArray |> FileContentEntry.PrefixedIdentifier // Some hardcoded files that reflect the file content of the first files in the Fantomas.Core project. // See https://github.com/fsprojects/fantomas/tree/0938a3daabec80a22d2e17f82aba38456bb793df/src/Fantomas.Core From 4d32db631d74df56940520222990c2e359e9302f Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 12:46:46 +0200 Subject: [PATCH 03/11] Fixed the case --- .../Driver/GraphChecking/FileContentMapping.fs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index ef0a6ca296d..3d69f422b6e 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -8,14 +8,11 @@ type Continuations = ((FileContentEntry list -> FileContentEntry list) -> FileCo /// Collect a list of 'U from option 'T via a mapping function. let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.collect mapping (Option.toList t) -let rec longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = +let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = match skipLast, longId with - | true, h :: t when h.idText = "`global`" -> - List.take (t.Length - 1) t - | true, _ :: _ -> - List.take (longId.Length - 1) longId - | _ -> - longId + | true, h :: t when h.idText = "`global`" && t.Length > 1 -> List.take (t.Length - 1) t + | true, _ :: _ -> List.take (longId.Length - 1) longId + | _ -> longId |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = From 9391a789480fb309c0a87865dd8a2a1d9e7fb0e5 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 12:48:41 +0200 Subject: [PATCH 04/11] Fixed the case --- src/Compiler/Driver/GraphChecking/FileContentMapping.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 3d69f422b6e..dfc6900b921 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -10,9 +10,9 @@ let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.c let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = match skipLast, longId with - | true, h :: t when h.idText = "`global`" && t.Length > 1 -> List.take (t.Length - 1) t | true, _ :: _ -> List.take (longId.Length - 1) longId | _ -> longId + |> List.filter (fun ident -> ident.idText <> "`global`") |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = From 9c186d24f6ffee6540690876ff19990bde881fbd Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 13:04:01 +0200 Subject: [PATCH 05/11] Make it recursive call --- .../Driver/GraphChecking/FileContentMapping.fs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index dfc6900b921..1e204039a55 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -9,11 +9,18 @@ type Continuations = ((FileContentEntry list -> FileContentEntry list) -> FileCo let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.collect mapping (Option.toList t) let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = - match skipLast, longId with - | true, _ :: _ -> List.take (longId.Length - 1) longId - | _ -> longId - |> List.filter (fun ident -> ident.idText <> "`global`") - |> List.map (fun ident -> ident.idText) + + let rec loop skipLast (longId: LongIdent) : LongIdent = + match skipLast, longId with + | true, h :: t when h.idText = "`global`" -> + loop true t + | true, _ :: _ -> + List.take (longId.Length - 1) longId + | _ -> + longId + + let longId = loop skipLast longId + longId |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = longIdentToPath skipLast synLongIdent.LongIdent From f7f8e4f99a98e710ca6597cba76162fc59925794 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 13:05:06 +0200 Subject: [PATCH 06/11] Format --- src/Compiler/Driver/GraphChecking/FileContentMapping.fs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 1e204039a55..9a3dbbfde1a 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -12,12 +12,9 @@ let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = let rec loop skipLast (longId: LongIdent) : LongIdent = match skipLast, longId with - | true, h :: t when h.idText = "`global`" -> - loop true t - | true, _ :: _ -> - List.take (longId.Length - 1) longId - | _ -> - longId + | true, h :: t when h.idText = "`global`" -> loop true t + | true, _ :: _ -> List.take (longId.Length - 1) longId + | _ -> longId let longId = loop skipLast longId longId |> List.map (fun ident -> ident.idText) From 2333d39096c30b2662dcb2b8cbf9a88cde9c83af Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 13:43:40 +0200 Subject: [PATCH 07/11] Fixed case --- src/Compiler/Driver/GraphChecking/FileContentMapping.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 9a3dbbfde1a..0bea91cef26 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -12,7 +12,7 @@ let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = let rec loop skipLast (longId: LongIdent) : LongIdent = match skipLast, longId with - | true, h :: t when h.idText = "`global`" -> loop true t + | _, h :: t when h.idText = "`global`" -> loop true t | true, _ :: _ -> List.take (longId.Length - 1) longId | _ -> longId From b7c94e2ef19378cc08d7e5279787f89d3938d65a Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 13:46:27 +0200 Subject: [PATCH 08/11] 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 3d4288458c9..36abbb99f5d 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.100.md @@ -9,6 +9,7 @@ * Compiler fails to recognise namespace in FQN with enabled GraphBasedChecking. ([Issue #17508](https://github.com/dotnet/fsharp/issues/17508), [PR #17510](https://github.com/dotnet/fsharp/pull/17510)) * Fix missing message for type error (FS0001). ([Issue #17373](https://github.com/dotnet/fsharp/issues/17373), [PR #17516](https://github.com/dotnet/fsharp/pull/17516)) * MethodAccessException on equality comparison of a type private to module. ([Issue #17541](https://github.com/dotnet/fsharp/issues/17541), [PR #17548](https://github.com/dotnet/fsharp/pull/17548)) +* Fixed checking failure when `global` namespace is involved with enabled GraphBasedChecking ([PR #17553](https://github.com/dotnet/fsharp/pull/17553)) ### Added From a3b78cc165c26b3634c391e5786003bd6921303d Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 14:28:33 +0200 Subject: [PATCH 09/11] Update test --- .../TypeChecks/Graph/Scenarios.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index 19e3cdb3a40..607206308f0 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -1067,6 +1067,6 @@ module X let v = global.Lib.File1.discState.Second let v2 = global.Lib.File2.discState.Rep """ - Set.empty + (set [| 0 |]) ] ] \ No newline at end of file From c46ae736dc731d6356d7f129a4f7b3bc9887a27a Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 14:55:27 +0200 Subject: [PATCH 10/11] Update logic --- .../Driver/GraphChecking/FileContentMapping.fs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 0bea91cef26..c1f6ce8ddb4 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -10,14 +10,16 @@ let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.c let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = - let rec loop skipLast (longId: LongIdent) : LongIdent = - match skipLast, longId with - | _, h :: t when h.idText = "`global`" -> loop true t - | true, _ :: _ -> List.take (longId.Length - 1) longId + // We always skip the "special" `global` identifier. + let longId = + match longId with + | h :: t when h.idText = "`global`" -> t | _ -> longId - let longId = loop skipLast longId - longId |> List.map (fun ident -> ident.idText) + match skipLast, longId with + | true, _ :: _ -> List.take (longId.Length - 1) longId + | _ -> longId + |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = longIdentToPath skipLast synLongIdent.LongIdent From 03e32c8115a9927f78eb9996108f2d27e9be5818 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Mon, 19 Aug 2024 14:58:56 +0200 Subject: [PATCH 11/11] Added one more test --- .../TypeChecks/Graph/Scenarios.fs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index 607206308f0..6ab98da50eb 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -1069,4 +1069,25 @@ let v2 = global.Lib.File2.discState.Rep """ (set [| 0 |]) ] + scenario + "Library with using global namespace as module alias" + [ + sourceFile + "Library.fs" + """ +namespace Z + +module N = + let mutable discState = System.DateTime.Now + """ + Set.empty + sourceFile + "App.fs" + """ +module X + +module Y = global.Z.N + """ + (set [| 0 |]) + ] ] \ No newline at end of file