Skip to content

Add a raw "address of" operator #64588

New issue

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

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
Dec 20, 2019
Merged

Conversation

matthewjasper
Copy link
Contributor

  • Parse and feature gate &raw [const | mut] expr (feature gate name is raw_address_of)
  • Add mir::Rvalue::AddressOf
  • Use the new Rvalue for:
    • the new syntax
    • reference to pointer casts
    • drop shims for slices and arrays
  • Stop using mir::Rvalue::Cast with a reference as the operand
  • Correctly evaluate mir::Rvalue::{Ref, AddressOf} in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc #64490

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@rust-highfive

This comment has been minimized.

_ => None,
};

if let Some((ctx, place)) = borrow_ctx {
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ecstatic-morse @oli-obk re. the qualification code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I think none of it should be changed and we should just treat raw borrows as unpromotable (because any potentially promotable things had to error out before)

@Centril

This comment has been minimized.

@rust-highfive rust-highfive assigned RalfJung and unassigned cramertj Sep 18, 2019
@Centril
Copy link
Contributor

Centril commented Sep 18, 2019

Thanks for the PR. I've made an initial review above ^--- (but Ralf should also review this).

Some more remarks on the PR description:

  • Use the new Rvalue for:

    • reference to pointer casts

This is not part of the RFC and should remain UB.

  • drop shims for slices and arrays

Can you elaborate on this please? Is this a spec change or just internal refactoring?

  • Stop using mir::Rvalue::Cast with a reference as the operand

Seems like the same as &x as *const _ using Rvalue::AddrOf (i.e. not part of the RFC).


Can you please also extend the unused_parens lint to handle &raw [const|mut] $place_expr?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 19, 2019

@matthewjasper You need to handle Rvalue::AddressOf(_, _) here and here as well. This is the part of the code that forbids taking &mut references in const bodies. We need to forbid anything that allows mutation through indirection, since that can cause an Option<NeedsDropType> to go from None to Some.

@matthewjasper
Copy link
Contributor Author

matthewjasper commented Sep 19, 2019

This is not part of the RFC and should remain UB.

What should be UB? &x as *const _ still is and x as *const _ could never cause UB.

Can you elaborate on this please? Is this a spec change or just internal refactoring?

It's a refactoring. If the precise stacked borrows effects of slice/array drop shims is defined, then maybe it's a change, but not a very big one since the parameter is a mutable reference, and the calls to drop the elements create mutable references.

Can you please also extend the unused_parens lint to handle &raw [const|mut] $place_expr?

This can be a follow up. The (lack of) behavior here is consistent with & AFAICT.

@Centril
Copy link
Contributor

Centril commented Sep 19, 2019

What should be UB? &x as *const _ still is and x as *const _ could never cause UB.

Oh I see; seems I misread. Can you add a mir-opt test to that effect?

This can be a follow up. The (lack of) behavior here is consistent with & AFAICT.

Sure, can you add a ticket to the tracking issue when this PR is merged?

@matthewjasper

This comment has been minimized.

@Centril

This comment has been minimized.

@bors

This comment has been minimized.

@JohnCSimon

This comment has been minimized.

@ecstatic-morse
Copy link
Contributor

@matthewjasper @RalfJung Some tests need to be added to ensure that &raw mut is forbidden in const contexts. Also, there's now two validation passes (one uses dataflow to support #49146) that are checked against one another for compatibility. Once &raw mut references are handled, I'm happy to port the changes to the new validator in librustc_mir/transform/check_consts/validation.rs.

@matthewjasper matthewjasper added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2019
@matthewjasper

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
@matthewjasper
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Dec 19, 2019

📌 Commit a749116 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2019
@bors

This comment has been minimized.

@Centril

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Dec 20, 2019
…-obk

Add a raw "address of" operator

* Parse and feature gate `&raw [const | mut] expr` (feature gate name is `raw_address_of`)
* Add `mir::Rvalue::AddressOf`
* Use the new `Rvalue` for:
    * the new syntax
    * reference to pointer casts
    * drop shims for slices and arrays
* Stop using `mir::Rvalue::Cast` with a reference as the operand
* Correctly evaluate `mir::Rvalue::{Ref, AddressOf}` in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc rust-lang#64490
Centril added a commit to Centril/rust that referenced this pull request Dec 20, 2019
…-obk

Add a raw "address of" operator

* Parse and feature gate `&raw [const | mut] expr` (feature gate name is `raw_address_of`)
* Add `mir::Rvalue::AddressOf`
* Use the new `Rvalue` for:
    * the new syntax
    * reference to pointer casts
    * drop shims for slices and arrays
* Stop using `mir::Rvalue::Cast` with a reference as the operand
* Correctly evaluate `mir::Rvalue::{Ref, AddressOf}` in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc rust-lang#64490
bors added a commit that referenced this pull request Dec 20, 2019
Rollup of 5 pull requests

Successful merges:

 - #64588 (Add a raw "address of" operator)
 - #67031 (Update tokio crates to latest versions)
 - #67131 (Merge `TraitItem` & `ImplItem into `AssocItem`)
 - #67354 (Fix pointing at arg when cause is outside of call)
 - #67363 (Fix handling of wasm import modules and names)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

@bors bors merged commit a749116 into rust-lang:master Dec 20, 2019
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2019
@matthewjasper matthewjasper deleted the mir-address-of branch December 20, 2019 19:45
@RalfJung
Copy link
Member

This is awesome, thanks so much. :)

Comment on lines +139 to 142
// Retag-as-raw after escaping to a raw pointer.
StatementKind::Assign(box (ref place, Rvalue::AddressOf(..))) => {
(RetagKind::Raw, place.clone())
}
Copy link
Member

Choose a reason for hiding this comment

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

So this is doing that for all &raw now, right? Certainly a reasonable start, though it doesn't seem to be entirely equivalent to the previous operation which also made sure that the "input" is a reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There wasn't any way to write &raw const *x where x is a raw pointer before.

bors added a commit to rust-lang/miri that referenced this pull request Jan 30, 2020
Test raw-ptr-cast without reborrow

With rust-lang/rust#64588 landed, we can finally test these things adequately. :)
bors added a commit to rust-lang/miri that referenced this pull request Jan 30, 2020
Test raw-ptr-cast without reborrow

With rust-lang/rust#64588 landed, we can finally test these things adequately. :)
@RalfJung RalfJung mentioned this pull request Jul 27, 2020
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
F-raw_ref_op `#![feature(raw_ref_op)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.