Use true previous lint level when detecting overriden forbids #78864
Conversation
rust-highfive
added
the
S-waiting-on-review
Mark-Simulacrum
added
I-nominated
T-lang
|
I am nominating this for lang discussion, as we had wanted to run an FCP on this. The PR description contains a description of the new behavior; it essentially matches #77713 (comment), though that had not really made clear that the behavior for nested scopes here also changes, avoiding a hard error that was previously present (but seemed undesirable). |
Mark-Simulacrum
added
the
relnotes
|
I'm also marking as relnotes because I would like for the regression here to be documented as a compatibility note. Technically it is really introduced by #77534 (so can point there instead) but the discussion here is likely to be a little more up to date. The regression from 1.48 is that same-level forbid followed by a non-forbid level will now be an error, but was a silent override previously. This does not affect non-local crates (e.g., crates.io dependencies) so should be a minimal impact for most users. |
Mark-Simulacrum
force-pushed
the
warn-on-forbids
branch
3 times, most recently
from
a9e794e
to
72f27be
Compare
|
Hey @Mark-Simulacrum , is there any unit test here that demonstrates the specific case you describe in the comment (for example, something where one uses I skimmed the tests and nothing seemed to match that case, which is changing behavior here. If there is no such test yet, can you add one? |
|
r=me once that test is added (or shown to otherwise exist). (But even with an r=me, this is still S-waiting-on-team for the T-lang review.) |
pnkfelix
added
S-waiting-on-author
|
Yes, I can add such a test. Could you kick off t-lang fcp? I think that's the next step here in that regard, and there's no need to block it on a test. |
Mark-Simulacrum
added
the
needs-fcp
Previously, cap-lints was ignored when checking the previous forbid level, which meant that it was a hard error to do so. This is different from the normal behavior of lints, which are silenced by cap-lints; if the forbid would not take effect regardless, there is not much point in complaining about the fact that we are reducing its level. It might be considered a bug that even `--cap-lints deny` would suffice to silence the error on overriding forbid, depending on if one cares about failing the build or precisely forbid being set. But setting cap-lints to deny is quite odd and not really done in practice, so we don't try to handle it specially. This also unifies the code paths for nested and same-level scopes. However, the special case for CLI lint flags is left in place (introduced by rust-lang#70918) to fix the regression noted in rust-lang#70819. That means that CLI flags do not lint on forbid being overridden by a non-forbid level. It is unclear whether this is a bug or a desirable feature, but it is certainly inconsistent. CLI flags are a sufficiently different "type" of place though that this is deemed out of scope for this commit.
Mark-Simulacrum
force-pushed
the
warn-on-forbids
branch
from
72f27be
to
64efcbe
Compare
Mark-Simulacrum
added
S-waiting-on-team
|
I was going to make an fcp merge, but I wanted to float one variation. I might prefer a rule that says "violating a forbid is an error unless cap-lints is allow, in which case it's ignored". This fits the logic of "who cares if the lint is forbid given that it would be allow anyway". |
|
I personally see that as more complicated / confusing, in some sense. The behavior this PR implements is that the error only happens if a forbid lint would have occurred, that is, we error when lowering the level from forbid to not forbid. With cap-lints set to allow, warn, or deny, no forbid lint can occur; this means that the level is not lowered. |
|
@rfcbot fcp merge I am proposed to merge this PR. It adopts the following semantics:
In terms of delta from nightly, the second point is newly introduced, so the net effect of the PR is to remove errors in the case where you are compiling code from crates.io. In terms of delta from older versions, there were some cases, specifically where forbid and allow attributes were combined in the same scope, where the code was accepted. Those cases became errors in #77534 and they remain so under this PR (unless cap-lints is being used). This triggered however some regressions for extant code under crates.io, which motivates this PR. |
|
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
Note that lowering the forbid level (either on the same level or in a nested scope) is already an error on nightly, due to #77534 - this relaxes this restriction to only apply with cap-lints=forbid or no cap-lints at all, both in nested and same level scopes. So the FCP here is really about removing errors in some cases, I think? Or at least that's what merging this PR means. |
|
My write-up was supremely unclear. Apologies. I've edited it for clarity. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
final-comment-period
|
📌 Commit 64efcbe has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
rfcbot
added
finished-final-comment-period
We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect.
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? `@Mark-Simulacrum`
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ``@Mark-Simulacrum``
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ```@Mark-Simulacrum```
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? `@Mark-Simulacrum`
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ``@Mark-Simulacrum``
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ``@Mark-Simulacrum``
Pkgsrc changes:
* Adjust patches, re-compute line offsets, fix capitalization.
* Remove i686/FreeBSD support, no longer provided upstream.
* Bump bootstraps to 1.49.0.
* Change USE_TOOLS from bsdtar to gtar.
* Reduce diffs to pkgsrc-wip package patches.
* Allow rust.BUILD_TARGET to override automatic choice of target.
* Add an i586/NetBSD (pentium) bootstrap variant (needs testing),
not yet added as bootstrap since 1.49 doesn't have that variant.
Upstream changes:
Version 1.50.0 (2021-02-11)
============================
Language
-----------------------
- [You can now use `const` values for `x` in `[x; N]` array
expressions.][79270] This has been technically possible since
1.38.0, as it was unintentionally stabilized.
- [Assignments to `ManuallyDrop<T>` union fields are now considered
safe.][78068]
Compiler
-----------------------
- [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi`
target.][78142]
- [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484]
- [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484]
\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.
Libraries
-----------------------
- [`proc_macro::Punct` now implements `PartialEq<char>`.][78636]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
arrays of any length.][74989]
- [On Unix platforms, the `std::fs::File` type now has a "niche"
of `-1`.][74699] This value cannot be a valid file descriptor,
and now means `Option<File>` takes up the same amount of space
as `File`.
Stabilized APIs
---------------
- [`bool::then`]
- [`btree_map::Entry::or_insert_with_key`]
- [`f32::clamp`]
- [`f64::clamp`]
- [`hash_map::Entry::or_insert_with_key`]
- [`Ord::clamp`]
- [`RefCell::take`]
- [`slice::fill`]
- [`UnsafeCell::get_mut`]
The following previously stable methods are now `const`.
- [`IpAddr::is_ipv4`]
- [`IpAddr::is_ipv6`]
- [`Layout::size`]
- [`Layout::align`]
- [`Layout::from_size_align`]
- `pow` for all integer types.
- `checked_pow` for all integer types.
- `saturating_pow` for all integer types.
- `wrapping_pow` for all integer types.
- `next_power_of_two` for all unsigned integer types.
- `checked_power_of_two` for all unsigned integer types.
Cargo
-----------------------
- [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976]
This option sets a wrapper to execute instead of `rustc`, for
workspace members only.
- [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire
contents of that directory for changes.][cargo/8973]
- [Added the `--workspace` flag to the `cargo update` command.][cargo/8725]
Misc
----
- [The search results tab and the help button are focusable with
keyboard in rustdoc.][79896]
- [Running tests will now print the total time taken to execute.][75752]
Compatibility Notes
-------------------
- [The `compare_and_swap` method on atomics has been deprecated.][79261]
It's recommended to use the `compare_exchange` and
`compare_exchange_weak` methods instead.
- [Changes in how `TokenStream`s are checked have fixed some cases
where you could write unhygenic `macro_rules!` macros.][79472]
- [`#![test]` as an inner attribute is now considered unstable like
other inner macro attributes, and reports an error by default
through the `soft_unstable` lint.][79003]
- [Overriding a `forbid` lint at the same level that it was set is
now a hard error.][78864]
- [Dropped support for all cloudabi targets.][78439]
- [You can no longer intercept `panic!` calls by supplying your
own macro.][78343] It's recommended to use the `#[panic_handler]`
attribute to provide your own implementation.
- [Semi-colons after item statements (e.g. `struct Foo {};`) now
produce a warning.][78296]
[74989]: rust-lang/rust#74989
[79261]: rust-lang/rust#79261
[79896]: rust-lang/rust#79896
[79484]: rust-lang/rust#79484
[79472]: rust-lang/rust#79472
[79270]: rust-lang/rust#79270
[79003]: rust-lang/rust#79003
[78864]: rust-lang/rust#78864
[78636]: rust-lang/rust#78636
[78439]: rust-lang/rust#78439
[78343]: rust-lang/rust#78343
[78296]: rust-lang/rust#78296
[78068]: rust-lang/rust#78068
[75752]: rust-lang/rust#75752
[74699]: rust-lang/rust#74699
[78142]: rust-lang/rust#78142
[77484]: rust-lang/rust#77484
[cargo/8976]: rust-lang/cargo#8976
[cargo/8973]: rust-lang/cargo#8973
[cargo/8725]: rust-lang/cargo#8725
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6
[`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align
[`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align
[`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size
[`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp
[`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take
[`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut
[`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then
[`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key
[`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp
[`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp
[`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key
[`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill
Previously, cap-lints was ignored when checking the previous forbid level, which
meant that it was a hard error to do so. This is different from the normal
behavior of lints, which are silenced by cap-lints; if the forbid would not take
effect regardless, there is not much point in complaining about the fact that we
are reducing its level.
It might be considered a bug that even
--cap-lints denywould suffice tosilence the error on overriding forbid, depending on if one cares about failing
the build or precisely forbid being set. But setting cap-lints to deny is quite
odd and not really done in practice, so we don't try to handle it specially.
This also unifies the code paths for nested and same-level scopes. However, the
special case for CLI lint flags is left in place (introduced by #70918) to fix
the regression noted in #70819. That means that CLI flags do not lint on forbid
being overridden by a non-forbid level. It is unclear whether this is a bug or a
desirable feature, but it is certainly inconsistent. CLI flags are a
sufficiently different "type" of place though that this is deemed out of scope
for this commit.
r? @pnkfelix perhaps?
cc #77713 -- not marking as "Fixes" because of the lack of proper unused attribute handling in this PR