Skip to content

Conversation

ThibsG
Copy link
Contributor

@ThibsG ThibsG commented Oct 31, 2020

Fix bad suggestions when in macro expansion for deref_addrof and try_err lints.

Fixes: #6234
Fixes: #6242
Fixes: #6237

changelog: none

r? @llogiq

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 31, 2020
@ThibsG ThibsG marked this pull request as draft October 31, 2020 19:03
@ThibsG ThibsG force-pushed the DerefAddrOf branch 2 times, most recently from 764dfe5 to 8d12b64 Compare November 2, 2020 17:57
@ThibsG ThibsG changed the title [WIP] Fix two issues for deref_addrof lint Fix bad suggestions for deref_addrof and try_err lints Nov 2, 2020
@ThibsG ThibsG marked this pull request as ready for review November 2, 2020 17:59
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

I only have one nit and one question, otherwise this looks good to go. 👍

let expr_err_ty = cx.typeck_results().expr_ty(err_arg);

let origin_snippet = if err_arg.span.from_expansion() {
let origin_snippet = if err_arg.span.from_expansion() && !in_macro(expr.span) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unrelated?

Copy link
Contributor Author

@ThibsG ThibsG Nov 5, 2020

Choose a reason for hiding this comment

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

This !in_macro(expr.span) is here to prevent the use of the macro callsite for suggestion.
In this example:

macro_rules! try_validation {
    ($e: expr) => {{
        match $e {
            Ok(_) => 0,
            Err(_) => Err(1)?, // <-- lint here
        }
    }};
}

fn calling_macro() -> Result<i32, i32> {
    try_validation!(Ok::<_, i32>(5));
    Ok(5)
}

For information,err_arg.span is referring to the 1 from Err(1)?
We want the suggestion to be return Err(1) and not to be the macro call site (try_validation!(Ok::<_, i32>(5));).

@ThibsG
Copy link
Contributor Author

ThibsG commented Nov 5, 2020

Just saw #6237, it is the same kind of problem we're solving here.
If it's ok for you to fix this here, I can also work on this before we merge this.

@ThibsG
Copy link
Contributor Author

ThibsG commented Nov 5, 2020

Ok now it should be good for #6237 as well

@llogiq
Copy link
Contributor

llogiq commented Nov 9, 2020

Good work! Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2020

📌 Commit 83e75f9 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Nov 9, 2020

⌛ Testing commit 83e75f9 with merge d212c38...

@bors
Copy link
Contributor

bors commented Nov 9, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing d212c38 to master...

@bors bors merged commit d212c38 into rust-lang:master Nov 9, 2020
@ThibsG ThibsG deleted the DerefAddrOf branch November 9, 2020 07:36
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
4 participants