Move unimplemented to an EarlyLintPass

[maxencefrenette]

fixes #3528
WIP

Hi all ! As suggested in the issue thread, I moved the unimplemented lint to an EarlyLintPass. It works great and makes the code much simpler. However, I don't know how to move the panic lint to an EarlyLintPass. I see a couple options right now.

• Keep the LateLintPass for it and move it to another file.
• Adapt the check_tts() function from clippy_lints/src/write.rs. However, I couldn't fully grasp how to adapt it to work in this case.
• Something much simpler that I'm not seeing right now.

[flip1995]

The check_tts function of write.rs is way to complex for this use case, but as you noticed correctly you can copy some code from there.

You basically just need this part of the check_tts function:

rust-clippy/clippy_lints/src/write.rs

Line 326 in 01ab9fe

let mut parser = parser::Parser::new(&cx.sess.parse_sess, tts, None, false, false, None);

rust-clippy/clippy_lints/src/write.rs

Lines 339 to 342 in 01ab9fe

	let (fmtstr, fmtstyle) = match parser.parse_str().map_err(|mut err| err.cancel()) {
	Ok((fmtstr, fmtstyle)) => (fmtstr.to_string(), fmtstyle),
	Err(_) => return (None, expr),
	};

For panic!("foo {}", a), this will parse you the string "foo {}", which you can then examine with the code

rust-clippy/clippy_lints/src/panic_unimplemented.rs

Lines 87 to 89 in 01ab9fe

	let string = string.as_str().replace("{{", "").replace("}}", "");
	if let Some(par) = string.find('{');
	if string[par..].contains('}');

This will tell you if there is a {} in the string.

After this, you can check if there are parameters with

rust-clippy/clippy_lints/src/write.rs

Lines 334 to 336 in 01ab9fe

	if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() {
	return (None, expr);
	}

and

// You only need to try to parse one expression and not all format parameters
parser.parse_expr().map_err(|mut err| err.cancel()).is_some()

You pretty much can put this all together in one if_chain!

[ghost]

Did the problem with EarlyLintExpansion and nested macros get fixed? I don't think we should use pre-expansion lints at all until that's resolved.

[flip1995]

The problem got not fixed and probably won't be fixed.

cc #3217 (comment)

We should limit PreExpansionPass to lints that really only look for a specific macro call.

This is the case here. So it should be fine to turn this into a PreExpansionPass

@maxencefrenette Could you add following test:

macro_rules! outer_macro {
    ($e:expr) => {
        $e;
    };
}

// ...
outer_macro!(unimplemented!());

This unimplemented! call should get linted. If that's not the case the problem @mikerite described is more severe than I thought.

[maxencefrenette]

The problem got not fixed and probably won't be fixed.

cc #3217 (comment)

We should limit PreExpansionPass to lints that really only look for a specific macro call.

This is the case here. So it should be fine to turn this into a PreExpansionPass

@maxencefrenette Could you add following test:

macro_rules! outer_macro {
    ($e:expr) => {
        $e;
    };
}

// ...
outer_macro!(unimplemented!());

This unimplemented! call should get linted. If that's not the case the problem @mikerite described is more severe than I thought.

Interesting stuff. I'll for sure add that test when I get back to it. Thanks a lot for your help @flip1995 !

[maxencefrenette]

Everything passes locally and I updated the .stderr for the test. @mikerite @flip1995 what you feared happened. It doesn't detect nested macros, as can be seen in the test files.

[flip1995]

It doesn't detect nested macros, as can be seen in the test files.

Well that's bad :( I'm not sure what's the best course of action here is... Let's be naive: Can we use the check_mac function in an EarlyLintPass after macro expansion?

[bors]

☔ The latest upstream changes (presumably #4251) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

Thanks for the work! But I don't think moving this lint to a pre_expansion_pass is the right thing to do here. Since this is stale for some time, I'm going ahead and close this PR.

[HKalbasi]

finished in #5811
@rustbot label -S-inactive-closed