ParseAndCheckFileInProject may return stale results

[auduchinok]

Consider the following repro steps for project with two files:

• Parse and check File2.fs (File1.fs is checked as a dependency, results for File1 and File2 get cached)
• Change File1.fs and parse and check it
• Parse and check File2.fs again, it'll have the same stamp (since its source hasn't changed), so it will use the old results

The following code uses the incremental builder cache that is seemingly never updated by the background compiler after a file check:

fsharp/src/fsharp/service/service.fs

Line 480 in 8bfb0c7

member _.GetCachedCheckFileResult(builder: IncrementalBuilder, filename, sourceText: ISourceText, options) =

After checking a file only background compiler caches are updated, not the incremental builder ones:

fsharp/src/fsharp/service/service.fs

Line 567 in 8bfb0c7

bc.RecordTypeCheckFileInProjectResults(fileName, options, parsingOptions, parseResults, fileVersion, timeStamp, Some checkAnswer, sourceText.GetHashCode())

This was discussed in #11228 (comment), but FCS 39 has different logic there, and a different fix is needed (unless the PR with the fix is merged soon and everything else is considered stable enough for a new FCS release).

[auduchinok]

I've checked commits that I based our FCS builds on and the issue appeared at some time between:

• d21e15e that doesn't have this issue.
• 8bfb0c7 that has.

[dsyme]

The comparison is here: d21e15e...8bfb0c7

The relevant diff is I believe this one:

@TIHan My understanding is the removal of the call to IncrementalBuilder.IsReady plus the use of a TimeStampCache is the cause of the bug. The old IsReady was this:

    let IsReady cache target bt = 
        let workList = CollectActions cache target bt 
        workList.IsEmpty

and the new one just isn't doing the same set of checks to make sure there is no work to do.

In any case #11228 should be fixing this. But we may want to help @auduchinok find a fix too that can be applied to FCS 16.9

[dsyme]

@auduchinok I think this fix is adequate, it puts the logic of AreCheckResultsBeforeFileInProjectReady back to where it was

    member builder.AreCheckResultsBeforeFileInProjectReady filename = 
        let slotOfFile = builder.GetSlotOfFileName filename
        let cache = TimeStampCache defaultTimeStamp
        let ctok = AssumeCompilationThreadWithoutEvidence()
        computeStampedReferencedAssemblies cache ctok
        computeStampedFileNames cache ctok
        match tryGetBeforeSlot slotOfFile with
        | Some _ -> true
        | _ -> false

This also basically matches what's in #11228, see https://github.com/dotnet/fsharp/pull/11228/files#diff-4cf589945def81188bed84b5466d6c2c6eca933e91427e54ed059f018fb9cec9R1210

[dsyme]

@auduchinok reports this bug is also in Visual Studio 16.9.1. @TIHan I'm not sure we can do anything about this except wait for 16.10

[dsyme]

I pushed the patch to this branch: https://github.com/dotnet/fsharp/tree/patches/fcs-39-bug-10538

@TIHan It might make sense for us to add a test case that explicitly gets fixed by that patch (assuming it's correct, I'm pretty sure it is) and then double check that #11228 also passes that test

[baronfel]

I can reproduce this in latest Ionide as well (which is based on FCS 39) with the following very simple test harness:

parent.fsx:

module Dependency =
    let value = 10

script.fsx:

#load "./parent.fsx"

open Parent

let result = Dependency.value + 20

Follow @auduchinok's test steps, but as the change to 'file 1' change value in parent.fsx to, say, a boolean value. You expect result's expression to be in error now, but it's not.

[TIHan]

My understanding is the removal of the call to IncrementalBuilder.IsReady plus the use of a TimeStampCache is the cause of the bug.

Yes, that is right.

#11228 should fix this since it puts the computing timestamps back in place.

[dsyme]

I can reproduce this in latest Ionide as well (which is based on FCS 39) with the following very simple test harness:

@baronfel Do you have a bespoke fork+build of FCS or do you rely solely on the nuget? (@auduchinok has a bespoke build in Rider and can thus apply the patch bug fix above). If not would be good to get the FCS 39.x release updated with the given fix.

[baronfel]

@dsyme we rely only on FCS39 at the moment so we'd need a patch release here in order to get a fix. I suppose we could also look at something like a git-reference using Paket, but we still couldn't use main in that case because our long tail of constituent libraries (like Ionide.ProjInfo, Fantomas, FSharpLint, etc) would be broken due to API changes.