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

implement --warningAsError:on and enforce it for compiler sources; fix some bugs with warningAsError:foo:on|off #14068

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 22, 2020

before PR:

  • warningAsError had some bugs, for example nim c -r --warningAsError:Deprecated:on hello.nim would crash without any error or warning shown
  • PR's keep introducing new warnings regularly and it goes un-noticed since CI doesn't check for it

after PR

  • that bug is fixed
  • semantics are now as follows:
    • --warningAsError:foo:on means enable warning and turn it into error
    • --warningAsError:foo:off means do not error for that warning but don't disable the warning
  • --warningAsError:on|off is now implemented, with semantics:
    • --warningAsError:on turns all warnings into errors but doesn't enable (nor disable) any warning
    • --warningAsError:off turns all warnings into non-errors but doesn't enable (nor disable) any warning
  • fix a pre-existing warning that was recently introduced in some recent PR
  • enable --warningAsError:on for compiler sources; more sources could follow later. Errors can be selectively disabled:

the flags combine in the way you'd expect, so that --warningAsError:on --warningAsError:foo:off can be used to turn all enabled warnings into errors except for foo.

note

I'm also incrementing NimPatch so user code can simply branch via when (NimMajor, NimMinor, NimPatch) >= version:. ./compiler/config.nims can't use that because of bootstrapping, but will be able to once querySetting reports the (NimMajor, NimMinor, NimPatch) it was compiled against

@timotheecour timotheecour force-pushed the pr_warning_as_error_all branch 2 times, most recently from f4fc026 to 018132d Compare April 22, 2020 13:37
@Araq
Copy link
Member

Araq commented Apr 22, 2020

But "turn all warnings into errors" is a misfeature that I won't accept.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 22, 2020

virtually every language I've used have that feature, and for good reasons.

C: -Werror
C++: -Werror
D: -w
java: -Werror
C#: -warnaserror
swift: -warnings-as-errors
rust: #![deny(warnings)]
go: only errors, no warnings (which is bad but fits with their minimalistic goal)

even non statically compiled languages have that feature for library code:

python: error (in warnings library)
ruby: you can override warnings to raise, eg https://stackoverflow.com/a/56502595/1426932

It's not a misfeature, warnings are so easy to ignore (who's looking at warnings in CI?).

--warningAsError:on is implemented so that:

  • it only affects warnings that are already turned on
  • it only affects warnings in the main project (not dependencies), a consequence of previous point
  • it can be selectively disabled

if nim compiler introduces a new warning WarnBar that breaks package Foo's CI that has --warningAsError:on, that package can choose to fix the warning (there's a good reason for the warning in the first place, eg unsafe code being flagged etc), or delay this by simply adding --warningAsError:WarnBar:off to its CI, to selectively disable just that warning.

This feature is meant for CI (whether in nim's CI or other nimble packages's CI), and helps phasing out deprecated features faster, while avoiding introducing regressions. A PR that makes a change that causes a new warning-as-error (eg unused module import) can and should fix it in the same PR, or if that's technically difficult, selectively disable that warning-as-error.

For example, testament/testament r nonexistantfile.nim merely prints [Warning] - nonexistantfile.nim test does not exist instead of error. This actually caused a recent regression introduced since #13798 to be un-noticed, which is that -d:nimHasLibFFI stopped being tested. I'm fixing that in another PR.

@Araq
Copy link
Member

Araq commented Apr 22, 2020

I don't mind turning warnings into errors, I mind blindlessly turning all warnings into errors. This switch will end up in somebody's build, we add a new warning, yay, we break somebody's build. That's exactly what we didn't want to do.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 22, 2020

The fix is trivial, it's --warningAsError:on --warningAsError:NewWarn:off, and --warningAsError:on is opt-in. If needed I can add a disclaimer in nim --fullhelp explaining the caveat.

What you're suggesting is not very practical, users would have to blacklist all existing warnings in their CI if they want to keep their code warning free in CI. That list would not only become stale, but they'll be unaware that some new warning is introduced, ie, miss an opportunity to fix a security issue etc.

However, I think the following suggestion should remove any remaining concern:

--warningAsError:Foo:on|off #same
--warningAsError:on|off  #same
--warningAsError:1.3.2 # turns all enabled warnings (introduced as of nim version 1.3.2) into errors 

so compiler sources and code nimble projects that WANT to break-and-fix because of new compiler warnings can use --warningAsError:on, and nimble projects that want to not break-and-fix can use --warningAsError:x.y.z with whatever version of nim they've last tested on, eg --warningAsError:1.2

@disruptek
Copy link
Contributor

I can see how the versioned, uh, version could work, but I cannot see how the on version could ever work, and for the same reason @Araq pointed out. It cannot be allowed to live.

@Araq
Copy link
Member

Araq commented Apr 23, 2020

--warningAsError:1.3.2 is good, let's please have that. -1 to the additional on/off/maybe switches and their complexity.

@stale
Copy link

stale bot commented Jan 20, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jan 20, 2022
@stale stale bot closed this Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants