elided_lifetime_in_path triggers for the format! macro

[nikomatsakis]

I was trying to use deny(elided_lifetime_in_path) and encountered the problem that this doesn't work:

#![feature(nll)]
#![deny(elided_lifetime_in_path)]

fn main() {
    format!("foo {}", 22)
}

I get

error: hidden lifetime parameters are deprecated, try `Foo<'_>`
 --> src/main.rs:5:5
  |
5 |     format!("foo {}", 22)
  |     ^^^^^^^^^^^^^^^^^^^^^
  |
note: lint level defined here
 --> src/main.rs:2:9
  |
2 | #![deny(elided_lifetime_in_path)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

I suppose the easiest fix is to fix the format! expansion, but maybe better to suppress this warning for macros outside of the current crate? Feels like a more general problem.

[nikomatsakis]

cc @Dylan-DPC

[withoutboats]

Feels like a more general problem.

I think the more general problem is that lints should be hygienic - we should only check them against ast nodes from this code, not those generated by a macro.

[shepmaster]

we should only check them against ast nodes from this code

I've seen someone bit by something similar. They had a deny(unsafe) but a crate's macro ended up expanding to an unsafe block. IIRC, they fixed the library to add an allow on the block inside the macro.

So +1 for this kind of fix.

[durka]

That won't help with forbid.

[Dylan-DPC-zz]

Any pointers on how to suppress this warning for macros outside of the current crate?

[durka]

Clippy has a function.

[nikomatsakis]

It does feel wrong to do this on a lint-by-lint basis, though, right? Maybe as part of struct_lint_level?

rust/src/librustc/lint/mod.rs

Lines 404 to 420 in 27a046e

	pub fn struct_lint_level<'a>(sess: &'a Session,
	lint: &'static Lint,
	level: Level,
	src: LintSource,
	span: Option<MultiSpan>,
	msg: &str)
	-> DiagnosticBuilder<'a>
	{
	let mut err = match (level, span) {
	(Level::Allow, _) => return sess.diagnostic().struct_dummy(),
	(Level::Warn, Some(span)) => sess.struct_span_warn(span, msg),
	(Level::Warn, None) => sess.struct_warn(msg),
	(Level::Deny, Some(span)) |
	(Level::Forbid, Some(span)) => sess.struct_span_err(span, msg),
	(Level::Deny, None) |
	(Level::Forbid, None) => sess.struct_err(msg),
	};

We could e.g. return the "dummy" value for spans that are not in the current crate:

rust/src/librustc/lint/mod.rs

Line 413 in 27a046e

(Level::Allow, _) => return sess.diagnostic().struct_dummy(),

[Manishearth]

It does feel wrong to do this on a lint-by-lint basis, though, right? Maybe as part of struct_lint_level?

It's not that simple, though. The node being linted may be outside of a macro, but it may contain macroy things that affect the lint. The correct way to handle this is to identify all the "important" nodes and do a macro check.

(Also: All currently supported ways of doing a macro check will not lint stuff like some_macro!(some.code.that.hits.the.lint), which isn't an uncommon breed of macro.)

[nikomatsakis]

@Manishearth

The correct way to handle this is to identify all the "important" nodes and do a macro check.

So actually struct_lint_level is probably not the right place. Rather, lint_level_at_node:

rust/src/librustc/ty/context.rs

Lines 2222 to 2247 in 27a046e

	pub fn lint_level_at_node(self, lint: &'static Lint, mut id: NodeId)
	-> (lint::Level, lint::LintSource)
	{
	// Right now we insert a `with_ignore` node in the dep graph here to
	// ignore the fact that `lint_levels` below depends on the entire crate.
	// For now this'll prevent false positives of recompiling too much when
	// anything changes.
	//
	// Once red/green incremental compilation lands we should be able to
	// remove this because while the crate changes often the lint level map
	// will change rarely.
	self.dep_graph.with_ignore(|| {
	let sets = self.lint_levels(LOCAL_CRATE);
	loop {
	let hir_id = self.hir.definitions().node_to_hir_id(id);
	if let Some(pair) = sets.level_and_source(lint, hir_id) {
	return pair
	}
	let next = self.hir.get_parent_node(id);
	if next == id {
	bug!("lint traversal reached the root of the crate");
	}
	id = next;
	}
	})
	}

That is, this seems like an identical problem to deciding whether the lint is enabled. We're basically talking about adding some measure of hygiene to that check, right?

(Also, I'm totally willing to be believe we should just work around this for now, and defer any more general changes to be done as part of a hygiene-overhaul-sort-of-thing.)

[nikomatsakis]

(One annoying problem: that fn does not presently have a span.)

[Manishearth]

It's almost identical to that problem, and I think we can get away with that approximation for now.

I can think of counterexamples but it just makes the lints slightly overconservative so nbd.

However, bear in mind that we may not want this at all for future compat lints (we already catalog these so we can special case this).

[nikomatsakis]

@Manishearth

However, bear in mind that we may not want this at all for future compat lints (we already catalog these so we can special case this).

Interesting point. Probably we don't, yes. =) (The story around future compatibility and dependencies is sort of complicated, I think, but see also #34596)

[Manishearth]

Issue for adding macro checks to lints at #48855

[reuvenpo]

This issue is also related to this topic: #51903.
Is this prioritised in any way? Seems like this had been known for a while now, and it's a shame because i would like to use this lint in my code.

[zackmdavis]

This is now fixed (lint was rewritten in #52069).

zmd@ReflectiveCoherence:~/Code/Misc$ cat 48385.rs 
#![feature(nll)]
#![deny(elided_lifetimes_in_paths)]

fn main() {
    format!("foo {}", 22);
}
zmd@ReflectiveCoherence:~/Code/Misc$ rustc +nightly --version
rustc 1.31.0-nightly (e7f5d4805 2018-10-18)
zmd@ReflectiveCoherence:~/Code/Misc$ rustc +nightly 48385.rs 
zmd@ReflectiveCoherence:~/Code/Misc$ echo $?
0