-
Notifications
You must be signed in to change notification settings - Fork 783
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
Warnings for invalid #nowarn arguments #17871
base: main
Are you sure you want to change the base?
Changes from all commits
35a4995
ba5e277
f210bfd
9d521fa
a85a29c
fba907a
d555e88
e564e51
d10b2b4
526b930
6ae40f7
87a95f1
79ff74c
9f63a5f
e69bd12
36b32c1
f4233e0
0f17316
f4c237a
c576d14
75b640a
c15706a
4e00d85
9496bae
3ac01a3
8bd4e67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1005,6 +1005,16 @@ let parsedHashDirectiveArguments (input: ParsedHashDirectiveArgument list) (lang | |
| false -> None) | ||
input | ||
|
||
let parsedHashDirectiveArgumentsNoCheck (input: ParsedHashDirectiveArgument list) (langVersion: LanguageVersion) = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like langversion is not used here, and we have broken the detection of unused symbols There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is called only for |
||
List.choose | ||
(function | ||
| ParsedHashDirectiveArgument.String(s, _, _) -> Some s | ||
| ParsedHashDirectiveArgument.SourceIdentifier(_, v, _) -> Some v | ||
| ParsedHashDirectiveArgument.Int32(n, m) -> Some(string n) | ||
| ParsedHashDirectiveArgument.Ident(ident, m) -> Some(ident.idText) | ||
| ParsedHashDirectiveArgument.LongIdent(ident, m) -> Some(longIdentToString ident)) | ||
input | ||
|
||
let parsedHashDirectiveStringArguments (input: ParsedHashDirectiveArgument list) (_langVersion: LanguageVersion) = | ||
List.choose | ||
(function | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,15 +24,20 @@ module NonStringArgs = | |
|> asExe | ||
|> compile | ||
|> shouldFail | ||
|> withDiagnostics[ | ||
|> withDiagnostics [ | ||
if languageVersion = "8.0" then | ||
(Warning 203, Line 6, Col 1, Line 6, Col 13, "Invalid warning number 'FS'") | ||
(Error 3350, Line 3, Col 9, Line 3, Col 11, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 4, Col 9, Line 4, Col 15, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 5, Col 9, Line 5, Col 13, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Warning 203, Line 6, Col 9, Line 6, Col 13, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 7, Col 9, Line 7, Col 17, """Invalid warning number '"FSBLAH"'"""); | ||
else | ||
(Warning 203, Line 3, Col 1, Line 3, Col 11, "Invalid warning number 'FS'") | ||
(Warning 203, Line 6, Col 1, Line 6, Col 13, "Invalid warning number 'FS'") | ||
(Warning 203, Line 3, Col 9, Line 3, Col 11, "Invalid warning number 'FS'"); | ||
(Warning 203, Line 4, Col 9, Line 4, Col 15, "Invalid warning number 'FSBLAH'"); | ||
(Warning 203, Line 5, Col 9, Line 5, Col 13, "Invalid warning number 'ACME'"); | ||
(Warning 203, Line 6, Col 9, Line 6, Col 13, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 7, Col 9, Line 7, Col 17, """Invalid warning number '"FSBLAH"'"""); | ||
(Warning 203, Line 8, Col 9, Line 8, Col 15, """Invalid warning number '"ACME"'""") | ||
] | ||
|
||
|
||
|
@@ -55,14 +60,20 @@ module NonStringArgs = | |
|> asExe | ||
|> compile | ||
|> shouldFail | ||
|> withDiagnostics[ | ||
|> withDiagnostics [ | ||
if languageVersion = "8.0" then | ||
(Warning 203, Line 2, Col 1, Line 9, Col 11, "Invalid warning number 'FS'") | ||
(Error 3350, Line 4, Col 5, Line 4, Col 7, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 5, Col 5, Line 5, Col 11, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 6, Col 5, Line 6, Col 9, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Warning 203, Line 7, Col 5, Line 7, Col 9, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 8, Col 5, Line 8, Col 13, """Invalid warning number '"FSBLAH"'"""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old compiler was pretty lax about what could be a warning prefix and what couldn't. For a refactoring the tests shouldn't really change. It looks like you have chosen to disallow all error prefix text other than FS. This will cause developers to have to fix places in their source and projects where nowarns include non FS prefixes. We would prefer them not to have to do to do that. Sometimes when the existing bugs are not terrible we leave them be, rather than causing everyone makework. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in the corresponding issue #17870, In the issue OP, I also proposed to add warnings for certain other (reasonable, I think) situations. If the decision is to not do that, I can remove it from both the issue and the PR. |
||
else | ||
(Warning 203, Line 2, Col 1, Line 9, Col 11, "Invalid warning number 'FS'") | ||
(Warning 203, Line 4, Col 5, Line 4, Col 7, "Invalid warning number 'FS'"); | ||
(Warning 203, Line 5, Col 5, Line 5, Col 11, "Invalid warning number 'FSBLAH'"); | ||
(Warning 203, Line 6, Col 5, Line 6, Col 9, "Invalid warning number 'ACME'"); | ||
(Warning 203, Line 7, Col 5, Line 7, Col 9, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 8, Col 5, Line 8, Col 13, """Invalid warning number '"FSBLAH"'"""); | ||
(Warning 203, Line 9, Col 5, Line 9, Col 11, """Invalid warning number '"ACME"'""") | ||
] | ||
|
||
|
||
|
@@ -81,12 +92,18 @@ module NonStringArgs = | |
|> shouldFail | ||
|> withDiagnostics [ | ||
if languageVersion = "8.0" then | ||
(Warning 203, Line 3, Col 1, Line 3, Col 44, "Invalid warning number 'FS'") | ||
(Error 3350, Line 3, Col 9, Line 3, Col 11, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 3, Col 12, Line 3, Col 18, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 3, Col 19, Line 3, Col 23, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Warning 203, Line 3, Col 24, Line 3, Col 28, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 3, Col 29, Line 3, Col 37, """Invalid warning number '"FSBLAH"'"""); | ||
else | ||
(Warning 203, Line 3, Col 1, Line 3, Col 44, "Invalid warning number 'FS'") | ||
(Warning 203, Line 3, Col 9, Line 3, Col 11, "Invalid warning number 'FS'"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We just underline the warning number now. I think that is an improvement |
||
(Warning 203, Line 3, Col 12, Line 3, Col 18, "Invalid warning number 'FSBLAH'"); | ||
(Warning 203, Line 3, Col 19, Line 3, Col 23, "Invalid warning number 'ACME'"); | ||
(Warning 203, Line 3, Col 24, Line 3, Col 28, """Invalid warning number '"FS"'"""); | ||
(Warning 203, Line 3, Col 29, Line 3, Col 37, """Invalid warning number '"FSBLAH"'"""); | ||
(Warning 203, Line 3, Col 38, Line 3, Col 44, """Invalid warning number '"ACME"'""") | ||
] | ||
|
||
|
||
|
@@ -128,12 +145,34 @@ module DoBinding = | |
(Warning 1104, Line 5, Col 15, Line 5, Col 31, "Identifiers containing '@' are reserved for use in F# code generation") | ||
(Error 3350, Line 2, Col 9, Line 2, Col 11, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Error 3350, Line 2, Col 12, Line 2, Col 18, "Feature '# directives with non-quoted string arguments' is not available in F# 8.0. Please use language version 9.0 or greater.") | ||
(Warning 203, Line 2, Col 26, Line 2, Col 34, """Invalid warning number '"FS3221"'""") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is "FS3221" invalid? it wasn't before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
] | ||
else | ||
compileResult | ||
|> shouldSucceed | ||
|
||
|
||
[<InlineData("8.0")>] | ||
[<InlineData("9.0")>] | ||
[<Theory>] | ||
let ``#nowarn - errors - compiler options`` (languageVersion) = | ||
|
||
FSharp """ | ||
match None with None -> () // creates FS0025 - ignored due to flag | ||
"" // creates FS0020 - ignored due to flag | ||
""" // creates FS0988 - not ignored, different flag prefix | ||
|> withLangVersion languageVersion | ||
|> withOptions ["--nowarn:NU0988"; "--nowarn:FS25"; "--nowarn:20"; "--nowarn:FS"; "--nowarn:FSBLAH"] | ||
|> asExe | ||
|> compile | ||
|> shouldFail | ||
|> withDiagnostics [ | ||
(Warning 203, Line 0, Col 1, Line 0, Col 1, "Invalid warning number 'FS'") | ||
(Warning 203, Line 0, Col 1, Line 0, Col 1, "Invalid warning number 'FSBLAH'") | ||
(Warning 988, Line 3, Col 3, Line 3, Col 3, "Main module of program is empty: nothing will happen when it is run") | ||
] | ||
|
||
|
||
[<InlineData("8.0")>] | ||
[<InlineData("9.0")>] | ||
[<Theory>] | ||
|
@@ -248,7 +287,7 @@ printfn "Hello, World" | |
|> asExe | ||
|> compile | ||
|> shouldFail | ||
|> withDiagnostics[ | ||
|> withDiagnostics [ | ||
(Error 76, Line 2, Col 9, Line 2, Col 11, "This directive may only be used in F# script files (extensions .fsx or .fsscript). Either remove the directive, move this code to a script file or delimit the directive with '#if INTERACTIVE'/'#endif'.") | ||
(Error 76, Line 3, Col 9, Line 3, Col 14, "This directive may only be used in F# script files (extensions .fsx or .fsscript). Either remove the directive, move this code to a script file or delimit the directive with '#if INTERACTIVE'/'#endif'.") | ||
(Error 76, Line 4, Col 9, Line 4, Col 17, "This directive may only be used in F# script files (extensions .fsx or .fsscript). Either remove the directive, move this code to a script file or delimit the directive with '#if INTERACTIVE'/'#endif'.") | ||
|
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 don't think regex makes the code easier to read.
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 agree that the regex language is not the most easy one to read. I felt that in this case the resulting conciseness offsets that disadvantage. But I have no problems replacing the regex matches by "manual" code, if that is preferred.
The key point here, however, is a different one. We have three different situations to deal with
#nowarn
directives in language version 9.0#nowarn
directives in earlier language versions--nowarn
compiler options (luckily not depending on the language version)Currently, the code for
is distributed in many places
(like here in CompilerOptions.fs (stripping the FS prefix for compiler options);
here in CompilerConfig.fs (stripping prefix, convert to int, warning invalid arg);
here in ParseAndCheckInputs.fs (check language version; ignore some invalid args);
here in SyntaxTreeOps.fs (conversion to string; check language version and possibly corresponding error);
here in pars.fsy (parsing into different types; possibly parse error))
This makes it very hard to reason about the #nowarn arguments. That's why in the course of working on "scoped nowarn" I felt the need to centralize this.