[WIP]Turn unused value warnings on by default

[cartermp]

Based on comment from #10457

Also worth noting that a lot of F# developers turn this one on explicitly

[cartermp]

Huh. So like, does NoWarn just like, not work?

[vzarytovskii]

@cartermp

The issue is that NoWarn has no effect when the warning is in WarningsAsErrors - when we use NoWarn, we add the warning to the tcConfigB.errorSeverityOptions.WarnOff, and then we process WarningsAsErrors, we add the same warning to tcConfigB.errorSeverityOptions.WarnAsError, and this is how we check whether we want to produce a warning.

let ReportWarningAsError options err =
    warningOn err options.WarnLevel options.WarnOn &&
    not (List.contains (GetDiagnosticNumber err) options.WarnAsWarn) &&
    ((options.GlobalWarnAsError && not (List.contains (GetDiagnosticNumber err) options.WarnOff)) ||
     List.contains (GetDiagnosticNumber err) options.WarnAsError)

Which, in our case evaluates to

<doesnt really matter what left evaluates to> || true // Since 1182 is in WarnAsError by default via the FSharpBuild.Directory.build.props

I've removed 1182 from WarningsAsErrors, since it's enabled by default now, and we treat all warnings as errors. Let me know if there are any objections.

CC @KevinRansom @brettfo

[vzarytovskii]

Some tests are failing, need to add some NoWarns to generated fsprojs (or fix unused vars).

[dsyme]

@vzarytovskii Let's discuss this

• 1182 should not be on for F# scripts. It's too intrusive. So this would need to be adjusted for that

• However if we want it on by default I think a simpler option is to make it on in the default templates for new projects? So much less intrusive, and then the user has control.

• If we changed it in the compiler I think it should be protected by a langversion switch - the very many changes to tests in this PR indicates how intrusive it is in the language

• I think we'd need to improve our error message to mention "you can add _ if you want to suppress this"

[vzarytovskii]

@vzarytovskii Let's discuss this

• 1182 should not be on for F# scripts. It's too intrusive. So this would need to be adjusted for that
• However if we want it on by default I think a simpler option is to make it on in the default templates for new projects? So much less intrusive, and then the user has control.
• If we changed it in the compiler I think it should be protected by a langversion switch - the very many changes to tests in this PR indicates how intrusive it is in the language
• I think we'd need to improve our error message to mention "you can add _ if you want to suppress this"

Yeah, the more tests I "fix", the more I think it may be not what we want to enforce via the compiler.

[dsyme]

Yeah, the more tests I "fix", the more I think it may be not what we want to enforce via the compiler.

Right. I mean I think it's ok for fresh project code.

[vzarytovskii]

Yeah, the more tests I "fix", the more I think it may be not what we want to enforce via the compiler.

Right. I mean I think it's ok for fresh project code.

Yeah, we can enable it in the templates by default, I guess.

[cartermp]

I'll just close this out since it was so long ago