Skip to content
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

Merged
merged 11 commits into from
Sep 10, 2024

Conversation

Martin521
Copy link
Contributor

@Martin521 Martin521 commented Aug 31, 2024

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:

namespace X
#line 10 "xyz.fsy"
#nowarn "25"
#line 4 "xyz.fs"
module B = match None with None -> ()

It produces a warning FS0025 for line 5. If you replace the 10 in the first #line directive by 1, 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 ubiquitous range 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 to xyz.fsy and does therefore not suppress the warning for the last line, which belongs to lineNoWarn.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.

Note to reviewers:
The core change is in DiagnosticsLoggerFilteringByScopedPragmas(...) in CompilerDiagnostics.fs.
The change in signature of that constructor is reflected in small changes in CompilerDiagnostics.fsi, fsc.fs, ParseAndCheckInputs.fs, IncrementalBuild.fs, TransparentCompiler.fs.
The new consistent behavior needs adaptation of the three .fsy files and of fsyaccdriver.fs.
The feature flag needs changes in FSComp.txt and LanguageFeatures.fs/fsi.
Tests are covered by Nowarn.fs, which is included now in FSharp.Compiler.ComponentTests.fsproj.

Checklist

  • Test cases added
  • Release notes entry updated

@Martin521 Martin521 requested a review from a team as a code owner August 31, 2024 16:32
Copy link
Contributor

github-actions bot commented Aug 31, 2024

❗ 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.100.md
LanguageFeatures.fsi docs/release-notes/.Language/preview.md

@Martin521 Martin521 force-pushed the fix-checkfile branch 2 times, most recently from 37e41ba to d3aee6e Compare September 1, 2024 06:10
@Martin521 Martin521 changed the title fix the checkFile bug Make the interaction between #line and #nowarn directives consistent Sep 1, 2024
@vzarytovskii vzarytovskii enabled auto-merge (squash) September 2, 2024 12:51
Martin521 added a commit to Martin521/FsLexYacc that referenced this pull request Sep 3, 2024
Copy link
Contributor

@edgarfgp edgarfgp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Martin.

auto-merge was automatically disabled September 5, 2024 14:42

Head branch was pushed to by a user without write access

@Martin521
Copy link
Contributor Author

Any volunteer for the second authorized review?

@vzarytovskii
Copy link
Member

A bunch of folks are off/away currently, will review it in few days

Copy link
Member

@KevinRansom KevinRansom left a 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.

src/Compiler/Driver/CompilerDiagnostics.fs Outdated Show resolved Hide resolved
@vzarytovskii vzarytovskii merged commit ac3d3c8 into dotnet:main Sep 10, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants