Emit error when using path-segment keyword as cfg pred

[mu001999]

View all comments

Fixes #146968

Emit error CfgPredicateIdentifier if the word is path-segment keyword.

Detailed change description - #146978 (comment).

r? petrochenkov

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

[petrochenkov]

Could you

• add similar test cases for some non path-segment keyword, like struct
• add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg].

After that we should be able to run crater on this.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[mu001999]

add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg]

--cfg will emit same error but will be suppressed because parse_cfg is called with ParseSess::with_fatal_emitter

[mu001999]

@rustbot ready

[petrochenkov]

add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg]

--cfg will emit same error but will be suppressed because parse_cfg is called with ParseSess::with_fatal_emitter

with_fatal_emitter doesn't suppress errors, it makes them fatal.
--cfg parsing has its own error reporting logic in fn parse_cfg in compiler\rustc_interface\src\interface.rs, and the new errors are not emitted there.
@rustbot author

[mu001999]

with_fatal_emitter doesn't suppress errors, it makes them fatal.

@petrochenkov I found with_fatal_emitter create dcx with FatalOnlyEmitter, see

rust/compiler/rustc_session/src/parse.rs

Lines 318 to 329 in a2db928

	pub fn with_fatal_emitter(locale_resources: Vec<&'static str>, fatal_note: String) -> Self {
	let translator = Translator::with_fallback_bundle(locale_resources, false);
	let sm = Arc::new(SourceMap::new(FilePathMapping::empty()));
	let fatal_emitter =
	Box::new(HumanEmitter::new(stderr_destination(ColorConfig::Auto), translator));
	let dcx = DiagCtxt::new(Box::new(FatalOnlyEmitter {
	fatal_emitter,
	fatal_note: Some(fatal_note),
	}))
	.disable_warnings();
	ParseSess::with_dcx(dcx, sm)
	}

and the comment of FatalOnlyEmitter and the impl of FatalOnlyEmitter::emit_diagnostic show only errors with diag.level == Level::Fatal can be emitted, see

rust/compiler/rustc_errors/src/emitter.rs

Lines 537 to 562 in 8155734

	/// An emitter that does nothing when emitting a non-fatal diagnostic.
	/// Fatal diagnostics are forwarded to `fatal_emitter` to avoid silent
	/// failures of rustc, as witnessed e.g. in issue #89358.
	pub struct FatalOnlyEmitter {
	pub fatal_emitter: Box<dyn Emitter + DynSend>,
	pub fatal_note: Option<String>,
	}
	impl Emitter for FatalOnlyEmitter {
	fn source_map(&self) -> Option<&SourceMap> {
	None
	}
	fn emit_diagnostic(&mut self, mut diag: DiagInner, registry: &Registry) {
	if diag.level == Level::Fatal {
	if let Some(fatal_note) = &self.fatal_note {
	diag.sub(Level::Note, fatal_note.clone(), MultiSpan::new());
	}
	self.fatal_emitter.emit_diagnostic(diag, registry);
	}
	}
	fn translator(&self) -> &Translator {
	self.fatal_emitter.translator()
	}
	}

[mu001999]

I have debug the logic in fn parse_cfg, and the err.emit is called in fact, but it is suppressed by the FatalOnlyEmitter.

[petrochenkov]

Ah, ok, "fatal emitter" means "fatal-only emitter".

In any case, the behavior is not correct.
I guess the assumption when using the fatal emitter was that we always reach the "expected key or key="value"" error at the bottom anyway, but that's not actually the case.
If we get to return (ident.name, meta_item.value_str()), then all the non-fatal errors are lost, which is incorrect.

[ehuss]

Filed #148910

[bors]

⌛ Testing commit 5d70446 with merge 3d503f1...

[Zalathar]

Possibly failed in rollup: #148927 (comment)

@bors r-

[Zalathar]

@bors try jobs=x86_64-gnu-aux

[Zalathar]

@bors retry

[mu001999]

seems test failed in https://github.com/rust-lang/cargo/blob/2d4fa139552ebdd5f091a1401ed03f7dc62cb43f/tests/testsuite/cfg.rs#L783-L819

I'll fix it later

[mu001999]

I will request retry after cargo updated

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[mu001999]

The corresponding cargo-update #149073 has been merged

@rustbot ready

[petrochenkov]

Still need to update some tests failing on CI.
@rustbot author

[mu001999]

@rustbot ready

[petrochenkov]

@bors r+

[bors]

📌 Commit fc822f8 has been approved by petrochenkov

It is now in the queue for this repository.