Ensure script load closure is always executed for GetProjectSnapshotFromScript #16880
Conversation
nojaf
added
NO_RELEASE_NOTES
|
tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs
Outdated
Show resolved
Hide resolved
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
This has a bit suspicious use of AsyncMemoize. I get why it's needed, but wondering if there's a more elegant solution to all this.
A "proper" solution would maybe require a two step process where we first gather all the files that are being loaded and then for the second step we can already supply their snapshots as input.
| [<InlineData(true)>] | ||
| let ``The script load closure should always be evaluated`` useTransparentCompiler = | ||
| async { | ||
| // The LoadScriptClosure uses the file system shim so we need to reset that. |
There was a problem hiding this comment.
Why do we need to deal with the file system when we have DocumentSource? Is it because of the original implementation?
There was a problem hiding this comment.
Yes, the actual ScriptLoadClosure relies on the FileSystemShim.
It currently cannot use the DocumentSource.
| useSdkRefs | ||
| assumeDotNetFramework | ||
| (projectFileName, "") | ||
| otherFlags |
There was a problem hiding this comment.
This is now different than the otherFlags the closure was computed with, is that ok?
There was a problem hiding this comment.
Yes, because without these changes it does not get reused in ParseAndCheckFileInProject.
|
|
||
| // Populate the cache. | ||
| let! _ = | ||
| caches.ScriptClosure.Get(loadClosureKey, node { return loadClosure }) |
There was a problem hiding this comment.
This will only populate the cache if the key isn't already there, is that expected?
There was a problem hiding this comment.
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.
Description
In the transparent compiler the
GetProjectSnapshotFromScriptwould currently re-use the load closure from the cache for the same file. This is problematic as newly loaded files would not be picked up.See test for a good example. There, the loading of file c.fsx (from b.fsx) would not be picked up if the load closure was re-used.
The changes in this PR align the behaviour better with what the background compiler does: never use the load closure cache in
GetProjectOptionsFromScriptbut add the result to the cache afterwards. So it can be reused inParseAndCheckFileInProject.//cc @0101 @auduchinok
Checklist
Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated: