[RFC FS-1146 Discussion] Scoped Nowarn

[Martin521]

This is a discussion about RFC PR #782.
Any feedback about the PR is appreciated.

[Martin521]

The RFC contains quite a few details that may be contentious.
Please check especially items 3, 4, 5 and 9 of the detailed specification section, the compatibility section, and the diagnostics section.

Early agreement on a final version is much appreciated because of the ongoing implementation work.

[T-Gro]

The nature of scoping (basically begin-end for directives) requires linear ordering of lines in a file.
However, linear ordering can be easily broken by the usage of #line directives, and a ruleset to make it work would make both the feature (explanation-wise) and implementation unnecessary complicated, possibly leading to bugs.

Since #line is meant for auto-generated and not hand-written code, I would like to suggest the following ruleset change:

For files that have 1 or more #line directives anywhere in that file, #nowarn affects the entire file and selective enablement/disablement of warnings it not supported in such a file. If #nowarn appears anywhere in such file, be it beginning/middle/end, it always affects the entire file.

This is how the documentation currently describes nowarn for all files anyway (which is wrong and this RFC will change it anyway). So in that case, it is not causing a regressed mismatch against the documented state. https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/compiler-directives#preprocessor-directives.

[Martin521]

For files that have 1 or more #line directives anywhere in that file, #nowarn affects the entire file and selective enablement/disablement of warnings it not supported in such a file. If #nowarn appears anywhere in such file, be it beginning/middle/end, it always affects the entire file.

I like the simplicity of this rule and support this change.

[Martin521]

For files that have 1 or more #line directives anywhere in that file, #nowarn affects the entire file and selective enablement/disablement of warnings it not supported in such a file. If #nowarn appears anywhere in such file, be it beginning/middle/end, it always affects the entire file.

I like the simplicity of this rule and support this change.

However, remembering this consideration in the RFC, we have to be aware that implementation will not be easy (probably more complicated than what I had in mind for the more complicated rule). When the warning is issued (in a part of the file where a #line directive is active), you don't have access to the name of the generated file, but only to the name of the generating file. We would probably need to somewhere store that mapping (perhaps in a global table similar to or in connection with FSharp.Compiler.Text.FileIndex).

[T-Gro]

I wonder if the fact of a file being generated (with line directives) can't be derived by a check, e.g. by checking consecutiveness of ranges or some other check. If not, then a single flag per file would have to be added, yes.
If yes, then it could be a lazy (+ cached) per file lookup, which would only happen if a diagnostic is produced.

[Martin521]

Yes, I agree, it is doable. It just needs some global table, since the only thing we have (when the warning is issued) is the FileIndex in the range of the warning.
(And that effort makes totally sense in view of the simplification in explainability of the feature.)

[KevinRansom]

Tomas and I had a chat about this, and I think we have level-set in our minds how we should think about this feature.

Whilst I have thoughts about the syntax of #nowarn and #warnon they are of a secondary nature, although I believe the C# syntax offers a very good guide and decent alignment with the .NET ecosystem.

The interaction with #line is the more interesting in my mind, because it offers the opportunity for deviations and strange behaviour. #nowarn and the obverse should work only against the actual source file being compiled, the #line directive should only impact the file and line number of error messages. For nowarn they are irrelevant,

Here is a C# example that I think demonstrates what I mean.

As you can see from the compiler output, the #line had no impact on the behavior of the nowarn.

I understand that the current implementation of the compiler makes it harder to figure out how to achieve this behaviour. Since the parser tells the type checker the filename and line number (modified by the #line).
And also there is some peculiar and very buggy code in the error reporting to figure out whether to use the source code file or something else.

I believe we can relatively easily figure out how to improve the nowarn behaviour so that it matches the C# behaviour, and probably simplify that wierd code too.

What we can do, when we are building and typechecking always use the filename and line numbers from the original source file, ignoring #line and # n. Store the #n and #line we encounter in a table while lexing. When we use nowarn, always use these values. When producing error messages map them back to the #line values we stored during lexing.

We can build the table here:https://github.com/dotnet/fsharp/blob/52133d002b14cbf5d16dbd771a7dda23bb29a6d4/src/Compiler/lex.fsl#L818
Or inside ApplyLineDirective might be better.

Fixing it won't be easy, but certainly not impossible. And we can probably reproduce the current wierd behavior on F# 8.0 too, if we really wanted to.

[Martin521]

Yes, that makes sense.
When I dismissed this alternative in the RFC draft, I based this on the local non-availability of the original file and line data when emitting the warning. In the meantime I have seen and understood FileIndexTable & Co and learned that such global tables are an accepted means in the compiler.

Unless anything else still comes up, I will change the RFC PR in a few days to reflect this.

[Martin521]

I have updated the RFC PR.

• The interaction between warn directives and line directives is now specified as proposed in the above thread.
• The proposal includes now warnings for invalid warn directive arguments (warning numbers).
• A number of items are now more precisely formulated.

Feedback is appreciated.

[Martin521]

How do you feel about the proposal to apply to scripts the same rules for #nowarn / #warnon as to .fs files?
This is a breaking change, but I see no good alternative. The only thing we could do is keep the old behaviour and ignore or disallow the use of #warnon in scripts.
Currently, any #nowarn in the script or even in the closure of all loaded scripts is valid for the whole script. I find this unintuitive, but it is the current behavior and also tested.

[Martin521]

I need to come back to the discussion on line directives. While I do prefer the LAST proposal over the PREVIOUS one, I'd like to bring up some consequences of the LAST proposal that we should consider before finally deciding.

Assume we parse a generated file xyz.fs that has #line directives pointing to abc.fsz. The idea is to create and store, by appropriate tracking of the line directives, a map from sections of the latter to sections of the former and thus construct the proper nowarn scopes for xyz.fs solely based on warn directives found in xyz.fs, ignoring any #line directives.

When later checking a warning that carries the file index of abc.fsz, we would use the map to find the corresponding position in xyz.fs and to check if it is in a nowarn scope.

There are, however, two issues.

First, a section in abc.fsz could have been used to generate multiple sections in xyz.fs. When checking warnings against nowarn scopes, we have no way to know which one of these sections we came from and thus which nowarn scopes would apply.

Second, the file index is stored in a global table that - as far as I can see - is kept and grown through the lifetime of the compiler service process. Therefore, if abc.fsz was used to generate not only xyz.fs, but also uvw.fs, we have again no way to ensure we point back to the correct file during warning checking.

Now, we know that in the typical fsyacc and fslex case, these issues don't come up. They might in other cases.

So, I am not sure if we should continue on this track, or rather fall back on the PREVIOUS proposal.

EDIT:
The above issue is actually present (in slightly different ways) for both proposals.
So, for now, I will assume we go for the LAST proposal and issue warnings if the mapping is not unique.