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 :)

[psfinaki]

Hi @Martin521 and sorry this has been taking forever! I know you're patient but we shouldn't exercise your patience too much :)

We've discussed this PR within the team. In generally, we're in favor of having this in - it's a valuable improvement. That said, since the PR touches a few important places, we should be very sure that we don't compromise the compiler design and VS functionality. It's not that both are perfect but you know - the boy scout rule.

I added a few notes in accord with this - let's iterate here a bit and refine the changes.

[psfinaki]

For my understanding, why this had to be removed (and nothing equivalent readded then)?

[Martin521]

The nowarn information (now: warn scope information) is no longer threaded from post-parse (where it is created) to the type checking diagnostics logger by means of hiding it in the AST (where IMO it does not belong), but is stored now directly in the diagnostic options. See also the more detailed comments below.

(BTW I think the error handler update in the next line can also be removed. I will look into that.)

[psfinaki]

Nice, thanks for the explanation!

[psfinaki]

WarnScopeData in its current form blurrs the type a bit. FSharpDiagnosticOptions used to represent a simple immutable piece of compiler configuration whereas adding a mutable object somewhat complicates the matters here.

Do you have ideas on how else can this be achieved? For example, we tend to have a lot of mutable data in TcGlobals and CompilerConfig, maybe those could be leveraged here?

[Martin521]

Yeah, I didn't particularly like it either.

Here are the reasons why I ended up with this mutable field.

1. The information about the warn scopes belongs into FSharpDiagnosticsOptions as much as all its other fields. All these fields are used only in one single place in the whole compiler, namely in PhasedDiagnostics.AddSeverity. Passing the information separately does not fit its purpose and would add an additional parameter to many functions.
2. This information needs to be updated during postparse for every file. This lies in the nature of the data: the #nowarn/#warnon directives change the way warnings are emitted.
3. I modeled this in some way after the BufferLocalStore of lexbuf. Just a way to store data for a certain aspect for the current compiler phase.
4. WarnScopeData is accessed only via private functions WarnScopes.getWarnScopeData and WarnScopes.setWarnScopeData in the WarnScopes module.

I have looked into alternatives but didn't find anything acceptable.

One way would be to thread the WarnScopeData separately, or as part of the inputs, back to the "main" program. In the case of fsc that would be here. Bring it from there into main2 and use it here, similarly to how it is done today. This also means that the "merging" of the warn scopes of the different files has to happen here (rather than in the WarnScope module where it is much better placed). Next to fsc, this also has to happen (in even more complicated ways) for fsi, BackgroundCompiler, TransparentCompiler and FSharpCheckerResults.

Another way would be to do the above (threading of WarnScopeData through the function calls) only on the way up to the "main" program (i.e. main1 in the case of fsc) and there do the merging, create a new FSharpDiagnosticsOptions with an immutable field WarnScopeData, and update TcConfig to contain this new version (by converting TcConfig to TcConfigB, adding it, going back to TcConfig, like in other places.)

But all of this is complicated and error prone, pulls the processing of the warn scope data into multiple places and is in the end doing the same unavoidable thing: changing the way that diagnostics is processed (which we would love to have immutable, but cannot) as a result of parsing the warn scope data, i.e. after parsing. (Which, BTW, you can still nicely see in the removed code of the previous comment (FSharpCheckerResults.fs:3223): in a nastily opaque way, the DiagnosticsOptions inside TcConfig were updated with the parse results. So much for immutability.)

To me, the way it is handled in the current PR version feels much cleaner and safer.

However, if the team is certain we should go another way, I will follow.

[psfinaki]

Thanks for the detailed explanation. It's good that it follows at least some pattern then... Maybe @T-Gro you'll have ideas.

[psfinaki]

I am afraid this is an issue for us. Compiler changes shouldn't degrade VS functionality. Can you share what's the problem appearing here? We can help with adjusting the things here.

[Martin521]

The above test creates a VS project and checks the reported errors. However, I am developing on Linux and have no access to VS. So, thanks for offering support here. Ideally, you would create a VS-independent test, working directly with the compiler service, for that same failing scenario. And I could take it from there.

This new feature necessitates changes in tooling. Not only for VS, but also for Rider, Ionide, Fantomas etc. I am happy to involve myself here. For the non-OSS tools this will necessarily be less hands-on than what I might be able to do for, say, Fantomas.

How is tooling affected by this PR?

First, for "coloring" (syntactically understanding) the #nowarn/#warnon directives themselves. Because of the new directive, a change in the respective API is unavoidable. As far as I understand the situation (see also the diagram that I used in the recent review), Ionide and Fantomas get the necessary information out of the Trivia that are attached to the AST, while for VS additional artificial tokens are generated by ServiceLexing to carry the same information. This PR includes the necessary changes for both interfaces. But the consumption side in the tools, of course, must also be changed.

Secondly, the tools need the error/warning information that is influenced by the new directive. Ideally, nothing should change here. The filtering of the messages according to the various warnon/off defaults / compiler options / directives should be happening only internally in CompilerDiagnostics.AdjustSeverity. Only the way the necessary information flows into that method has changed. But the filtering should be the same for all use cases.

Why is the test failing (if the Skip is removed)?

The same source code is working fine in other test environments.
I suspect that VS is using the compiler service in a way that is not sufficiently covered by the existing Component and Service tests. But I have no way to find out, without VS.

[psfinaki]

Alright, I will look into that and see what's happening there. If it's an easy fix, I'll push it here :)