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

New "warning incompatible with previous forbid in same scope" error #77713

Open
Tracked by #54910
Nemo157 opened this issue Oct 8, 2020 · 15 comments
Open
Tracked by #54910

New "warning incompatible with previous forbid in same scope" error #77713

Nemo157 opened this issue Oct 8, 2020 · 15 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@Nemo157
Copy link
Member

Nemo157 commented Oct 8, 2020

I tried this code:

#![forbid(unused_extern_crates)]
#![warn(unused_extern_crates)]

fn main() {}

I expected to see this happen: an error that can be disabled via --cap-lints=warn

Instead, this happened: a hard error

> rustc temp.rs --cap-lints=warn
error[E0453]: warn(unused_extern_crates) incompatible with previous forbid in same scope
 --> temp.rs:2:9
  |
1 | #![forbid(unused_extern_crates)]
  |           ---------------- `forbid` level set here
2 | #![warn(unused_extern_crates)]
  |         ^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

After writing this up, looking back through the history (#77534, #70819) it appears this is fixing a regression, but given that this has been accepted by the stable compiler for over 3 years at this point, maybe it's still worth having a deprecation period? (This was noticed by a user on the discord as their code started failing to compile).

Meta

> rustc --version --verbose
rustc 1.49.0-nightly (91a79fb29 2020-10-07)
binary: rustc
commit-hash: 91a79fb29ac78d057d04dbe86be13d5dcc64309a
commit-date: 2020-10-07
host: x86_64-unknown-linux-gnu
release: 1.49.0-nightly
LLVM version: 11.0
@Nemo157 Nemo157 added the C-bug Category: This is a bug. label Oct 8, 2020
@Nemo157
Copy link
Member Author

Nemo157 commented Oct 8, 2020

cc @Mark-Simulacrum

@jyn514 jyn514 added A-stability Area: `#[stable]`, `#[unstable]` etc. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum added I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Oct 8, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 8, 2020
@Mark-Simulacrum
Copy link
Member

Nominating for lang team. I suspect we'll want crater, but maybe we just don't fix this.

@camelid
Copy link
Member

camelid commented Oct 8, 2020

I know we need backwards-compatibility, but what is the usefulness of supporting

#![forbid(unused_extern_crates)]
#![warn(unused_extern_crates)]

?

@Nemo157
Copy link
Member Author

Nemo157 commented Oct 8, 2020

The actual original code was

#![forbid(rust_2018_idioms)]
#![warn(unused_extern_crates)]

@withoutboats
Copy link
Contributor

withoutboats commented Oct 12, 2020

Reviewing this in the lang team meeting I was surprised that this behavior is order-dependent. That is, this works without warning or error:

#![warn(unused_extern_crates)]
#![forbid(unused_extern_crates)]

fn main() {}

Not sure if this is backwards compatible with the present situation on stable or nightly, but to me the intuitive behavior of overlapping lint-setters at the same level is this: we take the maximum value that the user set, and then warn that the other attributes do nothing. That is, both of these examples (regardless of order), should set unused_extern_crates to forbid, and produce a warning that the warn attribute does nothing.

On the other hand, considering this different example:

#![warn(rust_2018_idioms)]
#![forbid(unused_extern_crates)]

fn main() {}

This should set unused_extern_crates to forbid, and every other lint in rust_2018_idioms to warn. There should be no unused attribute warning, because both have an effective impact on lint levels.

That is, I think the behavior should be order independent - two attributes applied to the same item (in this case, the top level module) should be considered the same regardless of the order they are in the program text.

@nikomatsakis
Copy link
Contributor

We spent a while discussing this in today's lang-team meeting. We didn't reach a clear consensus on what we thought was the most appropriate behavior, but we did come up with some consensus points:

  • definitely we should forbid in the crate if there is a forbid at the top level
    • because: if someone writes forbid(unsafe_code) and then someone PRs in some small line with warn(unsafe_code) on a module and then allow(unsafe_code) on an unsafe usage, that's really not what the person who wrote forbid wanted
    • note though that cap-lints does mean that you can't rely on forbid as a had guarantee (but also means that dependencies don't break)
  • top level should not be special, all levels should behave the same
  • we should at least warn, and maybe error, for forbid followed by allow
    • "later" items might be considered "within" former items, which corresponds to an error
    • but for back-compat or other reasons a warning ("this has no effect") might be reasonable
  • we can either accept, warn, or hard error for allow followed by forbid
    • as above, if you think of allow on one level and forbid in a nested level, then this would be fine
    • but it'd be nice to get a warning that this allow had no effect at all

We'll discuss a bit more next week but here are some notes and questions:

  • We'd like a clear timeline of what the behavior was at each point and maybe clarify a few points (some of these I added post meeting)
    • It would be good to clarify what the behavior is (and what it used to be) when combining forbid/allow in scopes other than the top-level -- is the top-level scope here special somehow?
    • Does order effect lints at other levels? What about command-line options?
  • Crater results would be useful to get a sense of overall impact
  • I think we'd be happy with something that ensures that the forbid takes precedence, but where we just issue lints for "incompatible" allow/forbid at the same attribute scope

