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.

Sign up for GitHub

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

Jump to bottom

beta regression: forbid(warnings) interacts poorly with common derives. #81218

Closed
pnkfelix opened this issue Jan 20, 2021 · 4 comments
Closed

beta regression: forbid(warnings) interacts poorly with common derives. #81218

pnkfelix opened this issue Jan 20, 2021 · 4 comments
Assignees
@Mark-Simulacrum
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone
1.50.0

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Jan 20, 2021 •
edited
Loading

Spawned off of #80988, but specifically talking about forbid and derive interacting in a surprising manner.

Code

I tried this code (play):

#![forbid(warnings)]

#[derive(serde::Serialize)]
struct Bar;

I expected to see this happen: Successful diagnostic-free compile, just like what happens on stable 1.49 Rust

Instead, this happened:

error[E0453]: allow(non_upper_case_globals) incompatible with previous forbid
 --> src/lib.rs:3:10
  |
1 | #![forbid(warnings)]
  |           -------- `forbid` level set here
2 | 
3 | #[derive(serde::Serialize)]
  |          ^^^^^^^^^^^^^^^^ overruled by previous forbid
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0453]: allow(unused_attributes) incompatible with previous forbid
 --> src/lib.rs:3:10
  |
1 | #![forbid(warnings)]
  |           -------- `forbid` level set here
2 | 
3 | #[derive(serde::Serialize)]
  |          ^^^^^^^^^^^^^^^^ overruled by previous forbid
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0453]: allow(rust_2018_idioms) incompatible with previous forbid
 --> src/lib.rs:3:10
  |
1 | #![forbid(warnings)]
  |           -------- `forbid` level set here
2 | 
3 | #[derive(serde::Serialize)]
  |          ^^^^^^^^^^^^^^^^ overruled by previous forbid
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0453]: allow(unused_macros) incompatible with previous forbid
 --> src/lib.rs:3:10
  |
1 | #![forbid(warnings)]
  |           -------- `forbid` level set here
2 | 
3 | #[derive(serde::Serialize)]
  |          ^^^^^^^^^^^^^^^^ overruled by previous forbid
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors

Version it worked on

It most recently worked on: Rust 1.49

Version with regression

1.50.0-beta.6

@rustbot modify labels: regression-from-stable-to-beta -regression-untriaged
@rustbot rustbot added regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 20, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.50.0 milestone Jan 20, 2021
@pnkfelix pnkfelix mentioned this issue Jan 20, 2021
Closed
@apiraino apiraino added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 21, 2021
@apiraino
Copy link
Contributor

Assigning P-high as sibling issue #80988

@Mark-Simulacrum Mark-Simulacrum self-assigned this Jan 21, 2021
@Mark-Simulacrum Mark-Simulacrum added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 21, 2021
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 25, 2021 •
edited
Loading

This is a regression because previously the "lowering forbid" lint did not actually detect lint groups as being forbidden, i.e., the allows within the derive or elsewhere would be silently accepted and used. Derive was not previously (or currently) treated in any special way for the forbid handling specifically.

This issue intends for the lang team to decide what to do with this. We have a number of options:

  1. accept that people should not use forbid with wide-sweeping lints (e.g., warnings) as this may break proc macros. targeted lints are more likely to be ok, but still may break when proc macros add allow. (The breakage, in any case, will be strictly local as cap-lints applies to the forbid checking; fixing it will be a matter of replacing forbid with deny in the easy case).
  2. try to assemble a list of lints which are commonly allowed within derives or other macros, and silence those within macro-expanded code such that allow is not necessary; this would let internal lints and the ecosystem stop issuing allows, in theory (though backwards compat for crates with longer version support cycles may be interesting here)
  3. try to formulate some kind of hygiene, where forbid does not "reach into" macros ever.
  4. try to rework derives such that the lints currently allowed are not emitted because of different code being expanded. It seems likely this will not be plausible long-term.

There may be other options, but these seem like the most apparent. For all but option 1 we will likely want to revert or apply targeted patches to avoid breakage in the short term while we figure out a proper solution.

(note: I will not be able to attend lang triage this week, but am happy to answer questions async here or on zulip)

@joshtriplett
Copy link
Member

A fourth possibility would be if derive could somehow be fixable to not need to allow that lint.

@nikomatsakis nikomatsakis mentioned this issue Jan 30, 2021
Merged
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Feb 2, 2021
@nikomatsakis
introduce future-compatibility warning for forbidden lint groups
b6b897b 
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 issue Feb 3, 2021
@m-ou-se
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
54fa9ea 
…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 issue Feb 3, 2021
@Dylan-DPC
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
2071f89 
…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 issue Feb 4, 2021
@m-ou-se
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
ed4d311 
…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 issue Feb 4, 2021
@m-ou-se
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
5ba06ab 
…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 issue Feb 4, 2021
@m-ou-se
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
c5990dd 
…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 issue Feb 5, 2021
@m-ou-se @ehuss
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
3b03446 
…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``
@pietroalbini
Copy link
Member

Fix backported to 1.50.0 and landed in 1.51.0, closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

No branches or pull requests
6 participants
@joshtriplett @pnkfelix @pietroalbini @Mark-Simulacrum @apiraino @rustbot