Skip to content

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

New issue
New issue
Open
Open
New "warning incompatible with previous forbid in same scope" error#77713
Labels
A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.C-enhancementCategory: An issue proposing an enhancement or a PR with one.

Description

@Nemo157
Nemo157
opened on 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

Activity

  1. ImageNemo157
    added
    C-bugCategory: This is a bug.
    on Oct 8, 2020

  2. Nemo157 commented on Oct 8, 2020

    @Nemo157
    Nemo157
    on Oct 8, 2020
    ContributorAuthor

    cc @Mark-Simulacrum

  3. Imagejyn514
    added
    A-stabilityArea: `#[stable]`, `#[unstable]` etc.
    T-langRelevant to the language team
    on Oct 8, 2020
  4. ImageMark-Simulacrum
    added
    I-nominated
    regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.
    on Oct 8, 2020
  5. Imagerustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Oct 8, 2020

  6. Mark-Simulacrum commented on Oct 8, 2020

    @Mark-Simulacrum
    Mark-Simulacrum
    on Oct 8, 2020
    Member

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

  7. camelid commented on Oct 8, 2020

    @camelid
    camelid
    on Oct 8, 2020
    Member

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

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

    ?

  8. Nemo157 commented on Oct 8, 2020

    @Nemo157
    Nemo157
    on Oct 8, 2020
    ContributorAuthor

    The actual original code was

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

  9. withoutboats commented on Oct 12, 2020

    @withoutboats
    withoutboats
    on Oct 12, 2020
    Contributor

    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.

  10. nikomatsakis commented on Oct 12, 2020

    @nikomatsakis
    nikomatsakis
    on Oct 12, 2020
    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?

  11. Nemo157 commented on Oct 12, 2020

    @Nemo157
    Nemo157
    on Oct 12, 2020
    ContributorAuthor

    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.

  12. camelid commented on Oct 12, 2020

    @camelid
    camelid
    on Oct 12, 2020
    Member

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

  13. Imagecamelid
    removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Oct 12, 2020

  14. 19 remaining items

  15. ImageMark-Simulacrum
    added
    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
    C-enhancementCategory: An issue proposing an enhancement or a PR with one.
    regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
    and removed
    T-langRelevant to the language team
    on Dec 3, 2020
  16. Imagerustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Dec 3, 2020
  17. ImageMark-Simulacrum
    added this to the 1.49.0 milestone on Dec 3, 2020
  18. ImageMark-Simulacrum
    removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Dec 3, 2020

  19. Mark-Simulacrum commented on Dec 3, 2020

    @Mark-Simulacrum
    Mark-Simulacrum
    on Dec 3, 2020
    Member

    The remaining action here is to:

  20. Mark-Simulacrum commented on Dec 10, 2020

    @Mark-Simulacrum
    Mark-Simulacrum
    on Dec 10, 2020
    Member

    Revert in #79903, untagging as regression

  21. ImageMark-Simulacrum
    removed
    regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
    on Dec 10, 2020
  22. ImageMark-Simulacrum
    removed this from the 1.49.0 milestone on Dec 10, 2020
  23. ImageMark-Simulacrum
    removed their assignment
    on Dec 10, 2020
  24. Imagebors
    added a commit that references this issue on Dec 10, 2020

    Auto merge of rust-lang#79903 - Mark-Simulacrum:beta-next, r=Mark-Sim…

    877c7cb
  25. Imagefmease
    added
    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
    and removed
    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
    on Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.C-enhancementCategory: An issue proposing an enhancement or a PR with one.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions