Scoped nowarn

[Martin521]

Description

Implements Scoped Nowarn according to draft RFC FS-1146.

This PR has taken a while. I had to deal with much more complexity than I imagined when I naively volunteered to tackle the feature request. Anyway, here we are.

I have split the PR into 7 commits that can be reviewed in sequence.
All of them compile, 1 and 4 - 7 also pass all tests locally.

1. Add the feature flag, baseline tests, and the core WarnScopes module. See src/Compiler/SyntaxTree/WarnScopes.fsi and the RFC for the functionality of the module.

2. Add the necessary changes to lexing and parsing. Note that the warn directives can no longer be collected during parsing (since they can now appear not only in top-level modules, but anywhere). So we collect them during lexing, similar to the processing of #if/#else/#endif directives.

3. Remove legacy #nowarn processing (but hold off AST changes)

4. Integrate the WarnScopes functionality and test it

5. Add warn directive trivia (but hold off AST changes)

6. Enable warn directive trivia (which means AST changes)

7. Remove defunct types and parameters related to former #nowarn processing (more AST changes)

There is also a separate commit for the IlVerify baseline updates (change in line numbers only)

Checklist

• Test cases added
• Performance benchmarks added in case of performance changes
• Release notes entry updated
• Create documentation update PRs (see RFC)

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.300.md
LanguageFeatures.fsi	docs/release-notes/.Language/preview.md

[psfinaki]

Hi @Martin521 - thanks for the contribution. It's a substantial effort and we appreciate it. The PR is on our radar - just keep in mind that it's big and specific, and it will take time to find capacity for it.

If anyone from the community gets to thoroughly review it, that would be valuable as well.

Thanks for your diligence and patience :)

[Martin521]

In any case, however, we need to get agreement on the main compatibility question. That's the question if we should remove the current inconsistency between Fsc and the IDEs regarding #nowarn, or deepen it.
I.e. if we should introduce a breaking change to make them consistent, or enable "warn scopes" only for fsc.

This is a question that should be discussed with the RFC, but here is the technical / historical background that gives rise to the question.

The #nowarn directive is handled in two very different ways. One way (my compiler archeology lets me think this is the older one) is to search through the modules of each parsed file, to extract the directives (but only for top-level modules) and for each #nowarn directive to either add a --nowarn flag to the compiler options (this is done in the checker when handling projects, and in fsi in certain situations) or to fake a --nowarn flag by mutating (in an obfuscated way) the "immutable" tcConfig.diagnosticsOptions.WarnOff (this is done in other situations in the checker and fsi, and when compiling scripts).

It seems that at some point then somebody wanted to create a better design. They created the field ScopedPragmas of the parsed input. Here, the #nowarn directives are collected with their file index and line number, so that the effect of the #nowarn can be scoped to the lines following the directive (until eof). These ScopedPragmas are then used in creating a DiagnosticsLoggerFilteringByScopedPragmas, which in principle can filter the warnings in the right "scoped" way. But then the scoping is disabled again in that logger except when it is called from fsc. Also, all the flag based #nowarn processing was left in place except for the fsc case. Because of the extremely complicated and obfuscated way the diagnostics loggers are created and used, it is almost impossible to find out which of the two ways of #nowarn processing is used where. Sometimes they are even just used in parallel (like here). So, that nice new design was implemented only for the "fsc" path and the rest was left in an inconsistent state. (BTW all of this happened before this repo was created, so nobody to "blame".)

So, there are now inconsistent ways in which #nowarn works. In straight fsc compilation of .fs and .fsi files, it works "scoped", i.e. is in effect from the directive to eof. But in your IDE, that same #nowarn is in effect for the whole file. And if you include a script file in your project and compile the project, a #nowarn in the script (or any script #loaded by the first one) is in effect for the whole compilation.

So, assuming we find a way to implement "warn scopes", should we implement it only for fsc, or everywhere?

[majocha]

A fix would be to make sure that in testing (for tests that use the same checker instance) the file names are unique. But there are some 300 tests that explicitely use "test.fs" or "test.fsx". (Pinging @majocha here for possibly other ideas.)

Probably hardcoded default names without a directory? Like this:

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

Line 1023 in b64ea2b

let sourceFileName = defaultArg fileName "test.fsx"

I'll try to replace the occurrences with generated names, shouldn't take long.

[Martin521]

I'll try to replace the occurrences with generated names, shouldn't take long.

If that's doable - that would be really great.

[psfinaki]

@majocha - thanks for your relentless support here :) Indeed, this would be great.

---

@Martin521 thanks for the detailed answer. I think I understand the inconsistency. We might want to discuss it a bit, but before that - what do you think is easier to do from the implementation perspective?

Very generally speaking, the preference is:
consistent in a wrong way << inconsistent << consistent in the right way :)

[Martin521]

what do you think is easier to do from the implementation perspective?

From an implementation point of view, a consistent implementation of warn scopes for all use cases is definitely simpler. It is simpler to implement and also enables us to remove all the complicated code around the old way of dealing with #nowarn.

(If you want to have a taste of the code around the #nowarn collection that I mentioned above, have a look at ApplyMetaCommandsFromInputToTcConfigAndGatherNoWarn, which calls ProcessMetaCommandsFromInput. The latter looks as if somebody has just found out that functions can be passed around. "Functional" design at its worst. No wonder that here the author no longer knows what they are doing (the comment says "apply any nowarn flags", but the code doesn't do that at all.)

[psfinaki]

@Martin521, so a consistent and simpler approach would be preferred here. In general, we count with breaking changes here.

Another way to put it - when users start relying on bugs, fixing bugs becomes a breaking change. But often that's still the right way to go. We're currently having such discussion here. But it might require some communication and documentation later :)

[Martin521]

@Martin521, so a consistent and simpler approach would be preferred here. In general, we count with breaking changes here.

Another way to put it - when users start relying on bugs, fixing bugs becomes a breaking change. But often that's still the right way to go. We're currently having such discussion here. But it might require some communication and documentation later :)

Thanks, that's very clear and I am glad you see it that way.

First, however, I have now to dive still deeper into the uses of the global FileIndex. I am relying on it now for scoped nowarn, and that is fine for all production scenarios and all sequential testing, but not for parallel testing. It is not so easy to decide if that is a test suite issue, or if I should not have used FileIndex in the way that I do. I want to really understand and document that in detail (which will probably take a while) and then come back to your 3 questions.

[psfinaki]

Sure, let's tackle issues one by one :)

[psfinaki]

@Martin521, I'll mark this as draft - the sole purpose is for the team to get notified once the feedback is addressed and the PR is ready to review again. I am personally watching this closely anyway :)

[Martin521]

I have fixed the testing issue mentioned above.

For those interested, here are some details.

FileIndex.fileIndexTable is a global mutable resource that represents a bidirectional mapping between file paths and file indices.
Originally, FileIndex was probably only meant to store source file identity in a compact way in range structs, and the FileIndexTable was used to get back to the file path. But then additional uses crept in. For example:

• DiagnosticsLoggerFilteringByScopedPragmas uses the file index to match warning ranges against #nowarn directives that belong to the same file index.
• Graph type checking uses FileIndex extensively to identify a "physical file in the current project".
• And now I want WarnScopes to use FileIndex to match warning ranges against warn scopes ranges that belong to the same file index.

This is all fine in any production scenario (fsc, fsi, FSAC), because all files have unique paths. Mapping to FileIndex and back is not a problem. In Testing, however, there are tests that use, in different tests, the same file path "test.fs" in what looks to the compiler service like the same project ("test.fsproj"). When these tests run in parallel, matching ranges by file index is no longer reliable because the same file index might now be used by different tests and a, say, #nowarn directive from one test might be matched against a warning range of another test.

What were the options?

• Either keep testing as is, but avoid in the compiler any use of FileIndex other than getting the file path (avoid, for example, the use of range.FileIndex to find out if two ranges belong to the same file),
• Or require that in testing all file paths are distinct,
• Or require that in testing all file paths in a project are distinct (where "project" is any collection of tests that end up using the same tcConfig).

