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

Warnings for invalid #nowarn arguments #17871

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

Martin521
Copy link
Contributor

@Martin521 Martin521 commented Oct 11, 2024

Description

Fixes #17870
Also centralizes the distributed logic for processing #nowarn arguments, for better maintainability.

Checklist

  • Test cases added
  • Release notes entry updated

Copy link
Contributor

github-actions bot commented Oct 11, 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.200.md

@Martin521 Martin521 marked this pull request as ready for review October 13, 2024 13:18
@Martin521 Martin521 requested a review from a team as a code owner October 13, 2024 13:18
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.

a few comments inline

let rxOptions = RegexOptions.CultureInvariant

let getNumber regexString failAction =
let mtch = Regex(regexString, rxOptions).Match(numStr)
Copy link
Member

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.

Copy link
Contributor Author

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

  1. Detecting the situation
  2. Parsing the argument (differently for the 3 situations)
  3. Detecting the need for a warning (differently for the 3 situations)

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.

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'");
Copy link
Member

Choose a reason for hiding this comment

The 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

@@ -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"'""")
Copy link
Member

Choose a reason for hiding this comment

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

Why is "FS3221" invalid? it wasn't before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the corresponding issue #17870, #nowarn "FSxxxx" (any nowarn argument with FS prefix) created a warning in F# 8.0. This warning got lost in #17206. I am bringing it back here for language version 8.0 and lower.

(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"'""");
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the corresponding issue #17870, #nowarn "FSxxxx" (any nowarn argument with FS prefix) created a warning in F# 8.0. This warning got lost in #17206. I am bringing it back here for language version 8.0 and lower.

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.

@@ -1005,6 +1005,16 @@ let parsedHashDirectiveArguments (input: ParsedHashDirectiveArgument list) (lang
| false -> None)
input

let parsedHashDirectiveArgumentsNoCheck (input: ParsedHashDirectiveArgument list) (langVersion: LanguageVersion) =
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called only for #nowarn hash directives, for which the arguments are checked in GetWarningNumber.

@Martin521 Martin521 marked this pull request as draft October 18, 2024 08:39
@Martin521 Martin521 marked this pull request as ready for review October 24, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

warnings for invalid #nowarn arguments
4 participants