Add a version of TryGetRecentCheckResultsForFile with project snapshot

[dawedawe]

Description

As the title says, this adds a version of TryGetRecentCheckResultsForFile that works with a project snapshot.

Checklist

• Test cases added

• Performance benchmarks added in case of performance changes

• Release notes entry updated:

  Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

  Release notes files:

  • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
  • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
  • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

  Information about the release notes entries format can be found in the documentation.
  Example:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
    *while! (Language suggestion #1038, PR #14238)

  If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

[github-actions]

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

[0101]

I think for this method we should try to use the cache we already have with the results instead of creating a new one.

Maybe we can extend the version of the ParseAndCheckFileInProject to include file hash and add a method to retrieve a value based on key and version -> bool predicate.

That way we would have the same behavior, if I understand it correctly, where we care about the file's contents + ProjectCore matching, but other files' contents could be different.

[dawedawe]

Hey @0101 ,
thanks a lot for the review.

Maybe we can extend the version of the ParseAndCheckFileInProject to include file hash and add a method to retrieve a value based on key and version -> bool predicate.

I extended the version of the ParseAndCheckFileInProject cache to include the hash of the source string.

Did I understand you correctly that we should add a new method to the AsyncMemoize type taking a key and a version predicate? As you can see, I'm not entirely sure what you had in mind there or how I should use the API, thus the dummyVersion to be able to call cache.GetAll

[0101]

Hey @dawedawe

Did I understand you correctly that we should add a new method to the AsyncMemoize type taking a key and a version predicate? As you can see, I'm not entirely sure what you had in mind there or how I should use the API, thus the dummyVersion to be able to call cache.GetAll

I have not necessarily thought it all through... But we shouldn't need dummyVersion :) I'd suggest adding

LruCache.GetAll: 'TKey -> ('TVersion * 'TValue) list

And make it return the strongly held value (if present) first, so that in case there are multiple items matching we'll pick that one (which would be most recent). Don't know if we can give any meaningful ordering besides that. If we really wanted we could maybe add a timestamp or a counter to JobCompleted.

Possibly we can then implement the existing GetAll: key: 'TKey * version: 'TVersion -> 'TValue option * ('TVersion * 'TValue) list using the new one to reduce duplication. (Or maybe we could even remove it, because it's only use if we have AsyncMemoize(cancelDuplicateRunningJobs = true) which we never use.)

[0101]

Looking at the original docstring:

fsharp/src/Compiler/Service/service.fsi

Lines 382 to 392 in 9d36d64

	/// <summary>
	/// Try to get type check results for a file. This looks up the results of recent type checks of the
	/// same file, regardless of contents. The version tag specified in the original check of the file is returned.
	/// If the source of the file has changed the results returned by this function may be out of date, though may
	/// still be usable for generating intellisense menus and information.
	/// </summary>
	///
	/// <param name="fileName">The file name for the file.</param>
	/// <param name="options">The options for the project or script, used to determine active --define conditionals and other options relevant to parsing.</param>
	/// <param name="sourceText">Optionally, specify source that must match the previous parse precisely.</param>
	/// <param name="userOpName">An optional string used for tracing compiler operations associated with this request.</param>

It says the sourceText is optional, and if not given we should return any result for that filename. Although BackgroundCompiler currently just returns None if sourceText is not given 🤷

fsharp/src/Compiler/Service/BackgroundCompiler.fs

Lines 1148 to 1161 in 9d36d64

	match sourceText with
	| Some sourceText ->
	let hash = sourceText.GetHashCode() |> int64
	let resOpt =
	parseCacheLock.AcquireLock(fun ltok -> checkFileInProjectCache.TryGet(ltok, (fileName, hash, options)))
	match resOpt with
	| Some res ->
	match res.TryPeekValue() with
	| ValueSome(a, b, c, _) -> Some(a, b, c)
	| ValueNone -> None
	| None -> None
	| None -> None

Not sure what we should do for the new API. Probably we don't have to worry about this behavior since no one can be relying on it now when it doesn't work... We could make it work with an extra parameter (optional version of the file), but also I'm not sure how useful are old diagnostics when the source has changed.

[dawedawe]

Looking at the original docstring:

fsharp/src/Compiler/Service/service.fsi

Lines 382 to 392 in 9d36d64

	/// <summary>
	/// Try to get type check results for a file. This looks up the results of recent type checks of the
	/// same file, regardless of contents. The version tag specified in the original check of the file is returned.
	/// If the source of the file has changed the results returned by this function may be out of date, though may
	/// still be usable for generating intellisense menus and information.
	/// </summary>
	///
	/// <param name="fileName">The file name for the file.</param>
	/// <param name="options">The options for the project or script, used to determine active --define conditionals and other options relevant to parsing.</param>
	/// <param name="sourceText">Optionally, specify source that must match the previous parse precisely.</param>
	/// <param name="userOpName">An optional string used for tracing compiler operations associated with this request.</param>

It says the sourceText is optional, and if not given we should return any result for that filename. Although BackgroundCompiler currently just returns None if sourceText is not given 🤷

fsharp/src/Compiler/Service/BackgroundCompiler.fs

Lines 1148 to 1161 in 9d36d64

	match sourceText with
	| Some sourceText ->
	let hash = sourceText.GetHashCode() |> int64
	let resOpt =
	parseCacheLock.AcquireLock(fun ltok -> checkFileInProjectCache.TryGet(ltok, (fileName, hash, options)))
	match resOpt with
	| Some res ->
	match res.TryPeekValue() with
	| ValueSome(a, b, c, _) -> Some(a, b, c)
	| ValueNone -> None
	| None -> None
	| None -> None

Not sure what we should do for the new API. Probably we don't have to worry about this behavior since no one can be relying on it now when it doesn't work... We could make it work with an extra parameter (optional version of the file), but also I'm not sure how useful are old diagnostics when the source has changed.

I also think we don't have to worry but from a conversation last week, I know that editor developers were confused about the behaviour regarding returning always None when no sourceText was given.

[0101]

Looks good! Sorry it was so difficult 😅 I'd be happy for any suggestions on how to make working with the code easier. Probably some things should be renamed but I don't know to what... Maybe some more documentation?

[dawedawe]

Looks good! Sorry it was so difficult 😅 I'd be happy for any suggestions on how to make working with the code easier. Probably some things should be renamed but I don't know to what... Maybe some more documentation?

Ah, don't take me as a benchmark, I was pretty dump this week.
But yes, some documentation about the cache design, how keys and their versions work would be great.

Thanks again for all the hand-holding here! I can't wait to have all this in the IDEs.

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

Good stuff. Thanks for your enduring help with TC :)

[dawedawe]

Mmh, no idea why the one test failed in the last run. I can't reproduce that locally 🤷
Can this be related to something else?

[psfinaki]

Yeah sorry we have a lot flaky tests recently.

[0101]

Yeah sorry we have a lot flaky tests recently.

But that was a test added by this PR 😂. We might have to revisit it so we don't increase the flakiness.

The tests in the CI sometimes behave quite unpredictably compared to local runs (when working with Tasks/async stuff), so these failures are not reproducible. I think it's when the CI machine is overloaded or something and tasks that are started don't actually start running for a long time. So some barriers are needed before we assert something.

But also maybe this test has been fully replaced by the new workflow tests and can just be removed?

[dawedawe]

But also maybe this test has been fully replaced by the new workflow tests and can just be removed?

Yep, let's remove it.