Diagnostics for -D flags make it slightly harder to allow vs. #![deny(...)]

[ojeda]

When using -D command-line flags to deny some lints, the compiler emits diagnostics that are slightly harder to allow, compared to a denied lint via #![deny(...)].

Compare the following cases:

echo "struct S;"                      | rustc --crate-type=lib             -
echo "#![deny(dead_code)]\nstruct S;" | rustc --crate-type=lib             -
echo "struct S;"                      | rustc --crate-type=lib -Ddead_code -
echo "struct S;"                      | rustc --crate-type=lib -Dwarnings  -

The first two provide, one way or another, the name of the lint that can be easily copy-pasted from the terminal to allow(...) it. In particular, they provide the name of the lint with underscores:

warning: struct `S` is never constructed
 --> <anon>:1:8
  |
1 | struct S;
  |        ^
  |
  = note: `#[warn(dead_code)]` on by default

error: struct `S` is never constructed
 --> <anon>:2:8
  |
2 | struct S;
  |        ^
  |
note: the lint level is defined here
 --> <anon>:1:8
  |
1 | #[deny(dead_code)]
  |        ^^^^^^^^^

However, in the latter two cases, we get a message that is useful, but one needs to manually replace the minuses with underscores (and there may be a few in other cases):

error: struct `S` is never constructed
 --> <anon>:1:8
  |
1 | struct S;
  |        ^
  |
  = note: requested on the command line with `-D dead-code`

error: struct `S` is never constructed
 --> <anon>:1:8
  |
1 | struct S;
  |        ^
  |
  = note: `-D dead-code` implied by `-D warnings`

(By the way, note that the diagnostics also seem to print minus signs even if one used underscores in the command line, like here, i.e. -Ddead_code -- see rust-lang/rust-clippy#11332 as well).

For newcomers, it may make it harder to understand that one can allow(...) things locally with an attribute, plus it may not be obvious that they need to replace the minus signs, even if they tried:

error: expected one of `(`, `,`, `::`, or `=`, found `-`
 --> <anon>:1:13
  |
1 | #[allow(dead-code)]
  |             ^ expected one of `(`, `,`, `::`, or `=`

And, for experienced developers, it is anyway a bit annoying to have to do the replacement all the time.

I don't know what is the best way to improve this. Perhaps an extra help: line would not hurt, and in principle, it could be given in all cases:

error: struct `S` is never constructed
 --> <anon>:1:8
  |
1 | struct S;
  |        ^
  |
  = note: `-D dead-code` implied by `-D warnings`
  = help: add an `#[allow(dead_code)]` attribute to allow

This follows the pattern for the suggestion when using unstable features:

error[E0658]: use of unstable library feature 'num_midpoint'
 --> <source>:2:5
  |
2 |     i32::midpoint(0, 2)
  |     ^^^^^^^^^^^^^
  |
  = note: see issue #110840 <https://github.com/rust-lang/rust/issues/110840> for more information
  = help: add `#![feature(num_midpoint)]` to the crate attributes to enable

Of course, this could potentially encourage users to allow things they should not.

[mj10021]

Is there a reason why the non hyphen_case lint names can't be emitted?

rust/compiler/rustc_middle/src/lint.rs

Lines 216 to 231 in fd56162

	LintLevelSource::CommandLine(lint_flag_val, orig_level) => {
	let flag = orig_level.to_cmd_flag();
	let hyphen_case_lint_name = name.replace('_', "-");
	if lint_flag_val.as_str() == name {
	err.note_once(format!(
	"requested on the command line with `{} {}`",
	flag, hyphen_case_lint_name
	));
	} else {
	let hyphen_case_flag_val = lint_flag_val.as_str().replace('_', "-");
	err.note_once(format!(
	"`{} {}` implied by `{} {}`",
	flag, hyphen_case_lint_name, flag, hyphen_case_flag_val
	));
	}
	}

[est31]

For newcomers, it may make it harder to understand that one can allow(...) things locally with an attribute, plus it may not be obvious that they need to replace the minus signs, even if they tried

I think that error message can be improved also, to error with a better error message and ideally a suggestion, especially if there is an actual lint with that name.

Of course, this could potentially encourage users to allow things they should not.

Yeah, maybe that wording is too encouraging in that direction. Lints are there to point out potentially bad code, and sometimes you might want to #![allow()] the entire module, sometimes you might want to do it right next to the statement, sometimes you want to allow a single lint, or an entire group. And very often you want to address it as well.

Personally I think the best way to address this issue is to provide the ability to pass a span for these lints, in a way that it can both be passed by linux's makefile, as well as by Cargo via [lints] (see this discussion especially). Then users will directly get the place where they can adjust the lint level at least for local development, to turn off the noise.

[est31]

Also yeah I think the best is to pass the original spelling in the diagnostic, so that you can easily grep for it in your makefile, but if the lint name is mentioned anywhere, it should use _ style instead, because that one is portable from the cli arg domain to the source code domain.

[ojeda]

Personally I think the best way to address this issue is to provide the ability to pass a span for these lints, in a way that it can both be passed by linux's makefile, as well as by rust-lang/cargo#12115 (see rust-lang/rfcs#3389 (comment) discussion especially). Then users will directly get the place where they can adjust the lint level at least for local development, to turn off the noise.

Ah, I think I see what you mean (thanks for the links and the context!).

When I opened the issue, I had in mind the "permanent/non-prototyping" case, i.e. when one needs to commit the allow(...) into the repository, and thus I was thinking about the case where one wants to copy-paste the particular lint name. I had to do the replacement of the minus signs a bunch of times already, and I kept thinking how easy it was in the Rust Playground to enable an unstable feature clicking on top of it, thus I opened this issue (and another one for CE now :)

In the kernel, for the "prototyping/local development" case, one may #![allow(...)] an entire module/crate locally while developing, or we could have some as -W instead of -D (the kernel has a config option that enables -Werror and -Dwarnings -- one is expected to keep the build warning-free anyway).

So a kernel developer is not expected to have to tweak Makefiles manually (neither in C nor Rust), which is, if I understand correctly, where the span would point to in the approach you suggest, right? Or perhaps you meant that the span could point to the current module/crate and perhaps give a custom message too?

(Tweaking the Makefile feels "wrong" because most of the flags are global, i.e. shared across many crates. One can override bits per target, though, which would be better, but still, it is probably simpler to just write #![allow(...)] in that case in the file/crate one is working on.)

[est31]

which is, if I understand correctly, where the span would point to in the approach you suggest, right?

The span would be chosen entirely by the person calling the compiler, so it's your choice to point it to the makefile, or some kbuild option file. But if latter is chosen, IIRC only non-default config options are stored so if the default is to warn, then there is only the makefile to point to.

Tweaking the Makefile feels "wrong" because most of the flags are global, i.e. shared across many crates.

Yeah for features it's easy to suggest something good because they are always per-crate, you can't enable features on a more fine or roughly grained basis (outside of allow_internal_unstable which operates per-macro). But for lints you might see the lint appear many times in the module so you want it to be module specific, or it might be about that single invocation, etc. The compiler could perform that clustering operation itself. On the other hand, one might want to reserve the rustfix suggestion to the path that addresses the lint.

[ojeda]

@Urgau @est31 With #114089 landed (thanks!), should we close this one or do you want to use it for discussion of the span approach?

[est31]

I'm fine with closing. Spans are cool technology but I think #114089 has fixed most of the issues.

[ojeda]

Sounds good, thanks! Let's see what @Urgau says, perhaps there was a reason for the "Related" tag instead of "Fixes" in the PR.

[Urgau]

This can be closed.

I didn't do it in the PR since it was wasn't clear if my PR was all that would be needed to fix it.

[ojeda]

Thanks! Closing then :)