Copy link

Contributor

rylev commented on Jul 7 •

Work that needs to be done to stabilize:

• Merge #86970
• Change name to --force-warn #87346
• Remove from the unstable book.
• Update the rustc book.
• Create a stabilization report.
• Make usage stable.

inquisitivecrystal commented on Jul 19

@rylev You mentioned that you're planning to work on stabilization. I'm happy to help out with this, but I don't want to steal your project.

rylev commented on Jul 19

@inquisitivecrystal I’m happy to split the work with you. I was going to open a PR rename the option to —force-warn, and I’m already working on a stabilization report. Let me know if you want to take on some piece of the work outlined above (or if we’re missing something that should be added to the list).

inquisitivecrystal commented on Jul 20

Well, I'm happy to help out however I can. You working on the stabilization report is definitely a good idea, since I wouldn't have any clue how to do that. I'm not entirely sure I know how to do the documentation PR, but I can work it out. The other steps are simple enough that anyone can do them. I'd definitely be happy to take final stabilization, as I find those kind of fun. I guess overall, it would make sense for me to take stabilization, and maybe also the documentation if you'd like me to that?

Incidentally, does this need to go through FCP after we get all the other stuff in order?

inquisitivecrystal commented 26 days ago

I've got a WIP combined documentation and stabilization PR that's pretty close to completion. I'm hoping to be able to submit it as a draft in the next few days, though it wouldn't be merged until the stabilization report and FCP. @rylev, is that alright with you?

rylev commented 24 days ago

@inquisitivecrystal Yep! Feel free to open the draft PR, and I will have the stabilization report done soon.

rylev commented 24 days ago •

Stabilization Report

(Let me know if I missed anything)

Description of behavior

• A new command line option force-warn is added which (like other lint options like -A, -W, etc.) accepts a lint name or lint group and ensures that if that lint is triggered it is always emitted as a "warn" level lint, ignoring any other lint level specification (cap-lints, attributes in code, command line options, etc. -- including levels like forbid).
• force-warn is to warn as forbid is to deny. It behaves just like the other option but cannot be suppressed through mechanisms like --cap-lints.

The primary use case of this feature is for cargo fix which uses this feature to ensure that compatibility lints are emitted (and subsequently the fixes for those lints are applied).

Possible points of contention

This feature is relatively low risk as we don't foresee it being widely used, and behavior of lints is not a part of the stability guarantee for Rust. Therefore, in the worst case, this option can be removed completely (though we would need to ensure that passing the option would not cause an error).

Small points of possible contention:

• The name force-warn was bikeshedded but was not selected due to any particular criteria other than "sounding good" to the PR authors and reviewers.

Documentation and Testing

• Stablization PR: #87472

This feature is being tested through the usage of cargo fix in the Rust 2021 edition testing. So far, no issues related to its usage have been found.

Remaining issues for stabilization can found here: #86516 (comment)

force-warn is to warn as forbid is to deny. It behaves just like the other option but cannot be suppressed through mechanisms like --cap-lints.

Can we elaborate on this point? I think it'd be useful to list the behavior in a sort of matrix with cap-lints on one axis and the environment the lint is issued in (deny/allow(warnings); allow/warn/deny/forbid(some_specific_lint)) on the other.

For example, does force-warn cause an allow'd lint to fire? If it does fire, is it forcing it to warn level, or does it become a regular warning and is affected by deny(warnings) to trigger a compilation error?

A separate question is whether --force-warn warnings is accepted. Warnings is not a statically defined lint group, and it may not be quite well suited to what this flag does. Preventing it would leave room open in the future and protect against unexpected behavior in the meantime.

inquisitivecrystal commented 23 days ago •

I have something of a last minute proposal. I was just updating the docs to reflect the fact that --cap-lints doesn't work for force-warn. This is extremely confusing, since it breaks completely with the behavior for forbid. I don't think we should stabilize it in this state.

I have two alternatives. First, we could make cap-lints work the same way for force-warn that it does for everything else. This would require a change to cargo's fix system to set its cap lints to force-warn instead of warn, but that's a one-line change. Alternatively, we could say that setting cap-lints to warn actually sets it to force-warn. This is slightly inconsistent, but you could argue that it's probably what the user intends anyway.

rylev commented 23 days ago

Can we elaborate on this point

--force-warn $LINT will cause $LINT to always emit a warning if triggered even if --cap-lints, any warn level CLI option (e.g., -A or -F), or code attribute (e.g., #![deny($LINT)]) is used. In other words, --force-warn always wins out and the lint will emit a warning if triggered.

whether --force-warn warnings is accepted

This is currently accepted. This sounds like an easy way to see all warnings for a codebase even if they are allowed by some other part of the codebase. Is that not a desirable?

it breaks completely with the behavior for forbid

This is not the only way that this option differs from forbid. For example, placing a #[deny($LINT)] on an item and then passing -F $LINT at the command line produces an error: " incompatible with previous forbid".

This does not happen with --force-warn. A warning is simply emitted if $LINT is triggered even if other parts of the code "deny" that lint.

inquisitivecrystal commented 23 days ago •

This is currently accepted. This sounds like an easy way to see all warnings for a codebase even if they are allowed by some other part of the codebase. Is that not a desirable?

If I understand correctly, --deny warnings actual denies anything with a lint level of Warn. Forcing things with a resolved lint level of Warn to Warn isn't going to do much.

This is not the only way that this option differs from forbid. For example, placing a #[deny($LINT)] on an item and then passing -F $LINT at the command line produces an error: " incompatible with previous forbid".

I personally consider that less confusing. shrugs

Edit:

There's a fair chance I'm wrong and we should just stick with what we have. It's just unfortunate ends up being really inconsistent, because it was designed around the needs of edition migration and doesn't really fit with the rest of the system.

m-ou-se commented 7 days ago

@rylev @Mark-Simulacrum This needs a decision so we can move forward. If we want to do a full FCP on this before stabilizing the edition, we're going to run a bit low on time soon.

I had missed the comment from a couple weeks ago -- taking a look now, and nominating for T-lang discussion (to move us toward FCP to stabilize). @m-ou-se what is the timeline on this needing to stabilize if we want to avoid shifting other schedules? I would prefer not to rush with stabilization of course, but knowing how much time we have without bumping things would be good.

whether --force-warn warnings is accepted

This is currently accepted. This sounds like an easy way to see all warnings for a codebase even if they are allowed by some other part of the codebase. Is that not a desirable?

Hm. I don't see tests for this (based on rg force.warn src/test | rg warnings), and I'd be a little surprised if it worked out of the box. As I mentioned in my comment, warnings is not a statically defined lint group -- it is composed of any lints that would have caused a warn-level lint when emitted, and sets those lints to actually cause its own level lint (e.g., deny(warnings) puts any warnings that would have been emitted at deny level).

I think in general this may be desirable, but we should at least have tests for it, and if we don't want to wait on those, not stabilize this part of the feature just yet.

The name force-warn was bikeshedded but was not selected due to any particular criteria other than "sounding good" to the PR authors and reviewers.

I'll throw my hat into the bucket of choosing a longer name, to avoid consuming interesting option space. Perhaps we even want to slightly generalize: --force-lint-level $lint=$level. For example, drawing upon @rylev's example of wanting to avoid anyone using allow/cap-lints etc, --force-lint-level unsafe_code=deny would ensure errors on unsafe code in the program, and is not silence-able; it could be used in e.g. CI. Not quite achievable with -F unsafe_code as that doesn't work with cap-lints, I believe.

rylev commented 6 days ago

what is the timeline on this needing to stabilize

This needs to be stable when the edition is stable which is currently planed for 1.56 which will be cut sometime in September.

whether --force-warn warnings is accepted

I'd be a little surprised if it worked out of the box

This indeed works now:

#![allow(dead_code)]

fn main() {
    println!("Hello, world!");
}

fn foo() {}

 cargo +nightly rustc -- -Zunstable-options --force-warn warnings
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
warning: function is never used: `foo`
 --> src/main.rs:7:4
  |
7 | fn foo() {}
  |    ^^^
  |
  = note: `--force-warn dead-code` implied by `--force-warn warnings`

warning: `deleteme` (bin "deleteme") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.24s

it is composed of any lints that would have caused a warn-level lint when emitted

I indeed think we have some sort of logical inconsistency here. The warnings lint group is composed of all lints that are considered warnings for the current invocation of the compiler (i.e., if I use -W missing_abi then missing_abi is considered a warning even though it is normally considered allow).

However, --force-warns only takes into consideration if the lint is ever considered a warning even if some additional part of the code later makes the lint not a warning. For example:

#![warn(missing_abi)]
fn main() {
    println!("Hello, world!");
}

extern fn foo() {}

 cargo +nightly rustc -- -Zunstable-options  --force-warn warnings -A missing_abi
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:6:1
  |
6 | extern fn foo() {}
  | ^^^^^^^^^^^^^^^ ABI should be specified here
  |
  = note: `--force-warn missing-abi` implied by `--force-warn warnings`
  = help: the default ABI is C

This code warns even though the command line allows it.

The other way around:

#![allow(warnings)]
fn main() {
    println!("Hello, world!");
}

extern fn foo() {}

cargo +nightly rustc -- -Zunstable-options  --force-warn warnings -W missing_abi
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:6:1
  |
6 | extern fn foo() {}
  | ^^^^^^^^^^^^^^^ ABI should be specified here
  |
  = note: `--force-warn missing-abi` implied by `--force-warn warnings`
  = help: the default ABI is C

 cargo +nightly rustc -- -Zunstable-options   -W missing_abi
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s

In a way --force-warns is sticky - it will warn if a lint was ever considered a warning. Here's an extreme:

#![warn(missing_abi)]
#![allow(warnings)]
fn main() {
    println!("Hello, world!");
}

extern fn foo() {}

 cargo +nightly rustc -- -Zunstable-options  --force-warn warnings
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:7:1
  |
7 | extern fn foo() {}
  | ^^^^^^^^^^^^^^^ ABI should be specified here
  |
  = note: `--force-warn missing-abi` implied by `--force-warn warnings`
  = help: the default ABI is C

warning: function is never used: `foo`
 --> src/main.rs:7:11
  |
7 | extern fn foo() {}
  |           ^^^
  |
  = note: `--force-warn dead-code` implied by `--force-warn warnings`

warning: `deleteme` (bin "deleteme") generated 2 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s

If there's not a test for this we should definitely add one (assuming we want to keep this behavior).

joshtriplett commented 2 days ago •

This seems reasonable to me. I think we should go ahead and take it as-is. I can also imagine wanting a "force deny" option in the future, but I don't want that to be a blocker for this.

We shouldn't make cap-lints affect this, I think; poking through cap-lints seems like part of the point of the option.

We should talk about the desired semantics for --force-warn warnings in today's meeting. I have no objections to just not allowing it for now, to allow ourselves to decide that later if and when we clean up what "warnings" means.

joshtriplett commented 2 days ago

@rfcbot merge

rfcbot commented 2 days ago •

Team member @joshtriplett 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 commented 2 days ago

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

rylev commented 2 days ago

In the lang team meeting, it was determined that the semantics seem to be correct as they are implemented. --force-warn $LINT causes a warning (or more sever level) to be emitted should $LINT be triggered.

The only wrinkle is that --force-warn warnings has unclear semantics since the warnings lint group doesn't seem to have very clear semantics, and thus we should forbid it for now. I will work on this.

I would like for us to rename the option to --min-lint-level, FWIW, since the option doesn't force a warning, but rather raises the minimum lint level. I think --force-warn sounds it like it acts as a cap on 'both sides', both --cap-lints warn and --min-lint-level warn, basically, but that isn't actually what it behaves as. And --min-lint-level lint=warn seems pretty clear, in contrast, to me, in terms of the resulting behavior.

I'd be happy to put in the work on the rename, and we could limit to just supporting =warn, for now, to minimize additional surface area. I think adding the force-warn option as-is isn't terrible, ultimately, since most users won't use it, but I think it'll create unnecessary baggage for the already pretty complicated system.

ehuss commented 2 days ago

--force-warn sounds it like it acts as a cap on 'both sides',

I believe that is what it does. That is, if a lint is denied or forbidden, it is lowered to a warning, and if it is allowed it is raised to a warning.

rylev commented 2 days ago •

@ehuss is correct, and I misspoke in the lang team meeting unfortunately.

#![deny(dead_code)]
fn main() {
    println!("Hello, world!");
}

fn foo() {}

$ cargo +nightly rustc -- -Zunstable-options --force-warn dead_code
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
warning: function is never used: `foo`
 --> src/main.rs:6:4
  |
6 | fn foo() {}
  |    ^^^
  |
  = note: requested on the command line with `--force-warn dead-code`

warning: `deleteme` (bin "deleteme") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.48s

$  cargo +nightly rustc
   Compiling deleteme v0.1.0 (/home/rylevick/deleteme)
error: function is never used: `foo`
 --> src/main.rs:6:4
  |
6 | fn foo() {}
  |    ^^^
  |
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![deny(dead_code)]
  |         ^^^^^^^^^

error: could not compile `deleteme` due to previous error

OK - in that case, name seems fine, though it's not clear to me that the behavior is generally useful; that doesn't seem to preclude stabilization for the targeted use case.

rylev commented yesterday

cc @joshtriplett and @nikomatsakis to make sure their aware of the fact that --force-warn causes warnings no matter what (and not warnings or worse as was suggested in the lang team meeting).

joshtriplett commented yesterday

That's not ideal; the semantics I'd expect are "at least a warning", not downgrading deny to warn.

How difficult would it be to change that, to make it a minimum but not a maximum?

rylev commented yesterday

It should be an easy change. I can have a PR soon. Do we need to bring this to the attention of the entire lang time to make a final call on which behavior we want?

@ehuss - this shouldn't matter either way for the cargo fix use case, correct?

rylev commented yesterday

Taking a look at what it would take to implement this, and it's unfortunately a little bit more complicated than a trivial change. The issue is that for deny-by-default lints for which we have set --force-warn $LINT we want to deny unless somewhere else in the code has changed the lint to a different level.

This leads to a situation where we --force-warn $LINT (which is deny by default) and another part of the code allows $LINT. Without the --force-warn the code would compile (because the lint is allowed), but with --force-warn the code errors since we take the higher between ForceWarn and the lint's default (which is Deny).

When a lint has been set to force warn, we don't know whether something like a command line flag or module attribute tried to lower a deny-by-default lint to another lower level. When we try to get the lint level we only have the lint's default level and the level it's been set to (presumably ForceWarn).

This isn't impossible to overcome obviously, but I unfortunately won't be able to get to it before my vacation begins. @Mark-Simulacrum can you take this over?

Yes, I can take this on.

ehuss commented yesterday

I can't think of any particular issues. If a lint is raised to an error, then the code wouldn't compile in the first place, and that is not something cargo fix handles.

I spent some time trying to make the future-incompatible warnings use --force-warn, but ended up abandoning the effort since there were too many tricky edge cases. For that kind of use case (you absolutely want a warning for something no matter what), you have to be careful with the interaction of --cap-lints and other controls. For example, if code had #![forbid(private_in_public)], compiled with --cap-lints=allow and --force-warn=private_in_public, I would want a warning. As long as --force-warn is "stronger" than --cap-lints, then I think that use case is covered, should anyone want to do something like that.

jplatte commented 3 hours ago

I don't understand why interaction with --cap-lints is so important. Isn't that only set for dependencies, where you wouldn't want cargo fix to do anything anyways (and so --cap-lints and --force-warn could just be made mutually exclusive)?