target json file contains unused fields unaffected by -Dwarnings

[ojeda]

I tried this:

# t.json is a target spec file with an unknown field
$ cat t.json | grep foo
  "foo": "bar",
$ rustc -Dwarnings --target=t.json < /dev/null

I expected to see this happen: the warning: target json file contains unused fields: foo diagnostic to be an error.

Instead, this happened: the warning is still a warning, even with -Dwarnings.

If this is intended (i.e. if -Dwarnings is only intended for code-related diagnostics), then this issue would be a feature request: a way to control whether non-code warnings are (or not) errors for CI purposes.

Meta

Happens in both stable (1.56.1) and the latest nightly:

rustc 1.58.0-nightly (dd549dcab 2021-11-25)
binary: rustc
commit-hash: dd549dcab404ec4c7d07b5a83aca5bdd7171138f
commit-date: 2021-11-25
host: x86_64-unknown-linux-gnu
release: 1.58.0-nightly
LLVM version: 13.0.0

[jyn514]

This happens because the code is using sess.warn instead of tcx.struct_span_lint_hir (presumably because tcx isn't available that early in compilation). I agree that this should be an error with -D warnings, but you could construe that as a breaking change for people who are running -D warnings currently. Could you open an MCP for changing this? I think it shouldn't be too hard to change in the diagnostic machinery, the compiler team just needs to decide it's a good idea.

[jyn514]

Relevant code:

rust/compiler/rustc_session/src/session.rs

Lines 1468 to 1479 in 6d246f0

	pub fn early_warn(output: config::ErrorOutputType, msg: &str) {
	let emitter: Box<dyn Emitter + sync::Send> = match output {
	config::ErrorOutputType::HumanReadable(kind) => {
	let (short, color_config) = kind.unzip();
	Box::new(EmitterWriter::stderr(color_config, None, short, false, None, false))
	}
	config::ErrorOutputType::Json { pretty, json_rendered } => {
	Box::new(JsonEmitter::basic(pretty, json_rendered, None, false))
	}
	};
	let handler = rustc_errors::Handler::with_emitter(true, None, emitter);
	handler.struct_warn(msg).emit();

[ojeda]

Could you open an MCP for changing this?

Thanks for the quick analysis -- will do!

[ojeda]

If this is intended (i.e. if -Dwarnings is only intended for code-related diagnostics), then this issue would be a feature request: a way to control whether non-code warnings are (or not) errors for CI purposes.

From the MCP/Zulip discussion, -Dwarnings is not intended to cover all warnings and changing that could potentially break existing users. Thus the next step would be identifying:

• Whether there are warnings that are not covered by -Dwarnings but should be (e.g. -D warnings should deny warnings from lint renames #94500).

• For those that are not intended to be covered by -Dwarnings, whether new options to control their "level" may make sense, either as a group or individually.

[BoxyUwU]

what "MCP/Zulip" discussion are you referring to?
edit: rust-lang/compiler-team#473

[ojeda]

rust-lang/compiler-team#473 was the MCP, and there is a link to the Zulip there from the bot (this one: https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/.60-Dwarnings.60.20to.20cover.20all.20warnings.20compiler-team.23473)

[jyn514]

From the MCP/Zulip discussion, -Dwarnings is not intended to cover all warnings and changing that could potentially break existing users.

that is not the conclusion i see in that MCP: rust-lang/compiler-team#473 (comment)

My understanding of the current consensus with nagisa is that this is fine for most warnings, but there are some warnings for unstable features that are explicitly warnings to avoid breaking people, which would change with this flag. To avoid a maintenance burden, nagisa is going to change those unstable features to give hard errors instead of warnings instead.

[ojeda]

My message was written before the second part of the discussion took place -- thanks for updating this!

So, all warnings are intended to be covered, in the end? Are @nagisa's proposed hard errors in place now?

[jyn514]

i am not aware of any warnings being converted to hard errors. looking at the MCP, nagisa gave -C target-feature as an example of a diagnostic that should be converted to a hard error: https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/.60-Dwarnings.60.20to.20cover.20all.20warnings.20compiler-team.23473/near/286500509

@nagisa were there any other diagnostics you wanted to change? i do see in retrospect that this is for a stable flag - maybe we should be using the future-incompat machinery instead of either hard warnings or hard errors?

[nagisa]

I’m… not sure. -Ctarget-feature doesn’t obviously scream to me like “definitely needs future-incompat” – we’ve been (unconditionally) warning about unknown or malformed features for a long time already, and I wouldn’t expect many build environment somehow set invalid features. While the wording of that warning is not particularly stern, it is an annoying warning nevertheless, in that majority of cargo build invocation will end up with more screenfuls of that warning than anything else.

(I also have no recollection on whether I went on to make -Ctarget-feature errors a hard error or not; I’d imagine that I didn’t, but I still would if I find time for general QoL contributions to rust-lang/*)

---

I don’t really have another example of a warning that should be an error instead. I was hoping for somebody to review warnings rustc can emit and summarize the list of those unaffected by the lint levels (as indicated earlier up thread in #91262 (comment).)