Enable FS as prefix and ignore invalid values for warnings #3631

KevinRansom · 2017-09-23T07:14:37Z

So ... this fixes: #3626

--warnaserror[+|-]:<warn;...>     Report specific warnings as errors
--nowarn:<warn;...>                   Disable specific warning messages
--warnon:<warn;...>                   Enable specific warnings that may be off by default

These all accept a list of integer values as the warning id.

The C# compiler allows ID's with the format 'CS0001' to identify warnings, it also ignores any values it doesn't recognize. This allows other tools to add their own nowarn setting to the msbuild property.

E.g.

<NoWarn>NU1069;FS0040</NoWarn>

This change

Allows the developer to specify valid warnings either as an integer or FS followed by an integer.
If the value does not start with FS or is an integer then it is discarded.
Add some more tests.

realvictorprm · 2017-09-23T07:36:23Z

Sorry but couldn't this break maybe existing stuff?

0x53A · 2017-09-23T10:51:59Z

src/fsharp/CompileOps.fs

-    with err -> 
-        warning(Error(FSComp.SR.buildInvalidWarningNumber(s), m))
+        // Trim off leading "FS" to allow ""/warnon:FS0001;FS0002;0003; anything else we ignore
+        let number = if s.StartsWith("FS", StringComparison.InvariantCulture) = true then s.Substring(2) else s


so this will detect FSxxx, but ignore fsxxx, ~~similar to~~ exactly like C#?
I'm all for C# compatibility, but we don't need to duplicate their bugs...

To be completely honest:

It is fine to accept "FS" or even "fs" prefix

But the compiler should still warn when it gets invalid values like NU1234 those need to be filtered in the SDK

What they have done with the C# compiler is wrong and feels more like a quick hack...

But @matthid don't forget it's so "convenient" to just hijack things.

@matthid the C# compiler thing may be a bug but it's not a hack. I expect it does what it does because the original native compiler did it this way. That's usually why it does unexpected things.

I have some sympathy with the splitting NU0000 in the targets file rather than the compiler, although there is a lot of benefit in just using the same approach as C#, it means if you learn something for one language it works the same in another.

@forki sometimes it is convenient to extend a generalization. The property in a build file doesn't specify only do it for F# compiler invocations. It's not unreasonable to make more of the build tools aware of it. If the F# compiler had behaved similarly to C# originally, no one would have mentioned it.

0x53A · 2017-09-23T11:14:18Z

src/fsharp/CompileOps.fs

    try 
-        Some (int32 s)
-    with err -> 
-        warning(Error(FSComp.SR.buildInvalidWarningNumber(s), m))


It now also no longer warns for invalid integers, or an invalid FSxxx, right?

@0x53A Correct ... There is not really the notion of an invalid FSxxx ... either the value was an integer or it failed. We did not check that the integer mapped to an actual error or warning number.

We did not check that the integer mapped to an actual error or warning number.

Ah, that's what I missed. Thanks.

0x53A · 2017-09-23T11:22:17Z

Even ignoring nuget, I am in favor of ignoring warnings with prefixes the compiler doesn't recognize.

One use-case would be a common "Directory.build.props" for a solution mixing C# and F#.

There I could set <NoWarn>CS001;FS0040;CS123</NoWarn> and neither csc nor fsc would complain.

I'm torn on whether it should also ignore all warnings numbers which don't (yet) exists. For example, assuming there is no warning FS1234567 (I didn't check), should it warn on --nowarn:1234567 or --nowarn:FS1234567? Previously it did. If I read the code correctly, it now doesn't.

anyway, that's my 2 pence ;-)

matthid · 2017-09-23T11:25:46Z

Even ignoring nuget, I am in favor of ignoring warnings with prefixes the compiler doesn't recognize.
One use-case would be a common "Directory.build.props" for a solution mixing C# and F#.
There I could set CS001;FS0040;CS123 and neither csc nor fsc would complain.

How is that even related? IMHO the parameters have to be transformed already to have --nowarn: prefix, why not filter at the same place in the SDK?

0x53A · 2017-09-23T11:27:36Z

How is that even related? IMHO the parameters have to be transformed already to have --nowarn: prefix, why not filter at the same place in the SDK?

Yes, my comment was more targeted at the end-user experience, and not at how this experience is achieved. Filtering in the sdk would obviously also work.

matthid · 2017-09-23T11:43:01Z

Anyway, there are more important battles to fight than this.
Just saying that "features" like this make it harder to find subtle problems and it will hit someone sooner or later and cost a lot of hours to find...

For example assume how some mistake in the tooling generates --nowarn:--otherflag now --otherflag is ignored silently without warning. In the worst case the code still compiles but not as expected.

KevinRansom · 2017-09-23T16:55:01Z

@realvictorprm . No it is not breaking. Working project files will continue to work exactly as before, unless I did something wrong.

* Generate source for .resx files on build. (#3607) * add build task to generate *.fs from *.resx files * generate source for embedded resources in tests * generate source for embedded resources in FSharp.Editor * generate source for embedded resources in FSharp.LanguageService * generate source for embedded resources in FSharp.ProjectSystem.FSharp * generate source for embedded resources in FSharp.VS.FSI * don't generate non-string resources when <=netstandard1.6 * update baseline error message for tests The error output should be the exception message, not the exception type. * perform up-to-date check before generating *.fs from *.resx * remove non-idiomatic fold for an array comprehension * correct newline replacement * output more friendly error message * throw if boolean value isn't explicitly `true` or `false` * only generate object resource code on non `netstandard1.*` and `netcoreapp1.*` platforms * ensure FSharp.Core specifies a target framework for resource generaton * rename attributes to be non-ambiguous and properly include them * fix order of file items in FSharp.Core * Fix build.cmd for certain always-shown errors (like "unable to find registry key"), improve finding of VS2017 tools, fix DEVGUIDE.md (#3635) * Fix build.com displaying the following error on each run "ERROR: The system was unable to find the specified registry key or value." * Remove warning about reg.exe errors introduced in #3614 (in commit b548bd7, but unrelated to that fix), it is no longer necessary. * Fix #3638, VS2017 Preview installation was not found when VS2017RTM is not installed. Remove comment introduced in #3614 (through commit 966bd7f) * Fixing JaroWinkler tests with InvariantCulture and fixing async tests by removing Debugger.Break() (#3627) * Fixing JaroWinkler tests to use InvariantCulture for number-to-string * Fixing the crashing of test runners because of a Debugger.Break() in a test * update to System.Collections.Immutable 1.3.1 (#3641) * update to System.Collections.Immutable 1.3.1 * fixes * fix assembly reference * [WIP] Adds optimized emit for int64 constants (#3620) * Adds optimized emit for int64 constants. * Adds comment linking to the changing PR. Thanks for this PR. Kevin * fix assembly reference (#3646) * Remove a few more build warnings (#3647) * fix assembly reference * remove more build warnings * fix build * move BuildFromSource projects to their own directory * Adds tests for emitted IL for new Int64 constants. (#3650) * Enable FS as prefix and ignore invalid values for warnings (#3631) * enable fs as prefix and ignore invalid values for warnings + tests * Allow #pragma to validate warnings * do it right * use ordinal compare * In both places * Add fs prefix to warnaserror * Fixup tests * Fix stack overflow on assembly resolution (#3658) * Fix stack overflow on tp assembly resolution * Feedback * Add impl files to file check results (#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (#3657) * bump FCS version (#3676) * bump version * Update RELEASE_NOTES.md * Parsing improvements: no reactor, add parsing options, error severity options (#3601) * Parse without reactor, add parsing options, error severity options * Revert parsing APIs (fallback to the new ones), fix VFT projects * Cache parse results after type check * Add impl files to file check results (#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (#3657) * bump FCS version (#3676) * bump version * Update RELEASE_NOTES.md * updates to make tests pass * restore old behaviour of CheckFileInProjectAllowingStaleCachedResults (builder had been created by ParseFileInProject) * restore use of CheckFileInProjectAllowingStaleCachedResults * deprecate test relying on whacky behaviour of deprecated GetCheckResultsBeforeFileInProjectEvenIfStale * Use ParseFile and FSharpParsingOptions instead of ParseFileInProject * prepare FCS release with this feature * whitespace cleanup (#3682) * whitespace and docs (#3684) * Preserve XML docs for in-memory project references (#3683) * fix xmldocs for in-memory project references * add test * fix tests * whitespace and comments (#3686) * fix assembly reference * whitespace and comments * whitespace and comments * whitespace and comments * cherry pick two PRs from FCS (#3687) * fix assembly reference * remove line endings from all *.nuspec files * ProjectCracker returns *.fsi files in FSharpProjectOptions.SourceFiles array (in addition to *.fs files, in right order) * ProjectCracker raises exception if ProjectCrackerTool returns non null ProjectCrackerOptions.Error (new field) * fix build on linux * fix a test * slashes * revert slashes * Update FSharp.Compiler.Service.ProjectCracker.nuspec * try to fix travis * try to fix travis * list dependencies * no obsolete pdb in nuget * list dependencies * cherry pick of fsharp/fsharp change * bump FCS version number (#3688) * Update FSharp.Compiler.Service.MSBuild.v12.nuspec * fix FCS nuget on windows * fix-resource (#3690) * Bump FSharp.Compiler.Tools to 4.1.27 and align mono build files (#3693) * ri change from fsharp * fix test * bump FSC tools to 4.1.27 * remove fsharp.core from Mono GAC * align mono directory * fix typo * install back versions with Mono * fix typo * update FCS doc generation (#3694) * update DEVGUIDE to add addiitional steps before running build (#3725) * Split templates out into a seperate vsix (#3720) * merge error * Merge issues

* Generate source for .resx files on build. (dotnet#3607) * add build task to generate *.fs from *.resx files * generate source for embedded resources in tests * generate source for embedded resources in FSharp.Editor * generate source for embedded resources in FSharp.LanguageService * generate source for embedded resources in FSharp.ProjectSystem.FSharp * generate source for embedded resources in FSharp.VS.FSI * don't generate non-string resources when <=netstandard1.6 * update baseline error message for tests The error output should be the exception message, not the exception type. * perform up-to-date check before generating *.fs from *.resx * remove non-idiomatic fold for an array comprehension * correct newline replacement * output more friendly error message * throw if boolean value isn't explicitly `true` or `false` * only generate object resource code on non `netstandard1.*` and `netcoreapp1.*` platforms * ensure FSharp.Core specifies a target framework for resource generaton * rename attributes to be non-ambiguous and properly include them * fix order of file items in FSharp.Core * Fix build.cmd for certain always-shown errors (like "unable to find registry key"), improve finding of VS2017 tools, fix DEVGUIDE.md (dotnet#3635) * Fix build.com displaying the following error on each run "ERROR: The system was unable to find the specified registry key or value." * Remove warning about reg.exe errors introduced in dotnet#3614 (in commit b548bd7, but unrelated to that fix), it is no longer necessary. * Fix dotnet#3638, VS2017 Preview installation was not found when VS2017RTM is not installed. Remove comment introduced in dotnet#3614 (through commit 966bd7f) * Fixing JaroWinkler tests with InvariantCulture and fixing async tests by removing Debugger.Break() (dotnet#3627) * Fixing JaroWinkler tests to use InvariantCulture for number-to-string * Fixing the crashing of test runners because of a Debugger.Break() in a test * update to System.Collections.Immutable 1.3.1 (dotnet#3641) * update to System.Collections.Immutable 1.3.1 * fixes * fix assembly reference * [WIP] Adds optimized emit for int64 constants (dotnet#3620) * Adds optimized emit for int64 constants. * Adds comment linking to the changing PR. Thanks for this PR. Kevin * fix assembly reference (dotnet#3646) * Remove a few more build warnings (dotnet#3647) * fix assembly reference * remove more build warnings * fix build * move BuildFromSource projects to their own directory * Adds tests for emitted IL for new Int64 constants. (dotnet#3650) * Enable FS as prefix and ignore invalid values for warnings (dotnet#3631) * enable fs as prefix and ignore invalid values for warnings + tests * Allow #pragma to validate warnings * do it right * use ordinal compare * In both places * Add fs prefix to warnaserror * Fixup tests * Fix stack overflow on assembly resolution (dotnet#3658) * Fix stack overflow on tp assembly resolution * Feedback * Add impl files to file check results (dotnet#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (dotnet#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (dotnet#3657) * bump FCS version (dotnet#3676) * bump version * Update RELEASE_NOTES.md * Parsing improvements: no reactor, add parsing options, error severity options (dotnet#3601) * Parse without reactor, add parsing options, error severity options * Revert parsing APIs (fallback to the new ones), fix VFT projects * Cache parse results after type check * Add impl files to file check results (dotnet#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (dotnet#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (dotnet#3657) * bump FCS version (dotnet#3676) * bump version * Update RELEASE_NOTES.md * updates to make tests pass * restore old behaviour of CheckFileInProjectAllowingStaleCachedResults (builder had been created by ParseFileInProject) * restore use of CheckFileInProjectAllowingStaleCachedResults * deprecate test relying on whacky behaviour of deprecated GetCheckResultsBeforeFileInProjectEvenIfStale * Use ParseFile and FSharpParsingOptions instead of ParseFileInProject * prepare FCS release with this feature * whitespace cleanup (dotnet#3682) * whitespace and docs (dotnet#3684) * Preserve XML docs for in-memory project references (dotnet#3683) * fix xmldocs for in-memory project references * add test * fix tests * whitespace and comments (dotnet#3686) * fix assembly reference * whitespace and comments * whitespace and comments * whitespace and comments * cherry pick two PRs from FCS (dotnet#3687) * fix assembly reference * remove line endings from all *.nuspec files * ProjectCracker returns *.fsi files in FSharpProjectOptions.SourceFiles array (in addition to *.fs files, in right order) * ProjectCracker raises exception if ProjectCrackerTool returns non null ProjectCrackerOptions.Error (new field) * fix build on linux * fix a test * slashes * revert slashes * Update FSharp.Compiler.Service.ProjectCracker.nuspec * try to fix travis * try to fix travis * list dependencies * no obsolete pdb in nuget * list dependencies * cherry pick of fsharp/fsharp change * bump FCS version number (dotnet#3688) * Update FSharp.Compiler.Service.MSBuild.v12.nuspec * fix FCS nuget on windows * fix-resource (dotnet#3690) * Bump FSharp.Compiler.Tools to 4.1.27 and align mono build files (dotnet#3693) * ri change from fsharp * fix test * bump FSC tools to 4.1.27 * remove fsharp.core from Mono GAC * align mono directory * fix typo * install back versions with Mono * fix typo * update FCS doc generation (dotnet#3694) * update DEVGUIDE to add addiitional steps before running build (dotnet#3725) * Split templates out into a seperate vsix (dotnet#3720) * merge error * Merge issues

enable fs as prefix and ignore invalid values for warnings + tests

c7b461d

msftclas added the cla-already-signed label Sep 23, 2017

0x53A reviewed Sep 23, 2017

View reviewed changes

KevinRansom added 5 commits September 25, 2017 18:37

Allow #pragma to validate warnings

5c390c3

do it right

f9aa24e

use ordinal compare

89b4cec

In both places

fa25d38

Add fs prefix to warnaserror

a4c52e9

msftgits removed the cla-already-signed label Sep 26, 2017

Fixup tests

a8d75f6

dsyme approved these changes Sep 27, 2017

View reviewed changes

KevinRansom merged commit 780b6c4 into dotnet:master Sep 27, 2017

abelbraaksma mentioned this pull request May 22, 2024

Allow #nowarn to support the FS prefix on error codes to disable warnings #17209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable FS as prefix and ignore invalid values for warnings #3631

Enable FS as prefix and ignore invalid values for warnings #3631

KevinRansom commented Sep 23, 2017

realvictorprm commented Sep 23, 2017

0x53A Sep 23, 2017 •

edited

Loading

matthid Sep 23, 2017

forki Sep 23, 2017

KevinRansom Sep 23, 2017

0x53A Sep 23, 2017

KevinRansom Sep 23, 2017

0x53A Sep 23, 2017

0x53A commented Sep 23, 2017 •

edited

Loading

matthid commented Sep 23, 2017

0x53A commented Sep 23, 2017

matthid commented Sep 23, 2017

KevinRansom commented Sep 23, 2017

Enable FS as prefix and ignore invalid values for warnings #3631

Enable FS as prefix and ignore invalid values for warnings #3631

Conversation

KevinRansom commented Sep 23, 2017

realvictorprm commented Sep 23, 2017

0x53A Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

matthid Sep 23, 2017

Choose a reason for hiding this comment

forki Sep 23, 2017

Choose a reason for hiding this comment

KevinRansom Sep 23, 2017

Choose a reason for hiding this comment

0x53A Sep 23, 2017

Choose a reason for hiding this comment

KevinRansom Sep 23, 2017

Choose a reason for hiding this comment

0x53A Sep 23, 2017

Choose a reason for hiding this comment

0x53A commented Sep 23, 2017 • edited Loading

matthid commented Sep 23, 2017

0x53A commented Sep 23, 2017

matthid commented Sep 23, 2017

KevinRansom commented Sep 23, 2017

0x53A Sep 23, 2017 •

edited

Loading

0x53A commented Sep 23, 2017 •

edited

Loading