Skip to content

Ensure script load closure is always executed for GetProjectSnapshotFromScript #16880

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Mar 18, 2024
Merged

Ensure script load closure is always executed for GetProjectSnapshotFromScript #16880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7c20f19
Ensure script load closure is always executed for GetProjectSnapshotF…
nojaf Mar 15, 2024
48dfe2c
assumeDotNetFramework is false for non Windows.
nojaf Mar 15, 2024
c286b44
Update Surface Area
nojaf Mar 15, 2024
468e8a3
Merge remote-tracking branch 'origin/fix-load-closure' into fix-load-…
nojaf Mar 15, 2024
a96eee6
Merge branch 'main' into fix-load-closure
nojaf Mar 18, 2024
6456b60
Merge branch 'main' into fix-load-closure
nojaf Mar 18, 2024
78b5424
Merge branch 'main' into fix-load-closure
nojaf Mar 18, 2024
edd54bf
Check for FF in test.
nojaf Mar 18, 2024
3454876
Merge branch 'main' into fix-load-closure
nojaf Mar 18, 2024
32af090
Address bad merge.
nojaf Mar 18, 2024
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
4 changes: 3 additions & 1 deletion src/Compiler/Service/BackgroundCompiler.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ type internal IBackgroundCompiler =
abstract GetProjectSnapshotFromScript:
fileName: string *
sourceText: ISourceTextNew *
documentSource: DocumentSource *
previewEnabled: bool option *
loadedTimeStamp: System.DateTime option *
otherFlags: string array option *
Expand Down Expand Up @@ -1627,6 +1628,7 @@ type internal BackgroundCompiler
(
fileName: string,
sourceText: ISourceTextNew,
documentSource: DocumentSource,
previewEnabled: bool option,
loadedTimeStamp: DateTime option,
otherFlags: string array option,
Expand All @@ -1653,7 +1655,7 @@ type internal BackgroundCompiler
userOpName
)

let! snapshot = FSharpProjectSnapshot.FromOptions(options, DocumentSource.FileSystem)
let! snapshot = FSharpProjectSnapshot.FromOptions(options, documentSource)
return snapshot, diagnostics
}

Expand Down
1 change: 1 addition & 0 deletions src/Compiler/Service/BackgroundCompiler.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ type internal IBackgroundCompiler =
abstract GetProjectSnapshotFromScript:
fileName: string *
sourceText: ISourceTextNew *
documentSource: DocumentSource *
previewEnabled: bool option *
loadedTimeStamp: System.DateTime option *
otherFlags: string array option *
Expand Down
193 changes: 133 additions & 60 deletions src/Compiler/Service/TransparentCompiler.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,76 @@ type internal TransparentCompiler
)
:> IBackgroundCompiler

let ComputeScriptClosureInner
(fileName: string)
(source: ISourceTextNew)
(defaultFSharpBinariesDir: string)
(useSimpleResolution: bool)
(useFsiAuxLib: bool)
(useSdkRefs: bool)
(sdkDirOverride: string option)
(assumeDotNetFramework: bool)
(otherOptions: string list)
=
let reduceMemoryUsage = ReduceMemoryFlag.Yes

let applyCompilerOptions tcConfig =
let fsiCompilerOptions = GetCoreFsiCompilerOptions tcConfig
ParseCompilerOptions(ignore, fsiCompilerOptions, otherOptions)

let closure =
LoadClosure.ComputeClosureOfScriptText(
legacyReferenceResolver,
defaultFSharpBinariesDir,
fileName,
source,
CodeContext.Editing,
useSimpleResolution,
useFsiAuxLib,
useSdkRefs,
sdkDirOverride,
Lexhelp.LexResourceManager(),
applyCompilerOptions,
assumeDotNetFramework,
tryGetMetadataSnapshot,
reduceMemoryUsage,
dependencyProviderForScripts
)

closure

