-
Notifications
You must be signed in to change notification settings - Fork 786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the interaction between #line and #nowarn directives consistent #17649
Conversation
❗ Release notes required
|
37e41ba
to
d3aee6e
Compare
d3aee6e
to
64f4d33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Martin.
Head branch was pushed to by a user without write access
Any volunteer for the second authorized review? |
A bunch of folks are off/away currently, will review it in few days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks this looks good, but please rename the feature name.
Description
This fix makes the interaction between
#line
and#nowarn
directives consistent and defined.Currently this is not the case, as the following example shows.
Assume this file
xyz.fs
:It produces a warning FS0025 for line 5. If you replace the
10
in the first#line
directive by1
, no warning is shown. (Use the compiler to test this. Your editor, using the background compiler, tells again a different story. But we will tackle that later.)The reason for the inconsistency is the checkFile flag, introduced more than 10 years ago. Originally probably a hack in a debugging situation, it survived reviews since, and ended up being used in further modules. When this flag is set to
false
(which it is in most situations),#nowarn
processing relies on comparing line numbers of one file with line numbers of another file. Accidentally, it sometimes works.There are in principle two ways to fix this. (Note that the F# spec says nothing about the interaction of
#nowarn
and#line
.)The first is to consider
#line
directives as irrelevant for the scope of#nowarn
. So, in the above example, "incomplete match" warnings would be disabled from line 3 to the end of the file. To implement this, however, we would need bookkeeping of the original line numbers and the line numbers adjusted by#line
directives. Which would mean to extend the ubiquitousrange
struct to carry both kinds of line numbers. The cost of this (both refactoring cost and compiler runtime cost) is prohibitive.The other other way is as follows. The sections that in terms of the
#line
directives belong to a certain file are considered by#nowarn
as a separate file. For the typical case of source generation, this means that, in the generated file, a#nowarn
in a line pointing to the generating file is effective only in other such lines, and not in lines that are not affected by any#line
directive. So, in the above example, the#nowarn 25
belongs toxyz.fsy
and does therefore not suppress the warning for the last line, which belongs tolineNoWarn.fs
.This is a breaking change (in that warnings might appear that were suppressed in previous language versions) and must therefore be put under a feature flag. With this flag, the new specification is applied only for new language versions. But for previous language versions, we still want to fix this absurd bug. We do this by implementing a behavior that is equivalent to always having
checkFile = false
(which is currently the case anyway in 4 out of 5 cases that use the flag) and omitting the nonsensical comparison of line numbers from different files, i.e. apply any#nowarn
to the whole file. This is a slightly looser check for "nowarn" than currently, which means that theoretically, a warning that was emitted by (say) SDK 8.0 might be suppressed when using language version 8.0 from the new SDK. This should be acceptable.Checklist