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

Make PlaceMention a non-mutating use. #110826

Merged
merged 3 commits into from
May 4, 2023
Merged

Conversation

cjgillot
Copy link
Contributor

Fixes #110781

r? @JakobDegen

I don't agree with your statement in #110781 (comment). I suggest that we start fixing PlaceContext to be accurate enough for optimizations to use it. This structure is very convenient to use in visitors, and we perhaps have an opportunity to make it less of a footgun.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@@ -1251,6 +1251,8 @@ pub enum NonMutatingUseContext {
UniqueBorrow,
/// AddressOf for *const pointer.
AddressOf,
/// PlaceMention statement.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a clarifying comment on why this is here, given that it used to be in the wrong category.

Inspect | Copy | Move | SharedBorrow | ShallowBorrow | UniqueBorrow | AddressOf
| Projection,
Inspect | Copy | Move | PlaceMention | SharedBorrow | ShallowBorrow | UniqueBorrow
| AddressOf | Projection,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're going from invariant to covariant here? If intentional, probably a behavior change that would need a types FCP?

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'd consider this more of a bugfix for #104844, which is still on nightly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm going to cc @rust-lang/types here to get some eyes on this. From my understanding it's probably fine, but I'm definitely not a qualified reviewer

Copy link
Contributor

Choose a reason for hiding this comment

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

being covariant is fine here 👍

@JakobDegen
Copy link
Contributor

I suggest that we start fixing PlaceContext to be accurate enough for optimizations to use it.

I'd be very happy to do that if we can. My concern is just that PlaceMention is (presumably) the way it is because it makes sense elsewhere. If we start redefining it to be useful to opts, do we just move the bugs to borrowck?

@cjgillot cjgillot force-pushed the place-mention-use branch from a82e082 to 4ec76df Compare April 29, 2023 16:16
@JakobDegen
Copy link
Contributor

JakobDegen commented Apr 29, 2023

lgtm modulo the variance change

@RalfJung
Copy link
Member

RalfJung commented May 4, 2023

@JakobDegen

lgtm modulo the variance change

@lcnr

being covariant is fine here +1

@bors r=JakobDegen,lcnr

@bors
Copy link
Contributor

bors commented May 4, 2023

📌 Commit 4ec76df has been approved by JakobDegen,lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108865 (Add a `sysroot` crate to represent the standard library crates)
 - rust-lang#110651 (libtest: include test output in junit xml reports)
 - rust-lang#110826 (Make PlaceMention a non-mutating use.)
 - rust-lang#110982 (Do not recurse into const generic args when resolving self lifetime elision.)
 - rust-lang#111009 (Add `ascii::Char` (ACP#179))
 - rust-lang#111100 (check array type of repeat exprs is wf)
 - rust-lang#111186 (Add `is_positive` method for signed non-zero integers.)
 - rust-lang#111201 (bootstrap: add .gitmodules to the sources)

Failed merges:

 - rust-lang#110954 (Reject borrows of projections in ConstProp.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0ac8ebd into rust-lang:master May 4, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 4, 2023
@cjgillot cjgillot deleted the place-mention-use branch May 4, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UB: edition 2015/18 with -Zmir-opt-level=2: destructing tuple inside closure
6 participants