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

Heads-up: const_err lint is going away #68

Closed
RalfJung opened this issue Sep 23, 2022 · 3 comments · Fixed by #71
Closed

Heads-up: const_err lint is going away #68

RalfJung opened this issue Sep 23, 2022 · 3 comments · Fixed by #71
Assignees

Comments

@RalfJung
Copy link

This crate carries a allow(const_err). That lint is going away since it becomes a hard error, which causes a warning due to the removed lint being used, which then triggers deny(warnings).

The crate does not actually seem to trigger const_err (according to crater), so the hard error itself should not be a problem. The allow(const_err) can likely just be removed.

@hkratz
Copy link
Contributor

hkratz commented Sep 24, 2022

The problem is that a false-positive error is triggered with the MSRV 1.38.0, see CI log from draft PR #69.

cargo-bisect-rustc tells me that this was fixed in nightly-2020-02-21 (regression = success). So I can remove allow(const_err) but then I need to bump the MSRV to 1.43.0.

********************************************************************************
Regression in nightly-2020-02-21
********************************************************************************

fetching https://static.rust-lang.org/dist/2020-02-20/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2020-02-20: 40 B / 40 B [===================================] 100.00 % 275.82 KB/s converted 2020-02-20 to 7760cd0fbbbf2c59a625e075a5bdfa88b8e30f8a
fetching https://static.rust-lang.org/dist/2020-02-21/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2020-02-21: 40 B / 40 B [===================================] 100.00 % 257.34 KB/s converted 2020-02-21 to 2c462a2f776b899d46743b1b44eda976e846e61d
looking for regression commit between 2020-02-20 and 2020-02-21
opening existing repository at "/Users/hans/dev/rust"
Found origin remote under name `origin-https`
refreshing repository at "/Users/hans/dev/rust"
fetching (via local git) commits from 7760cd0fbbbf2c59a625e075a5bdfa88b8e30f8a to 2c462a2f776b899d46743b1b44eda976e846e61d
opening existing repository at "/Users/hans/dev/rust"
Found origin remote under name `origin-https`
refreshing repository at "/Users/hans/dev/rust"
looking up first commit
looking up second commit
checking that commits are by bors and thus have ci artifacts...
finding bors merge commits
found 8 bors merge commits in the specified range
  commit[0] 2020-02-19UTC: Auto merge of #69293 - Dylan-DPC:rollup-imcbvgo, r=Dylan-DPC
  commit[1] 2020-02-19UTC: Auto merge of #68988 - Zoxc:query-caches, r=eddyb
  commit[2] 2020-02-20UTC: Auto merge of #69256 - nnethercote:misc-inlining, r=Centril
  commit[3] 2020-02-20UTC: Auto merge of #67925 - petertodd:2020-fromstr-infallible, r=LukasKalbertodt
  commit[4] 2020-02-20UTC: Auto merge of #68847 - ecstatic-morse:const-impl, r=oli-obk
  commit[5] 2020-02-20UTC: Auto merge of #69309 - Dylan-DPC:rollup-gjdqx7l, r=Dylan-DPC
  commit[6] 2020-02-20UTC: Auto merge of #69145 - matthewjasper:mir-typeck-static-ty, r=nikomatsakis
  commit[7] 2020-02-20UTC: Auto merge of #69325 - Centril:rollup-vce2ko2, r=Centril
ERROR: no CI builds available between 7760cd0fbbbf2c59a625e075a5bdfa88b8e30f8a and 2c462a2f776b899d46743b1b44eda976e846e61d within last 167 days

@hkratz hkratz self-assigned this Sep 24, 2022
@RalfJung
Copy link
Author

RalfJung commented Sep 24, 2022

Oh interesting, I didn't know/remember we had such false positives in the past. I hope they are fixed now (but I assume we would have heard about them otherwise^^).

You could also remove the deny(warnings) and instead set RUSTFLAGS="-D warnings -A const_err" on CI only for the MSRV build (and -D warnings for everything else), then the
'const_err lint has been removed' warning won't cause a build failure.

(Anyway your users are fine since all lints are capped to warnings for dependencies. Still AFAIK the usual advice is not to put deny(warnings) into the code.)

@RalfJung
Copy link
Author

That nightly was the first to ship with rust-lang/rust#69185, which split some things out of const_err into new lints unconditional_panic and arithmetic_overflow. So it probably didn't remove the false positive, just shifted it to a different lint.

Looking at the code, this is indeed an unconditional panic, but inside dead code. This is another instance of the same issue. It is currently by-design and hard to fix but indeed somewhat unfortunate (and might be an argument for making the lint warn-by-default).

Anyway I just wanted to make sure there is not some subtle bug in our diagnostics here that I did not know about. :)

@hkratz hkratz closed this as completed in #71 Jan 8, 2023
hkratz added a commit that referenced this issue Jan 8, 2023
We can just use `.get_unsafe()` here. Drive-by-fix: clippy warning. Drive-by-fix: clippy warning.

Fixes #68.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants