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

MIR borrowck: print lvalues in error messages in the same way that the AST borrowck #44985

Merged
merged 12 commits into from Oct 12, 2017

Conversation

Projects
None yet
6 participants
@zilbuz
Copy link
Contributor

zilbuz commented Oct 2, 2017

Fix #44974

  • Print fields with .name rather than .<num>
  • Autoderef values if followed by a field or an index
  • Output [..] when borrowing inside a slice
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 2, 2017

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@zilbuz

This comment has been minimized.

Copy link
Contributor Author

zilbuz commented Oct 2, 2017

I'd like some feedback on this PR. I can't think of any example that trigger the messages marked debug!() in my diff of borrow_check.rs. Can someone give me some example ?

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 2, 2017

@zilbuz I'm about to hit the sack, but if you use the following input code, I think you should see some examples of some of the cases you are wondering about:

#![feature(slice_patterns)]

fn main() {
    #[derive(Debug)]
    enum E<X> { A(X), B { x: X } }

    #[derive(Debug)]
    struct S<'a, X: 'a, Y>{ x: &'a [X], y: (Y, Y), };

    let e = E::A(3);

    match e {
        E::A(ref ax) => println!("e.ax: {:?}", ax),
        E::B { x: ref bx } => println!("e.bx: {:?}", bx),
    }

    let s = S { x: &[1, 2, 3,], y: (999, 998) };

    match s {
        S  {
            x: &[ref x0, ref xn..],
            y: (ref y0, ref y1)
        } => {
            println!("s.x0: {:?} s.xn: {:?}", x0, xn);
            println!("s.y0: {:?}, s.y1: {:?}", y0, y1);
        }

        _ => panic!("other case"),
    }
}

zilbuz added some commits Oct 2, 2017

mir-borrowck: print values in error messages in the same way that the…
… AST borrowck

- Print fields with `.name` rather than `.<num>`
- Autoderef values if followed by a field or an index
mir-borrowck: Autoderef values followed by a constant index, and fix …
…reported lvalue for constant index

Previously the constant index was reported as `[x of y]` or `[-x of y]` where
`x` was the offset and `y` the minimum length of the slice. The minus sign
wasn't in the right case since for `&[_, x, .., _, _]`, the error reported was
`[-1 of 4]`, and for `&[_, _, .., x, _]`, the error reported was `[2 of 4]`.
This commit fixes the sign so that the indexes 1 and -2 are reported, and
remove the ` of y` part of the message to make it more succinct.

@zilbuz zilbuz force-pushed the zilbuz:issue-44974 branch from 148856c to e32e81c Oct 6, 2017

@zilbuz zilbuz changed the title [WIP] MIR borrowck: print lvalues in error messages in the same way that the AST borrowck MIR borrowck: print lvalues in error messages in the same way that the AST borrowck Oct 6, 2017

@zilbuz

This comment has been minimized.

Copy link
Contributor Author

zilbuz commented Oct 6, 2017

r? @pnkfelix

The only commit untested is e32e81c. I couldn't find a way to access a field through a subslice. I'd have to be able to do something like [1..3].x or xn @ [F { x }..] in a match maybe...

// Static and field from struct
unsafe {
let _x = sfoo.x();
sfoo.x; //[mir]~ ERROR cannot use `sfoo.x` because it was mutably borrowed (Mir)

This comment has been minimized.

@pnkfelix

pnkfelix Oct 9, 2017

Member

oh interesting...

(I think this represents a bug in MIR-borrowck. But fixing that is orthogonal to this PR in any case.)

This comment has been minimized.

@pnkfelix

pnkfelix Oct 9, 2017

Member

(likewise for all the other cases below that are paths based off of static variables)

This comment has been minimized.

@pnkfelix

pnkfelix Oct 9, 2017

Member

Filed as #45129

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 9, 2017

@bors r+

thanks for all the work here @zilbuz !

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2017

📌 Commit e32e81c has been approved by pnkfelix

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2017

⌛️ Testing commit e32e81c with merge fd85391...

bors added a commit that referenced this pull request Oct 10, 2017

Auto merge of #44985 - zilbuz:issue-44974, r=pnkfelix
MIR borrowck: print lvalues in error messages in the same way that the AST borrowck

Fix #44974

- Print fields with `.name` rather than `.<num>`
- Autoderef values if followed by a field or an index
- Output `[..]` when borrowing inside a slice
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2017

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Oct 11, 2017

@bors retry

  • Android SDK HTTPS issue (fixed in #45193)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 12, 2017

⌛️ Testing commit e32e81c with merge d274114...

bors added a commit that referenced this pull request Oct 12, 2017

Auto merge of #44985 - zilbuz:issue-44974, r=pnkfelix
MIR borrowck: print lvalues in error messages in the same way that the AST borrowck

Fix #44974

- Print fields with `.name` rather than `.<num>`
- Autoderef values if followed by a field or an index
- Output `[..]` when borrowing inside a slice
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing d274114 to master...

@bors bors merged commit e32e81c into rust-lang:master Oct 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@zilbuz zilbuz deleted the zilbuz:issue-44974 branch Mar 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.