Also, one point of clarification for @Nemo157. In the OP, you wrote:

I expected to see this happen: an error that can be disabled via --cap-lints=warn

At first, we thought this might mean that, prior to #77534, the cap-lints setting was used to determine whether a hard error occurred or not. But we later concluded that you were saying "cap-lints should affect the result here because otherwise existing crates in crates.io will stop compiling", is that correct?

@Nemo157
Copy link
Member Author

Nemo157 commented Oct 12, 2020

But we later concluded that you were saying "cap-lints should affect the result here because otherwise existing crates in crates.io will stop compiling", is that correct?

Exactly, I wrote that before I had looked into the history and realised that this was fixing a prior regression, I was just assuming it was a newly added error that was being over-zealously implemented as a hard error rather than a deny-by-default lint.

@camelid
Copy link
Member

camelid commented Oct 12, 2020

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 12, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.49.0 milestone Oct 19, 2020
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 19, 2020

Discussed in the lang team meeting:

  • We would be happy with the behavior of "forbid wins but generates a lint" (but current behavior may be acceptable too)
  • But the older behavior @pnkfelix thinks was that the "most recent attribute" would win, which is definitely incorrect
  • Making it an error was thus an easy fix

Next step:

  • Timeline and investigation

@Mark-Simulacrum Mark-Simulacrum self-assigned this Oct 19, 2020
@Mark-Simulacrum
Copy link
Member

Always true:

  • Behavior of attributes is order-dependent, last attribute wins. Duplicate (e.g., #![deny(unused)] ... #![deny(unused)] attributes are not an error.
  • cap-lints always overrides any level set, including forbid.
  • It is an error to attempt to lower the level from forbid when in nested scopes (e.g., CLI says forbid, code says deny at top level). At the same scope this is not an error. Note that for modules, the outer and inner scopes can be in different files (#[attr] mod foo; and foo.rs has #![attr]).

New behavior, introduced by #77534:

Forbid (essentially) takes immediate effect, rather than being overridden by other attributes at the same level. This is unique behavior specifically to forbid; no other lint receives the same treatment.

I would propose the following (the implementation should be worked out, of course, but this seems feasible -- I would not FCP until we have a PR, just that if people have objections to this raising them now seems good):

  • Warn (unused attribute) on skipped/ignored attributes at the same level.
    • This is not an error for backwards compatibility, primarily.
    • One downside is that this is basically not possible to silence in a targeted fashion (you have to disable it for a whole tree, but that's probably not too important -- in practice this is true of all attribute lints).
  • Locally overriding forbid (i.e., at different or the same level) is a forbid-by-default lint. This means you can't disable it except with cap-lints.
    • This also matches just the behavior of forbid itself which seems quite reasonable.
    • This is backwards compatible in dependencies, though not locally. I think the breakage for forbid followed by an allow/deny whatever at the same level is likely to be minimal, and desirable. If we choose not to do this it would hopefully be caught by the first bullet here, but I suspect that one will not be as easy to implement.

@Mark-Simulacrum
Copy link
Member

I will try to work out an implementation to better get a sense of what would fall out of the impl naturally without too much contortion. We'll then try to FCP that proposal.

(If this ends up taking too long, we will likely just revert #77534 on beta or so; I am not too worried, as it is not yet a beta regression).

@Mark-Simulacrum
Copy link
Member

Going to denominate this until I end up with an implementation to FCP; I expect there's nothing further to discuss here until then.

@Mark-Simulacrum
Copy link
Member

I have posted #78864 which fixes the regression here in dependencies (i.e., cap-lints will affect the new lint/error).

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Nov 16, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 2, 2020
…kfelix

Use true previous lint level when detecting overriden forbids

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.

r? `@pnkfelix` perhaps?

cc rust-lang#77713 -- not marking as "Fixes" because of the lack of proper unused attribute handling in this PR
@Mark-Simulacrum Mark-Simulacrum removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 3, 2020
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.49.0 milestone Dec 3, 2020
@Mark-Simulacrum Mark-Simulacrum added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed A-stability Area: `#[stable]`, `#[unstable]` etc. C-bug Category: This is a bug. P-high High priority T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 3, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 3, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.49.0 milestone Dec 3, 2020
@Mark-Simulacrum Mark-Simulacrum removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 3, 2020
@Mark-Simulacrum
Copy link
Member

The remaining action here is to:

@Mark-Simulacrum
Copy link
Member

Revert in #79903, untagging as regression

@Mark-Simulacrum Mark-Simulacrum removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.49.0 milestone Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum removed their assignment Dec 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 10, 2020
…ulacrum

[beta] backports

* Revert rust-lang#77534 fixing rust-lang#77713 on beta, principled fix landed on master
* fix soundness issue in `make_contiguous` rust-lang#79814
* Fix exhaustiveness in case a byte string literal is used at slice type rust-lang#79072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

No branches or pull requests

7 participants