let mkScriptClosureCacheKey
(fileName: string)
(source: ISourceTextNew)
(useSimpleResolution: bool)
(useFsiAuxLib: bool)
(useSdkRefs: bool)
(assumeDotNetFramework: bool)
(projectIdentifier: ProjectIdentifier)
(otherOptions: string list)
(stamp: int64 option)
=
{ new ICacheKey<string * ProjectIdentifier, string> with
member _.GetKey() = fileName, projectIdentifier
member _.GetLabel() = $"ScriptClosure for %s{fileName}"

member _.GetVersion() =
Md5Hasher.empty
|> Md5Hasher.addStrings
[|
yield! otherOptions
match stamp with
| None -> ()
| Some stamp -> string stamp
|]
|> Md5Hasher.addBytes (source.GetChecksum().ToArray())
|> Md5Hasher.addBool useSimpleResolution
|> Md5Hasher.addBool useFsiAuxLib
|> Md5Hasher.addBool useSdkRefs
|> Md5Hasher.addBool assumeDotNetFramework
|> Md5Hasher.toString
}

let ComputeScriptClosure
(fileName: string)
(source: ISourceTextNew)
Expand All @@ -393,57 +463,32 @@ type internal TransparentCompiler
let useSdkRefs = defaultArg useSdkRefs true
let assumeDotNetFramework = defaultArg assumeDotNetFramework false

let key =
{ new ICacheKey<string * ProjectIdentifier, string> with
member _.GetKey() = fileName, projectIdentifier
member _.GetLabel() = $"ScriptClosure for %s{fileName}"

member _.GetVersion() =
Md5Hasher.empty
|> Md5Hasher.addStrings
[|
yield! otherOptions
match stamp with
| None -> ()
| Some stamp -> string stamp
|]
|> Md5Hasher.addBytes (source.GetChecksum().ToArray())
|> Md5Hasher.addBool useFsiAuxLib
|> Md5Hasher.addBool useFsiAuxLib
|> Md5Hasher.addBool useSdkRefs
|> Md5Hasher.addBool assumeDotNetFramework
|> Md5Hasher.toString
}
let key: ICacheKey<string * ProjectIdentifier, string> =
mkScriptClosureCacheKey
fileName
source
useSimpleResolution
useFsiAuxLib
useSdkRefs
assumeDotNetFramework
projectIdentifier
otherOptions
stamp

caches.ScriptClosure.Get(
key,
node {
let reduceMemoryUsage = ReduceMemoryFlag.Yes

let applyCompilerOptions tcConfig =
let fsiCompilerOptions = GetCoreFsiCompilerOptions tcConfig
ParseCompilerOptions(ignore, fsiCompilerOptions, otherOptions)

let closure =
LoadClosure.ComputeClosureOfScriptText(
legacyReferenceResolver,
defaultFSharpBinariesDir,
fileName,
source,
CodeContext.Editing,
useSimpleResolution,
useFsiAuxLib,
useSdkRefs,
sdkDirOverride,
Lexhelp.LexResourceManager(),
applyCompilerOptions,
assumeDotNetFramework,
tryGetMetadataSnapshot,
reduceMemoryUsage,
dependencyProviderForScripts
)

return closure
return
ComputeScriptClosureInner
fileName
source
defaultFSharpBinariesDir
useSimpleResolution
useFsiAuxLib
useSdkRefs
sdkDirOverride
assumeDotNetFramework
otherOptions
}
)

Expand Down Expand Up @@ -676,8 +721,13 @@ type internal TransparentCompiler
(getSwitchValue useSimpleResolutionSwitch) |> Option.isSome

let! (loadClosureOpt: LoadClosure option) =
match projectSnapshot.SourceFiles, projectSnapshot.UseScriptResolutionRules with
| [ fsxFile ], true -> // assuming UseScriptResolutionRules and a single source file means we are doing this for a script
let lastScriptFile =
match List.tryLast projectSnapshot.SourceFiles with
| None -> None
| Some file -> if IsScript file.FileName then Some file else None

