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

declare_interior_mutable_const, borrow_interior_mutable_const: resolve <T as Trait>::AssocT projections #14125

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

GrigorenkoPV
Copy link
Contributor

@GrigorenkoPV GrigorenkoPV commented Jan 31, 2025

changelog: [declare_interior_mutable_const, borrow_interior_mutable_const]: resolve <T as Trait>::AssocT projections


This came up during rust-lang/rust#130543 where we have <T as AtomicPrimitive>::Assoc = AtomicT instead of just AtomicT and clippy failed to resolve that properly.

This really needs a review, because

  • I don't know if try_normalize_erasing_regions is the right thing to call here.
  • I'm not sure if I peel off the correct amount of ValTree::Branch layers (I think I do).

Also, shouldn't this lint's infrastructure rely on Freeze trait (rust-lang/rust#121675) instead of hardcoding a list of known-to-be-interior-mutable types?


Previously filed this in the main rust repo (rust-lang/rust#136369), was asked to do it here instead (rust-lang/rust#136369 (comment)).

@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 31, 2025
@GrigorenkoPV GrigorenkoPV changed the title Resolve projections during internal mutability analysis declare_interior_mutable_const, borrow_interior_mutable_const: resolve <T as Trait>::AssocT projections Jan 31, 2025
@blyxyas
Copy link
Member

blyxyas commented Feb 9, 2025

Also, shouldn't this lint's infrastructure rely on Freeze trait (rust-lang/rust#121675) instead of hardcoding a list of known-to-be-interior-mutable types?

This is a great idea! Would you like to implement it here, in another PR or would you like for someone else (probably me) to implement it?

@GrigorenkoPV
Copy link
Contributor Author

Also, shouldn't this lint's infrastructure rely on Freeze trait (rust-lang/rust#121675) instead of hardcoding a list of known-to-be-interior-mutable types?

This is a great idea! Would you like to implement it here, in another PR or would you like for someone else (probably me) to implement it?

I think I might give it a try myself, but definitely in a separate PR, because for now I just want to unblock rust-lang/rust#136316

@blyxyas
Copy link
Member

blyxyas commented Feb 9, 2025

Oh I didn't know this was a blocking PR, sorry for the wait. I'll be swift in the reviews then.

@GrigorenkoPV
Copy link
Contributor Author

GrigorenkoPV commented Feb 9, 2025

Oh I didn't know this was a blocking PR, sorry for the wait. I'll be swift in the reviews then.

No problem, it's nothing urgent. In fact it is not even 100% blocking. We can just avoid using Atomic<T> in places where clippy does not like it for now, because eventually there will be a switch from Atomic<T> being an alias for AtomicT via projections to AtomicT being an alias for Atomic<T> with (afaict) no projections involved.

Copy link
Member

@blyxyas blyxyas 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 transparency, I had to open a thread on Zulip asking for some assistance because try_normalize_erasing_regions was out of my area of expertise, and with that newly-found knowledge, I think this is a good use of the function.

Also, compiler-errors took a look at this and he also thinks it's fine.

@blyxyas blyxyas added this pull request to the merge queue Feb 11, 2025
Merged via the queue into rust-lang:master with commit c40898e Feb 11, 2025
11 of 17 checks passed
@GrigorenkoPV GrigorenkoPV deleted the freeze-projections branch February 11, 2025 20:03
@GrigorenkoPV
Copy link
Contributor Author

GrigorenkoPV commented Feb 15, 2025

Also, compiler-errors took a look at this and he also thinks it's fine.

Yay, that is reassuring.


As for the Freeze, I've opened an issue (#14222) so at least this does not get lost to time. Hopefully, I will be able to work on it in the nearest future. If not, I guess others can claim it.

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.

3 participants