I went for the third option. Because it was simple to implement on the testing side (thanks to @majocha for providing the code), while implementing warn scopes under the first option would be very difficult.

[Martin521]

Now, finally, the answers to the questions.

1. Do you consider any changes here breaking by any definition of breaking? (Fixing deep long-standing bugs might sometimes qualify...)

This is completely covered by the compatibility section of the RFC.

(Note that, currently, the code in this PR produces the new behavior for scripts (item e) independent of the feature flag. If we want to keep the old behavior for old language versions, this still needs to be fixed.)

2. What parts of the compiler code are changes beyond propagation of the function parameters etc.? And how does the PR ensure that nothing breaks in that sense? Can you think of any testing there on top of what you've already added?

The substantial changes in this PR are all based on one of two reasons.
One is the fact that the warn directives can no longer be collected during parsing (since they can now appear not only in top-level modules, but anywhere). So - in the absence of a preprocessor - we have to collect them now during lexing, similar to the processing of #if/#else/#endif directives. The related code changes are in the second commit of this PR.

The second reason is the feature itself and the processing of the warn directives that it needs. Previously, the #nowarn directives were processed in a number of different places (postparse, fsc, fsi, bgCompiler, TransparentCompiler, FSharpCheckerResult, ScriptClosure) to make them land in either FSharpDiagnosticOptions.WarnOff or in a --warnoff compiler option, often in complicated ways that were not easy to follow and IMO already crying for refactoring. Now, with the additional complexity of the Warn Scopes feature (we have to compute "on" and "off" scopes from the #nowarn/#warnof directives for each warning number and file), that way of handling the directives was no longer maintainable. So, all that complicated code was removed and instead we have now all warn directive processing in a single module WarnScopes with a very small surface. The related changes to existing code are in commits 3 and 4 of this PR.

Every large refactoring comes, of course, also with risks. I want to put these risks here into two categories.
The first one is about the new implementation deviating from the spec (the RFC). I believe the risks in this category are low. The whole feature functionality is implemented in the single WarnScopes module, is accessed only internally from very few places, and is quite well tested in FSharp.Compiler.ComponentTests:CompilerDirectives.NoWarn.

The second category is about incompatibilities with the previous behavior.
Next to the known and intended incompatibilities listed in the RFC, there might still exist unknown incompatibilities. Due to the previously distributed (duplicated) processing of #nowarn in the many different "compilers" (fsc, fsi script, fsi eval, bg, transparent, checker), this is very difficult to track. I also suspect that in general not all of these paths are sufficiently covered by tests. But I don't think the risk is too high - nothing popped up when building this big repo.

3. We respect the fact you don't have Visual Studio for testing the feature - that said, in principle, what do you think should be tested to be sure things work there and in other editors? I did some smoke testing yesterday in my VS (successfully) but I might have missed some scenarios. We might set up some sort of community session to try things out or brainstorm things a bit.

This is a very good question.

Currently, I cannot test any of the IDEs.
As far as I understand, there is a way to make a custom version of FsAutoComplete with my compiler version, and to configure Ionide to use it. But will have to find out if and how this works in my dev container setup.

If I knew how exactly the IDEs use the compiler service to obtain diagnostics, I would add tests for exactly these scenarios to FSharp.Compiler.Service.Tests.

It might also make sense to add a few warn scope tests to vsintegration and/or FsAutoComplete.

To have a session with some experts is a good idea.

[psfinaki]

Thanks @Martin521 for the update and the response. I'll get to that tomorrow, stay tuned.

[psfinaki]

Summoning @baronfel for FSAC thoughts :)

[psfinaki]

So I think we are getting there.

We generally two things:

1. Review the compiler changes once again, with fresh eyes. We can organize a community session for it also - @edgarfgp FYI.
2. Come up with editor related testing scenarios - and implement them.

[psfinaki]

Sorry I pressed the wrong button!