Unique names for generated "test.fs" files

[majocha]

WIP, just to see how many things break.

See: #18049 (comment)

Main problem is that the hardcoded "test.fs" makes it into expected IL as implicit module name (?) in many VerifyIL tests.

[github-actions]

✅ No release notes required

[majocha]

I don't really like the approach in this PR.
I wonder what it would take to implement what the comment here says instead:

fsharp/src/Compiler/Utilities/range.fs

Lines 189 to 195 in 5d0812f

	// Note: we should likely adjust this code to always normalize. However some testing (and possibly some
	// product behaviour) appears to be sensitive to error messages reporting un-normalized file names.
	// Currently all names going through 'mkRange' get normalized, while this going through just 'fileIndexOfFile'
	// do not. Also any file names which are not put into ranges at all are non-normalized.
	//
	// TO move forward we should eventually introduce a new type NormalizedFileName that tracks this invariant.
	member t.FileToIndex normalize filePath =

And if it would help?

[KevinRansom]

What are you trying to solve?

[majocha]

There's a problem with FileIndexTable that Martin is hitting in his PR:

A week ago I found that some random test failure that appeared in some of the CI runs were no longer flaky tests that everybody experiences (like a few weeks ago), but are specific to this PR. The reason turned out to be that, for a certain class of tests, global resources (in this case, the FileIndex tables) are kept across tests but are used in ways that were not foreseen (in this case, using different source files with the same name). Therefore these tests are not suited for the parallelization that was recently introduced. It seems that for other uses of FileIndex (like for #line directives, or for file names for error messages) this issue is not visible or just very rare, because of low test coverage.

It seems it is fundamentally non thread safe :(

[KevinRansom]

You mean the compiler caches based on just filename rather than the path?

[majocha]

Yes, so it seems.

[KevinRansom]

Well, that is a compiler bug then.

[majocha]

Yes, exactly! Working around it doesn't seems right to me.

[KevinRansom]

For IL verify, because the module name appears in the output I try to use
withName "MyNameForTest"
and
|> withOutputContainsAllInOrderWithWildcards [
"All Classes and Methods in*MyNameForTest.exe Verified."

rather than rely on generated names.

Someone should add a bug and repro for the FileIndex issue. I wonder what it means for deterministic builds though.

[Martin521]

Aren't these two different problems?

• FileIndex seems to be computed based on file name only instead of path
• Some test sets use (with the same checker instance) that same file name "test.fs" in multiple tests

[majocha]

Aren't these two different problems?

• FileIndex seems to be computed based on file name only instead of path
• Some test sets use (with the same checker instance) that same file name "test.fs" in multiple tests

Yes, right. I have to take a deeper look again.

[Martin521]

I guess you didn't want to use Path.PathSeparator, which is "A platform-specific separator character used to separate path strings in environment variables."

[majocha]

@Martin521 I'll try to redo it with more limited approach, only touching the places where the checker is used directly. (i.e. ParseAndCheckFileInProject). Other code paths using checker.Compile actually do commit the files to a unique temp directory first, so they should be good.

[majocha]

Ok, so I just made the names of synthetic sources (that never got written to disk) unique.
But now I wonder if this is necessary at all.
The checker checking different sources under the same filename is equivalent of checking many quick changes in the same file. This should never break anything, even when executed in parallel. If it does break, it is a bug, not a test suite issue.

[Martin521]

The checker checking different sources under the same filename is equivalent of checking many quick changes in the same file. This should never break anything, even when executed in parallel. If it does break, it is a bug, not a test suite issue.

Well, if it is done in parallel, one test might use the (intermediate) FileIndex-based results of another one. In "my" case, one might skip a warning because the other one had a "#nowarn". This is a rare race condition, but it is happening. It is at least my explanation for runs like this one that show errors that did not appear before or after.

I would love to test your code in my PR.

[majocha]

Ok, then it seems my approach is wrong again. Probably we should just give a unique ProjectId or Stamp:

fsharp/tests/FSharp.Test.Utilities/CompilerAssert.fs

Lines 410 to 420 in 5d0812f

	ProjectFileName = "Z:\\test.fsproj"
	ProjectId = None
	SourceFiles = [|"test.fs"|]
	OtherOptions = Array.append testDefaults assemblies
	ReferencedProjects = [||]
	IsIncompleteTypeCheckEnvironment = false
	UseScriptResolutionRules = false
	LoadTime = DateTime()
	UnresolvedReferences = None
	OriginalLoadReferences = []
	Stamp = None

each time we generate a test project.

[Martin521]

Probably we should just give a unique ProjectId or Stamp

How would that help? Does it change the file name?

[majocha]

It instructs the service that we have different project. I think it shoud create separate incremental builders for each, or in case of Transparent Compiler, cache the intermediate results correctly, because project snapshots are different.

[Martin521]

But as long as it runs in the same process, we have the same global FileIndex table, no?

(Which means also that my remark above about "the same checker instance" does not make sense. It is about the same process, I think.)

[majocha]

Yes! I'm not familiar with that stuff, but I think parsing and lexing operates on a lower level that is oblivious of the checker, and uses global state that is somehow not fully threadsafe. If it comes out that having a file with same name in two different projects leads to errors in parallel checking, this is really a compiler bug.

[majocha]

It would be great to have a small clean repro.

[Martin521]

In all production scenarios it is not a problem, because a file with the same path is the same file. The problem appears only in testing.

[Martin521]

It would be great to have a small clean repro.

Let me try to create that

[majocha]

It seems when this is called, there is no requirement for fileName to be absolute path:

fsharp/src/Compiler/SyntaxTree/LexHelpers.fs

Line 121 in 5d0812f

lexbuf.EndPos <- Position.FirstLine(FileIndex.fileIndexOfFile fileName)

I wonder if it is sufficient to have two different files with same name in two different projects for this to break.