Skip to content

fix fp when [undocumented_unsafe_blocks] not able to detect comment on globally defined const/static variables #11375

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

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Aug 22, 2023

fixes: #11246

changelog: fix detection on global variables for [undocumented_unsafe_blocks]

@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2023

r? @Centri3

(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 Aug 22, 2023
span = e.span;
// Note: setting this to `false` is to making sure a "in-function defined"
// const/static variable not mistakenly processed as global variable,
// since global var doesn't have an `Expr` parent as its parent???
Copy link
Member Author

@J-ZhengLi J-ZhengLi Aug 22, 2023

Choose a reason for hiding this comment

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

I'm only 70% sure of this... so, can someone please confirm this for me? 😆

(also please forgive my terrible English...)

Copy link
Member

@Centri3 Centri3 Aug 24, 2023

Choose a reason for hiding this comment

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

I think this is because a const's initializer is a body, and when getting a body's value's parent it'll get wherever the body's used; see the HIR for const A: u32 = { 0 }: https://rust.godbolt.org/z/3cY4vaq6T

So this is getting the Expr every other time, yet here, it's getting the Const/Static, then nothing, afaik? So maybe we just need to handle ItemKind::Const and ItemKind::Static parents as well.

I think we already do that so maybe the issue is somewhere within that block.

Copy link
Member Author

@J-ZhengLi J-ZhengLi Aug 25, 2023

Choose a reason for hiding this comment

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

wait a minute, I think I unintentionally increase the search span for other scenarios (actually not, but the search span for module level const/static was too big because I forgor to add a break), let me see if I can find a better solution

@Centri3
Copy link
Member

Centri3 commented Aug 24, 2023

Interestingly, unnecessary_safety_comment doesn't have the same issue despite mostly sharing the same code. I'll see if we can replicate what it does since it looks like there is an issue in block_has_safety_comment or block_parents_have_safety_comment that stmt_has_safety_comment doesn't have.

@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Aug 25, 2023

Interestingly, unnecessary_safety_comment doesn't have the same issue despite mostly sharing the same code. I'll see if we can replicate what it does since it looks like there is an issue in block_has_safety_comment or block_parents_have_safety_comment that stmt_has_safety_comment doesn't have.

it is the get_body_search_span function that's causing troubles, I think...

the rest of change I've made was mainly refractoring. tbh I had trouble understanding most of its code lol

@J-ZhengLi J-ZhengLi marked this pull request as draft August 25, 2023 03:24
and narrow search span when const/static items are in a mod block
@J-ZhengLi J-ZhengLi marked this pull request as ready for review August 25, 2023 07:07
@Centri3
Copy link
Member

Centri3 commented Sep 4, 2023

This version looks fine to me. I don't particularly like maybe_global_var but ah well; I have nothing to suggest over it.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2023

📌 Commit 6eec4a3 has been approved by Centri3

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 4, 2023

⌛ Testing commit 6eec4a3 with merge bcf856b...

@bors
Copy link
Contributor

bors commented Sep 4, 2023

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

@bors bors merged commit bcf856b into rust-lang:master Sep 4, 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.

undocumented_unsafe_blocks false positive on consts
4 participants