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

Use true previous lint level when detecting overriden forbids #78864

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

Mark-Simulacrum
Copy link
Member

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 #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2020
@Mark-Simulacrum Mark-Simulacrum added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 8, 2020
@Mark-Simulacrum
Copy link
Member Author

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 Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 8, 2020
@Mark-Simulacrum
Copy link
Member Author

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.

@pnkfelix
Copy link
Member

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 --cap-lints=deny on a file that itself has a #[forbid(..)] in it, and so you don't actually get an error from compiling the source due to the presence of --cap-lints ?

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?

@pnkfelix
Copy link
Member

pnkfelix commented Nov 13, 2020

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 pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2020
@Mark-Simulacrum
Copy link
Member 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 Mark-Simulacrum added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 13, 2020
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 Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 14, 2020
@nikomatsakis
Copy link
Contributor

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".

@Mark-Simulacrum
Copy link
Member Author

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.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 17, 2020

@rfcbot fcp merge

I am proposed to merge this PR. It adopts the following semantics:

  • It is an error to lower the lint level from forbid to any other level.
  • However, if cap-lints is set to something less than forbid, then the lint will never become forbid level, so no error is reported.

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.

@rfcbot
Copy link

rfcbot commented Nov 17, 2020

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 17, 2020
@Mark-Simulacrum
Copy link
Member Author

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.

@nikomatsakis
Copy link
Contributor

My write-up was supremely unclear. Apologies. I've edited it for clarity.

@rfcbot
Copy link

rfcbot commented Dec 1, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 1, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors
Copy link
Contributor

bors commented Dec 1, 2020

📌 Commit 64efcbe has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 1, 2020
@bors
Copy link
Contributor

bors commented Dec 2, 2020

⌛ Testing commit 64efcbe with merge eb4860c...

@bors
Copy link
Contributor

bors commented Dec 2, 2020

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing eb4860c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 2, 2020
@bors bors merged commit eb4860c into rust-lang:master Dec 2, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 2, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 11, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 17, 2020
nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Feb 2, 2021
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.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 3, 2021
…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`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 3, 2021
…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``
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
…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```
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
…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`
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
…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``
ehuss pushed a commit to ehuss/rust that referenced this pull request Feb 5, 2021
…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``
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 19, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants