Skip to content

[arithmetic_side_effects] Fix #10792 #10793

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
Jun 19, 2023
Merged

Conversation

c410-f3r
Copy link
Contributor

Fix #10792

changelog: [`arithmetic_side_effects`]: Retrieve field values of structures that are in constant environments

@rustbot
Copy link
Collaborator

rustbot commented May 17, 2023

r? @Manishearth

(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 May 17, 2023
_ => None,
}
mir::ConstantKind::Val(ConstValue::Scalar(Scalar::Int(int)), _) => match result.ty().kind() {
ty::Adt(_, _) => Some(Constant::Int(int.assert_bits(int.size()))),
Copy link
Contributor Author

@c410-f3r c410-f3r May 17, 2023

Choose a reason for hiding this comment

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

Initially I thought of bringing the field's ident from ExprKind::Field to here in order to search the actual type with something like def.all_fields().find(|field| ... match ident logic ...) but then I realized that someone probably already did this in a clever way.

The problem is, I don't know any other way. Do you have any thoughts about this matter?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @rust-lang/clippy, has someone maybe worked mir and has an idea how to solve this? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My search in the deepest and darkest parts of Rustc wasn't a success and since no one else replied, I moved forward with the initial idea. Hey, at least it is working 🙄

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to add a variant to Constant to hold struct types and then resolve fields as they come up. The variant can just be a reference to an allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry @Jarcho, can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing he recommends adding a new variant like Constant::Field(bits-as-int) and let the lint handle the type of the bits inside. Does that make sense? And @Jarcho was this what you meant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add something like Constant::Adt(ConstantKind<'tcx>), have Constant::fetch_path return that when needed and then fields as needed in Constant::expr.

You need to be handle any nested fields (e.g. a.b.c.d) as well arrays and slices (can be done in a different pr).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add something like Constant::Adt(ConstantKind<'tcx>), have Constant::fetch_path return that when needed and then fields as needed in Constant::expr.

Done as requested

@Manishearth
Copy link
Member

r? @xFrednet

(limited internet this week, redirecting reviews)

@rustbot rustbot assigned xFrednet and unassigned Manishearth May 17, 2023
@c410-f3r c410-f3r force-pushed the bbbbbbbbbbb branch 2 times, most recently from fa54b19 to 8f1ed40 Compare May 24, 2023 13:16
@xFrednet
Copy link
Member

This version looks good to me. Thank you for figuring this out :)

Lord bors, would you be so kind and merge this PR :)

@bors
Copy link
Contributor

bors commented Jun 19, 2023

📌 Commit dbe4057 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 19, 2023

⌛ Testing commit dbe4057 with merge c8c03ea...

@bors
Copy link
Contributor

bors commented Jun 19, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jun 19, 2023

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

@bors bors merged commit c8c03ea into rust-lang:master Jun 19, 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.

[arithmetic_side_effects] Constant fields are ignored
6 participants