-
Notifications
You must be signed in to change notification settings - Fork 143
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
FS-1146-scoped-nowarn RFC proposal #782
Open
Martin521
wants to merge
17
commits into
fsharp:main
Choose a base branch
from
Martin521:FS-1146-scoped-nowarn
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
1916f9f
initial (almost) empty version
Martin521 046fbdf
First filled-in version, with some open items
Martin521 29d6110
First proposal
Martin521 953d1a3
added discussion link
Martin521 2e216ca
some typo and formatting corrections
Martin521 39ca4c7
Added a section on documentation
Martin521 744a036
Update (see discussion thread)
Martin521 f0c29bc
Merge remote-tracking branch 'upstream/main' into FS-1146-scoped-nowarn
Martin521 6344edc
More details on diagnostics
Martin521 0e4eee9
Merge remote-tracking branch 'upstream/main' into FS-1146-scoped-nowarn
Martin521 1398abf
fixed detail about diagnostics
Martin521 1bdf07a
compatibility and oddities
Martin521 771304e
more details on diagnostics and documentation
Martin521 8cb1306
added remarks on tooling
Martin521 2c3d707
updated PR link
Martin521 72ac047
updated PR link
Martin521 c079de4
added remark about `# nowarn`
Martin521 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
# F# RFC FS-1146-scoped-nowarn | ||
|
||
The design suggestion [278](https://github.com/fsharp/fslang-suggestions/issues/278) has been marked "approved in principle". | ||
|
||
This RFC covers the detailed proposal for this suggestion. | ||
|
||
- [x] [Suggestion](https://github.com/fsharp/fslang-suggestions/issues/278) | ||
- [x] Approved in principle | ||
- [ ] [Implementation](https://github.com/dotnet/fsharp/pull/17507) (draft / PoC for now) | ||
- [ ] Discussion (t.b.d.) | ||
|
||
# Summary | ||
|
||
Allow the #nowarn directive to become scoped (per warning number) by a corresponding #warnon directive. | ||
|
||
# Motivation | ||
|
||
Today, the #nowarn directive disables the specified warnings for all code from the directive to the end of the file. Usually, however, people want to see all warnings except for some very specific situations. Therefore, a way to disable warnings for some fragment of the code is a much-requested feature. | ||
|
||
Quotes form the language suggestion thread: | ||
|
||
- "being able to nowarn a few lines is most critical for my workplace" | ||
- "it would be huge if we could get this" | ||
- "it would be great to do only small sections that we know we want to ignore rather than the whole file" | ||
|
||
Motivating examples, again from the suggestion thread: | ||
|
||
``` | ||
// We know f is never called with a None. | ||
let f (Some a) = // creates warning 25, which we want to suppress | ||
// 2000 loc, where the incomplete match warning is beneficial | ||
``` | ||
|
||
``` | ||
// We have a union with an obsolete case | ||
type MyErrors = | ||
| CoolError1 of CoolError1 | ||
| CoolError2 of CoolError2 | ||
| [<Obsolete>] OldError1 of OldError1 | ||
|
||
let handler e = | ||
match e with | ||
| CoolError1 ce -> ... | ||
| CoolError2 ce -> ... | ||
| OldError ce -> ... // warning 44, which we want to suppress only here | ||
``` | ||
|
||
# Detailed specification | ||
|
||
1. The compiler shall recognize a new *compiler directive* (see §12.4 of the F# spec) `#warnon`. | ||
2. `#warnon` has warning numbers as arguments in the same way as `#nowarn` (including non-string arguments (RFC-1147)). | ||
3. Currently, `#nowarn` disables warnings from the directive to the end of the file. This will be changed as follows. | ||
If a certain warning has been disabled by a `#nowarn` directive, then it can be re-enabled by a `#warnon` directive later in the file, carrying the same warning number as an argument. So, the "WarningOff" code checking for that warning number is scoped from the `#nowarn` to the corresponding `#warnon`. | ||
4. If no corresponding `#warnon` is encountered for a `#nowarn` for a certain warning, the warning is disabled from the `#nowarn` to the end of the file. | ||
5. Multiple "WarningOff" scopes can be defined in a file by `#nowarn` / `#warnon` pairs. | ||
6. To use `#warnon` for a warning number without a previous `#nowarn` for that warning number is not allowed (error). | ||
7. A `#nowarn` directive without arguments is not allowed (error). | ||
8. `--nowarn` compiler flags are not considered for processing `#warnon` directives. This means that for a warning number `n` that is disabled by a compiler flag | ||
|
||
a) `#warnon` for `n` without previous `#nowarn` for `n` is an error. | ||
|
||
b) A `#warnon` for `n` after a `#nowarn` for `n` enables the warning for the rest of the file (or the next `#nowarn` for `n`). | ||
|
||
9. The current definition "*Compiler directives are declarations in non-nested modules or namespace declaration groups*" (§12.4) shall be relaxed | ||
|
||
> Note: *This will be detailed after draft implementation* | ||
|
||
10. Indentation rules (not defined in the spec, except for (obsolete) no-indentation of conditional compilation directives) shall be relaxed. | ||
|
||
> Note: *This will be detailed after draft implementation* | ||
11. For scripts and interactive fsi, the same rules shall apply. | ||
12. For the language service, the same rules shall apply. | ||
|
||
> Note: *This is currently not the case. Currently, any `#nowarn` for a warning number will disable the warning in the whole file.* | ||
|
||
> Note: *This will be detailed after draft implementation* | ||
|
||
### Examples: | ||
|
||
``` | ||
module A | ||
match None with None -> () // warning | ||
#nowarn 25 | ||
match None with None -> () // no warning | ||
#warnon 25 | ||
match None with None -> () // warning | ||
#nowarn 25 | ||
match None with None -> () // no warning | ||
``` | ||
|
||
``` | ||
//TODO: example covering items 9 and 10 of the spec | ||
``` | ||
|
||
# Drawbacks | ||
|
||
Again more logic to maintain in the compiler. | ||
|
||
# Alternatives | ||
|
||
A number of different design have been discussed in the language suggestion, but the discussion converged on the current one. | ||
|
||
# Compatibility | ||
|
||
This is not a breaking change for the language. | ||
|
||
Technically, it is a breaking change for the F# compiler, because the compiler swallows unknown compiler directives without error or warning (which btw one might consider a bug). So, if somebody has used `#warnon` "illegally" after a `nowarn` AND relies on NOT getting a warning, they will be suprised to get a warning with the new compiler. Probably a scenario that can be neglected. | ||
|
||
Since previous versions of the compiler ignore unknown compiler directives, they continue to work as before. | ||
|
||
Since warnings are a compile time feature, there is no binary compatibility issue. | ||
|
||
|
||
# Pragmatics | ||
|
||
## Diagnostics | ||
|
||
Errors and Warnings on misuse of the feature are mentioned in the detailed specification above. | ||
|
||
## Tooling | ||
|
||
No specific tooling change is expected. | ||
(Except for checking regarding item 12 of the spec.) | ||
|
||
## Performance and Scaling | ||
|
||
No performance and scaling impact is expected. | ||
|
||
## Culture-aware formatting/parsing | ||
|
||
There is no interaction with culture-aware formatting and parsing of numbers, dates and currencies. | ||
|
||
# Unresolved questions | ||
|
||
The items with notes in the detailed specification above are still to be detailed. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I understand the perspective of protecting against user mistakes, by calling it an error.
But I do not like the UX for the following scenario:
Would the recommendation be to use:
--nowarn 123
on project level#nowarn 123
in top of the file#warnon 123
right below it ?I am wondering if the repetitive
#nowarn
would be needed.The F# compilation could be changed to allow the following pairs:
#nowarn
and#warnon
--nowarn
and a file level#warnon
And produce an error in remaining cases
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.
@T-Gro Thanks for the feedback. This RFC is still in an early phase and therefore marked draft.
My plan was to get a bit further in understanding the implications of these items on the implementation and then open a discussion thread for this RFC.
If it is ok for you I would wait with a response until then.
Edit: I think the current proposal has dealt with that concern now.