Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FP in unnecessary_safety_comment #10106

Merged
merged 1 commit into from
Jan 22, 2023
Merged

Conversation

koka831
Copy link
Contributor

@koka831 koka831 commented Dec 21, 2022

Fix #10084

changelog: FP: [unnecessary_safety_comment]: No longer lints code inside macros
#10106

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2022

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 21, 2022
@Alexendoo
Copy link
Member

Thanks, there seems to be other uses of in_external_macro in the lint, do those also need changing or do they work within macro definitions?

@koka831
Copy link
Contributor Author

koka831 commented Dec 22, 2022

Thank you for quick review:)
Yes, other is_external_macro is necessary to check inside of macro definition so I keep left them and minimum fix.

However some suggestions can be improved.
Other lint method using is_external_macro emits a warning with caller:

    macro_rules! with_safety_comment {
        ($t:ty) => {
            // Safety: unnecessary
            impl T for $t {}
        };
    }

    with_safety_comment!(i32);
error: impl has unnecessary safety comment
  --> $DIR/unnecessary_safety_comment.rs:33:13
   |
LL |             impl T for $t {}
   |             ^^^^^^^^^^^^^^^^
...
LL |     with_safety_comment!(i32);
   |     ------------------------- in this macro invocation
   |
help: consider removing the safety comment

@Alexendoo Is there a way to check if a given span is in a macro definition?

@Alexendoo
Copy link
Member

Alexendoo commented Dec 30, 2022

The span.from_expansion() you have checks if it's in a macro, in_external_macro permits spans from macros in the same crate. I'd be inclined to say we should swap them all over to .from_expansion() since where the safety comment should go is ambiguous for macros

@koka831
Copy link
Contributor Author

koka831 commented Jan 7, 2023

I replaced all in_external_macro with span#from_expansion() and removed unused method.

@Alexendoo
Copy link
Member

Alexendoo commented Jan 8, 2023

Oh hm, didn't realise the file was also used for UNDOCUMENTED_UNSAFE_BLOCKS, in that case we wouldn't want to remove the macro handling

Poking around a bit, I think the issue is actually in expr_has_unnecessary_safety_comment, it is returning Some(..) for expressions inside an unsafe block, I think a solution there would be to do a cx.tcx.hir().parent_iter(expr.hir_id) and if any of the nodes are

Node::Block(Block {
    rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
    ..
})

then return None early

@koka831
Copy link
Contributor Author

koka831 commented Jan 9, 2023

Ah I get it. sorry for bothering you, I really appreciate your help.

it is returning Some(..) for expressions inside an unsafe block

it is indeed the root problem.

@Alexendoo
Copy link
Member

Thanks! Sorry for the delay, I didn't realise that you updated the code to cover already 😅

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 22, 2023

📌 Commit 1a7ef02 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 22, 2023

⌛ Testing commit 1a7ef02 with merge a9c251f...

@bors
Copy link
Collaborator

bors commented Jan 22, 2023

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

1 similar comment
@bors
Copy link
Collaborator

bors commented Jan 22, 2023

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

@bors bors merged commit a9c251f into rust-lang:master Jan 22, 2023
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_safety_comment false positive in macro
4 participants