match lastScriptFile, projectSnapshot.UseScriptResolutionRules with
| Some fsxFile, true -> // assuming UseScriptResolutionRules and a single source file means we are doing this for a script
node {
let! source = fsxFile.GetSource() |> NodeCode.AwaitTask

Expand Down Expand Up @@ -2158,6 +2208,7 @@ type internal TransparentCompiler
bc.GetProjectSnapshotFromScript(
fileName,
SourceTextNew.ofISourceText sourceText,
DocumentSource.FileSystem,
previewEnabled,
loadedTimeStamp,
otherFlags,
Expand All @@ -2177,6 +2228,7 @@ type internal TransparentCompiler
(
fileName: string,
sourceText: ISourceTextNew,
documentSource: DocumentSource,
previewEnabled: bool option,
loadedTimeStamp: DateTime option,
otherFlags: string array option,
Expand All @@ -2199,7 +2251,8 @@ type internal TransparentCompiler
let previewEnabled = defaultArg previewEnabled false

// Do we assume .NET Framework references for scripts?
let assumeDotNetFramework = defaultArg assumeDotNetFramework true
// No, because the bootstrap info call also doesn't
let assumeDotNetFramework = defaultArg assumeDotNetFramework false

let extraFlags =
if previewEnabled then
Expand All @@ -2219,20 +2272,22 @@ type internal TransparentCompiler
let currentSourceFile =
FSharpFileSnapshot.Create(fileName, sourceText.GetHashCode().ToString(), (fun () -> Task.FromResult sourceText))

let! loadClosure =
ComputeScriptClosure
let otherFlags = List.ofArray otherFlags

// Always perform the load closure as we cannot be sure that the incoming file does not load any new additional files.
// Consider the scenario where a.fsx loads b.fsx. Based purely on a.fsx, we cannot know if b.fsx loads another file.
// Therefore we cannot rely on any caching for the script closure in this API.
let loadClosure =
ComputeScriptClosureInner
fileName
sourceText
FSharpCheckerResultsSettings.defaultFSharpBinariesDir
useSimpleResolution
(Some useFsiAuxLib)
(Some useSdkRefs)
useFsiAuxLib
useSdkRefs
sdkDirOverride
(Some assumeDotNetFramework)
(projectFileName, fileName)
(List.ofArray otherFlags)
optionsStamp
|> Async.AwaitNodeCode
assumeDotNetFramework
otherFlags

let otherFlags =
[
Expand All @@ -2243,13 +2298,31 @@ type internal TransparentCompiler
yield "--nowarn:" + code
]

// Once we do have the script closure, we can populate the cache to re-use can later.
let loadClosureKey =
mkScriptClosureCacheKey
fileName
sourceText
useSimpleResolution
useFsiAuxLib
useSdkRefs
assumeDotNetFramework
(projectFileName, "")
otherFlags
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now different than the otherFlags the closure was computed with, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because without these changes it does not get reused in ParseAndCheckFileInProject.

optionsStamp

// Populate the cache.
let! _ =
caches.ScriptClosure.Get(loadClosureKey, node { return loadClosure })
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only populate the cache if the key isn't already there, is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the ISourceTextNew is part of the key, I would also expect this to more or less always be the case that the cache won't have the entry.

Unless GetProjectSnapshotFromScript is being called with the exact same input twice, I don't think this will ever happen.

|> Async.AwaitNodeCode

let sourceFiles =
loadClosure.SourceFiles
|> List.map (fun (sf, _) ->
if sf = fileName then
currentSourceFile
else
FSharpFileSnapshot.CreateFromFileSystem sf)
FSharpFileSnapshot.CreateFromDocumentSource(sf, documentSource))

let references =
loadClosure.References
Expand Down
3 changes: 3 additions & 0 deletions src/Compiler/Service/service.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,7 @@ type FSharpChecker
(
fileName,
source,
?documentSource,
?previewEnabled,
?loadedTimeStamp,
?otherFlags,
Expand All @@ -577,10 +578,12 @@ type FSharpChecker
?userOpName: string
) =
let userOpName = defaultArg userOpName "Unknown"
let documentSource = defaultArg documentSource DocumentSource.FileSystem

backgroundCompiler.GetProjectSnapshotFromScript(
fileName,
source,
documentSource,
previewEnabled,
loadedTimeStamp,
otherFlags,
Expand Down
2 changes: 2 additions & 0 deletions src/Compiler/Service/service.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ type public FSharpChecker =

/// <param name="fileName">Used to differentiate between scripts, to consider each script a separate project. Also used in formatted error messages.</param>
/// <param name="source">The source for the file.</param>
/// <param name="documentSource">DocumentSource to load any additional files.</param>
/// <param name="previewEnabled">Is the preview compiler enabled.</param>
/// <param name="loadedTimeStamp">Indicates when the script was loaded into the editing environment,
/// so that an 'unload' and 'reload' action will cause the script to be considered as a new project,
Expand All @@ -268,6 +269,7 @@ type public FSharpChecker =
member GetProjectSnapshotFromScript:
fileName: string *
source: ISourceTextNew *
?documentSource: DocumentSource *
?previewEnabled: bool *
?loadedTimeStamp: DateTime *
?otherFlags: string[] *
Expand Down
Loading