Forbid type ascriptions of place expressions in lvalue contexts #80788

b-naber · 2021-01-07T16:25:41Z

Opening to get some feedback.

edit: sorry lcnr, forgot the rust-highfive command

rust-highfive · 2021-01-07T16:25:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

lcnr · 2021-01-07T17:52:24Z

no worries 👍 this looks impressive for a first pr

r? @nikomatsakis

nikomatsakis

Here are some scenarios to test.

First, we want to test that every kind of borrow errors. Here are the ones I can think of:

let ref x = expr: T should error
(expr:T).as_ref() should error (invoking AsRef::as_ref from the standard library, which does auto-ref)
&(expr:T) should error

Next, for each kind of borrow, we want to try both a regular coercion and a coercion that is part of some larger replace. I would say cover these three scenarios:

&(expr:T).f should error
&(expr:T)[3] should error
&*(expr:T)

It's probably enough to cover those three scenarios for one kind of borrow, and then test that the other kind of borrows each handle (expr:T).f or one of the more complex cases, but ideal would be to test all for all.

compiler/rustc_typeck/src/check/type_ascription.rs

nikomatsakis · 2021-01-16T09:56:07Z

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -45,12 +45,18 @@ pub trait Delegate<'tcx> {
        &mut self,
        place_with_id: &PlaceWithHirId<'tcx>,
        diag_expr_id: hir::HirId,
+        borrow_expr: Option<&hir::Expr<'_>>,


Oh, I see, you added this parameter. I don't think we should do that. I think that place_with_id already carries a HirId and we ought to be able to use that to recover the expression.

I tried to use 'place_with_id' initially, but the mem_categorization functionality returns only the place expression id of a type ascription in some contexts (or rather in all contexts I tested) in diag_expr_id, i.e. for (x: T) diag_expr_id only contains the HirId of x, so I had to introduce this parameter to get information to decide whether we're dealing with a type ascription. I didn't look into the mem_categorization part to see why diag_expr_id only corresponds to the place expression part of a type ascription expression.

nikomatsakis · 2021-01-16T10:16:58Z

compiler/rustc_typeck/src/check/type_ascription.rs

+            place_with_id.place, borrow_expr, bk
+        );
+
+        if let Some(borrow) = borrow_expr {


This is the right general idea, but I'm worried it's not quite covering all the bases. This will work for something like

&(x:T)

but I don't think it would work for &(x:T).f. I'll try to create what seem to me to be a good set of test cases and link them below.

I'm also not sure if looking at borrow_expr is the right thing; the Place has a HirId, maybe we want to look at that. I have to check under what conditions borrow_expr is None.

Update: OK, I see you added borrow_expr. I think instead of doing that, you can use expect_expr to get the hir::Expr.

To manage the cases I was talking about, you'll need to use a HIR visitor or something to recursively walk the HIR structure.

I am, however, pondering whether the right fix here is to modify the Place structure a bit. Let me take a look at that.

So yes, what I think is that we should modify place construction -- I suspect we should

modify PlaceBase::Rvalue to contain a HirId

treat ascription as an rvalue, during place construction but...probably just everywhere -- that means modifying this function and perhaps this one, I haven't read that much into that code

check in this code where place.base is an rvalue and, if so, check whether its an ascription and report an error

See comment above for why I introduced borrow_expr. I also tried to somehow (currently don't remember what exactly I tried) to get access to a parent expression (the type ascription expression) for diag_expr_id, but wasn't able to do that.

I will take a look at your suggestions later.

b-naber · 2021-01-22T22:42:28Z

src/test/ui/type-ascription/as_ref.stderr

+LL | let r: &Foo = &mut foo;
+   | ^^^^^^^^^^^
+
+error: type ascriptions are not allowed in lvalue contexts


I think we can fix these repetitions of errors caused by nested delegate.borrow calls using the hir visitor. I want to see whether the general logic is correct now, before doing that.

nikomatsakis

This is looking pretty good-- I think it's missing a few things, though.

Specifically, I think we want to categorize place expressions as rvalues consistenly. That means we need to edit this function:

rust/compiler/rustc_hir/src/hir.rs

Line 1478 in 399b645

ExprKind::Type(ref e, _) => e.is_place_expr(allow_projections_from),

I was reading into the THIR/MIR construction code to see if any changes are needed there, but I don't think so.

nikomatsakis · 2021-02-06T11:43:07Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                let place_with_id = return_if_err!(self.mc.cat_expr(expr));
+
+                self.delegate_consume(&place_with_id, place_with_id.hir_id);
+                self.walk_expr(subexpr);


Hmm, this is interesting. It surprises me a bit but I actually think it's correct -- still, I would expect self.consume_expr(subexpr).

b-naber · 2021-02-11T15:49:17Z

src/test/ui/coercion/type-ascription/coerce-struct-fields-pass-in-let-stmt.rs

@@ -26,4 +24,5 @@ fn main() {
  let tup = (9, (3, &arr : &[u32]), &mut bar);


There is no error here, but two lines down there is, in what looks to be the same expression. Is this correct?

nikomatsakis

Looking good! Here are some thoughts/comments. Sorry about the delay. Thanks for pushing on this, @b-naber! I am beginning to think we'll be able to get this over the finish line soon.

nikomatsakis · 2021-02-11T20:31:21Z

compiler/rustc_hir/src/hir.rs

-            // operand. See:
-            // https://github.com/rust-lang/rfcs/blob/master/text/0803-type-ascription.md#type-ascription-and-temporaries
-            ExprKind::Type(ref e, _) => e.is_place_expr(allow_projections_from),
+            // We always treat type ascriptions as rvalues


probably worth adding a comment here like ", but if a type ascription is used in a reference, it will be an error"

nikomatsakis · 2021-02-21T11:37:50Z

compiler/rustc_middle/src/hir/place.rs

@@ -18,7 +18,7 @@ use rustc_target::abi::VariantIdx;
 )]
 pub enum PlaceBase {
    /// A temporary variable.


Can we extend this comment to indicate what the HirId is?

nikomatsakis · 2021-02-21T11:40:46Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

-            let possible_ancestor_proj_kinds =
-                capture.place.projections.iter().map(|proj| proj.kind).collect();
-            is_ancestor_or_same_capture(&possible_ancestor_proj_kinds, &hir_projections)
+        let possible_ancestor_proj_kinds =


what's going on with these changes :) they seem like random whitespace changes. Are they a side-effect of running rustfmt or something?

nikomatsakis · 2021-02-21T11:41:53Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

-                block.and(place_builder)
-            }
-            ExprKind::ValueTypeAscription { source, user_ty } => {
+            ExprKind::TypeAscription { source, user_ty } => {


Huzzah for code being deleted! 🎉

nikomatsakis · 2021-02-21T11:45:10Z

compiler/rustc_typeck/src/check/expr.rs

@@ -282,7 +277,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
            ExprKind::Cast(ref e, ref t) => self.check_expr_cast(e, t, expr),
            ExprKind::Type(ref e, ref t) => {


Pre-existing, but ExprKind::Type is really unclear to me. TypeAscription would be much better.

nikomatsakis · 2021-02-21T11:47:41Z

compiler/rustc_typeck/src/check/type_ascription.rs

+            if let Some(expr) = self.fcx.tcx.hir().maybe_expr(id) {
+                debug!("expr behind place: {:?}", expr);
+                self.finder.visit_expr(expr);
+                if let Some(ascr_expr) = self.finder.found_type_ascr() {


Nit: found_type_ascription -- spell it out

nikomatsakis · 2021-02-21T11:48:34Z

compiler/rustc_typeck/src/check/type_ascription.rs

+                debug!("expr behind place: {:?}", expr);
+                self.finder.visit_expr(expr);
+                if let Some(ascr_expr) = self.finder.found_type_ascr() {
+                    if let hir::ExprKind::Type(ref e, ref t) = ascr_expr.kind {


this is probably an assertion, right? maybe we can make found_type_ascr return the e and t directly instead?

nikomatsakis · 2021-02-21T11:48:50Z

compiler/rustc_typeck/src/check/type_ascription.rs

+                        err.emit();
+                    }
+                }
+                self.finder.reset();


this would be part of the method I suggested above

nikomatsakis · 2021-02-21T11:54:44Z

compiler/rustc_typeck/src/check/type_ascription.rs

+            assignee_place, diag_expr_id
+        );
+
+        if let PlaceBase::Rvalue(id) = assignee_place.place.base {


It'd be good to have a code example of what we are detecting and forbidding

nikomatsakis · 2021-02-21T11:54:56Z

compiler/rustc_typeck/src/check/type_ascription.rs

+            place_with_id.place, bk
+        );
+
+        if let PlaceBase::Rvalue(id) = place_with_id.place.base {


It'd be good to have a code example of what we are detecting and forbidding

camelid · 2021-03-12T03:42:57Z

Ping from triage: @b-naber could you address Niko's review? Thanks!

b-naber · 2021-03-12T14:40:49Z

@camelid Landing this will require the lang team to accept the updated RFC we will write for the type ascription feature. I haven't yet addressed Niko's review, because I wanted to wait for that approval. Is that ok?

nikomatsakis · 2021-03-13T12:48:36Z

We could close the PR and re-open it.

b-naber · 2021-03-13T12:52:30Z

OK, let's do that.

camelid · 2021-03-13T20:44:45Z

@camelid Landing this will require the lang team to accept the updated RFC we will write for the type ascription feature. I haven't yet addressed Niko's review, because I wanted to wait for that approval. Is that ok?

That's totally fine! I didn't see anything in the PR about waiting on an RFC, so I was confused.

We could close the PR and re-open it.

So you know for the future: that's not necessary, just indicate that the PR is waiting on something by adding S-blocked.

coerce type ascriptions by default

312c9e6

rust-highfive assigned lcnr Jan 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2021

This comment has been minimized.

Sign in to view

rust-highfive assigned nikomatsakis and unassigned lcnr Jan 7, 2021

b-naber mentioned this pull request Jan 11, 2021

Permit Coercions in Type Ascriptions #79730

Closed

nikomatsakis requested changes Jan 16, 2021

View reviewed changes

b-naber force-pushed the type_ascr_lvalue branch 2 times, most recently from 253f849 to a37d16d Compare January 22, 2021 20:33

This comment has been minimized.

Sign in to view

b-naber force-pushed the type_ascr_lvalue branch from a37d16d to b0e1733 Compare January 22, 2021 21:01

This comment has been minimized.

Sign in to view

b-naber commented Jan 22, 2021

View reviewed changes

b-naber added 2 commits January 24, 2021 12:25

forbid type ascriptions in lvalue contexts

d70f5bc

add tests

d2485ca

b-naber force-pushed the type_ascr_lvalue branch from b0e1733 to d2485ca Compare January 24, 2021 11:26

This comment has been minimized.

Sign in to view

nikomatsakis requested changes Feb 6, 2021

View reviewed changes

b-naber added 4 commits February 11, 2021 12:46

get rid of duplicate errors

ad7ba4d

treat type ascriptions as rvalues

40af99d

update tests

f699d41

consume subexpr of type ascription in expr_use_visitor

597cb28

b-naber force-pushed the type_ascr_lvalue branch from 9007ab3 to 597cb28 Compare February 11, 2021 15:43

b-naber commented Feb 11, 2021

View reviewed changes

b-naber changed the title ~~[WIP] Forbid type ascriptions of place expressions in lvalue contexts~~ Forbid type ascriptions of place expressions in lvalue contexts Feb 17, 2021

b-naber marked this pull request as ready for review February 17, 2021 16:18

nikomatsakis approved these changes Feb 21, 2021

View reviewed changes

camelid added the A-type-system Area: Type system label Mar 12, 2021

camelid 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2021

b-naber closed this Mar 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forbid type ascriptions of place expressions in lvalue contexts #80788

Forbid type ascriptions of place expressions in lvalue contexts #80788

b-naber commented Jan 7, 2021 •

edited

Loading

rust-highfive commented Jan 7, 2021

This comment has been minimized.

lcnr commented Jan 7, 2021

nikomatsakis left a comment

nikomatsakis Jan 16, 2021

b-naber Jan 16, 2021 •

edited

Loading

nikomatsakis Jan 16, 2021

nikomatsakis Jan 16, 2021

nikomatsakis Jan 16, 2021

b-naber Jan 16, 2021

This comment has been minimized.

This comment has been minimized.

b-naber Jan 22, 2021

This comment has been minimized.

nikomatsakis left a comment •

edited

Loading

nikomatsakis Feb 6, 2021

b-naber Feb 11, 2021 •

edited

Loading

nikomatsakis left a comment •

edited

Loading

nikomatsakis Feb 11, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

nikomatsakis Feb 21, 2021

camelid commented Mar 12, 2021

b-naber commented Mar 12, 2021

nikomatsakis commented Mar 13, 2021

b-naber commented Mar 13, 2021

camelid commented Mar 13, 2021

		@@ -26,4 +24,5 @@ fn main() {
		let tup = (9, (3, &arr : &[u32]), &mut bar);

		@@ -282,7 +277,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
		ExprKind::Cast(ref e, ref t) => self.check_expr_cast(e, t, expr),
		ExprKind::Type(ref e, ref t) => {

Forbid type ascriptions of place expressions in lvalue contexts #80788

Forbid type ascriptions of place expressions in lvalue contexts #80788

Conversation

b-naber commented Jan 7, 2021 • edited Loading

rust-highfive commented Jan 7, 2021

This comment has been minimized.

lcnr commented Jan 7, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

nikomatsakis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

nikomatsakis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Mar 12, 2021

b-naber commented Mar 12, 2021

nikomatsakis commented Mar 13, 2021

b-naber commented Mar 13, 2021

camelid commented Mar 13, 2021

b-naber commented Jan 7, 2021 •

edited

Loading

b-naber Jan 16, 2021 •

edited

Loading

nikomatsakis left a comment •

edited

Loading

b-naber Feb 11, 2021 •

edited

Loading

nikomatsakis left a comment •

edited

Loading