Skip to content

Conversation

kraktus
Copy link
Contributor

@kraktus kraktus commented Sep 16, 2022

fix #9485, fix #9439

I didn't find a way to add a test since fps occur due to an external crate.

changelog: [unnecessary_lazy_eval] Do not lint in external macros

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 16, 2022
@dswij
Copy link
Member

dswij commented Sep 16, 2022

@kraktus FYI, I've recently added a test for Derive macro FP here. Maybe we can do something similar?

@kraktus
Copy link
Contributor Author

kraktus commented Sep 16, 2022

Thanks, I've identified the source of the false positive in rocket codebase: https://github.com/SergioBenitez/Rocket/blob/707936177a7fea6d8eca103cf0b3601ca45fe657/core/codegen/src/derive/from_form.rs#L287

Following your example I've tried to reproduce the issue with a dummy derive macro

#[proc_macro_derive(LazyEvalDerive)]
pub fn unnecessary_lazy_eval(_: TokenStream) -> TokenStream {
    quote!(
        pub fn unnecessary_then() {
            Some(1).unwrap_or_else(|| 2); // should not lint because in external macro
        }
    )
}

but it's not linting it even before my patch, so unlikely my fix is working as intended

@Jarcho
Copy link
Contributor

Jarcho commented Sep 16, 2022

The fix for this needs is_from_proc_macro. You can use the with_span! macro in tests using aux-build:proc_macro_with_span.rs.

@kraktus kraktus force-pushed the lazy_eval branch 2 times, most recently from 4cbd911 to f289353 Compare September 16, 2022 17:34
@bors
Copy link
Contributor

bors commented Sep 29, 2022

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

Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for this!

@giraffate Hope you don't mind me stealing this review 😁

@dswij
Copy link
Member

dswij commented Sep 30, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2022

📌 Commit 6ec7759 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 30, 2022

⌛ Testing commit 6ec7759 with merge 68408c5...

@bors
Copy link
Contributor

bors commented Sep 30, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 68408c5 to master...

@bors bors merged commit 68408c5 into rust-lang:master Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary closure used with bool::then in field definition unnecessary_lazy_evaluations gets triggered by a derive macro
6 participants