Skip to content

Fix End-user description not implemented for field access on `TyClosure #45938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 15, 2017
Merged

Fix End-user description not implemented for field access on `TyClosure #45938

merged 2 commits into from
Nov 15, 2017

Conversation

vramana
Copy link
Contributor

@vramana vramana commented Nov 11, 2017 •
edited
Loading

  • Add Tests

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 12, 2017
@nikomatsakis
Copy link
Contributor

I think we are not quite ready for a PR here. I think we will want to edit the caller of describe_field_from_ty -- the end goal is to render the whole lvalue (*_0.0) as just the name of the local variable.

@nikomatsakis
Copy link
Contributor

What is the issue number again?

@nikomatsakis
Copy link
Contributor

ah #45698

arielb1
arielb1 reviewed Nov 12, 2017
View reviewed changes
src/librustc_mir/borrow_check.rs Outdated
@@ -1235,6 +1237,18 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
("", format!(""), None), // (dont emit downcast info)
ProjectionElem::Field(field, _ty) => {
autoderef = true;
match proj.base {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use self.mir.lvalue_ty(proj.base).to_ty(tcx)

arielb1
arielb1 reviewed Nov 12, 2017
View reviewed changes
src/librustc_mir/borrow_check.rs Outdated
}
},
_ => ()
};
("", format!(".{}", self.describe_field(&proj.base, field.index())), None)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like it would print .local. I think it's probably a better idea to ditch the prefix/middle/suffix scheme and just push things from the match arms.

@vramana vramana force-pushed the fix-ice-45698 branch 2 times, most recently from badbd1e to c9533cb Compare November 12, 2017 17:36
@alexcrichton
Copy link
Member

r? @nikomatsakis

@arielb1
Copy link
Contributor

arielb1 commented Nov 13, 2017

So r=me with one of the tests (e.g. src/compile-fail/borrowck/borrowck-multiple-captures.rs) adjusted to also test MIR borrowck (see e.g. src/test/compile-fail/borrowck/borrowck-use-uninitialized-in-cast-trait.rs to how this is done).

@vramana
Copy link
Contributor Author

vramana commented Nov 13, 2017

@arielb1 Sure

@bors
Copy link
Collaborator

bors commented Nov 14, 2017

☔ The latest upstream changes (presumably #45436) made this pull request unmergeable. Please resolve the merge conflicts.

@vramana vramana force-pushed the fix-ice-45698 branch 2 times, most recently from 3b92d6b to e2825ac Compare November 14, 2017 19:30
@vramana vramana force-pushed the fix-ice-45698 branch 3 times, most recently from c831fbb to a0cd79f Compare November 14, 2017 19:46
@vramana vramana changed the title [WIP] Fix End-user description not implemented for field access on `TyClosure Fix End-user description not implemented for field access on `TyClosure Nov 14, 2017
nikomatsakis
nikomatsakis requested changes Nov 14, 2017
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Can you add a nested test and convert to use the helper I suggested?

src/librustc_mir/borrow_check.rs Outdated
let local_def = self.tcx.with_freevars(node_id, |fv| fv[field_index].def);

match local_def {
Def::Local(local_node_id) => self.tcx.hir.name(local_node_id).to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that the code in ppaux does self.tcx.hir.name(local_def.var_id()) -- maybe we should do that. I think in particular this could be a Def::Upvar for nested closures. Maybe try this example:

// Field from upvar
{
    let mut x = 0;
       || { || {
            let y = &mut x;
            &mut x; //[ast]~ ERROR cannot borrow `**x` as mutable more than once at a time
                    //[mir]~^ ERROR cannot borrow `**x` as mutable more than once at a time (Ast)
                    //[mir]~| ERROR cannot borrow `(**x)` as mutable more than once at a time (Mir)
            *y = 1;
        } };
}

@nikomatsakis
Copy link
Contributor

I just realized that the Mir has a vector of upvar_decls:

/// A closure capture, with its name and mode.
#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
pub struct UpvarDecl {
    pub debug_name: Name,

    /// If true, the capture is behind a reference.
    pub by_ref: bool
}

This might be a nicer thing to use than freevars for getting the names of upvars. Moreover, it would tell us the upvar is by reference, which should allow us to eliminate those extra *.

@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2017

@nikomatsakis

I'll r+ this PR anyway - we need a followup to fix the deref situation, but it's "pre-existing" #46003

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 15, 2017

📌 Commit 5144d32 has been approved by arielb1

@kennytm kennytm 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 Nov 15, 2017
@nikomatsakis nikomatsakis mentioned this pull request Nov 15, 2017
Closed
@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2017

@bors p=1

I'll like to land this today so I can have some sense of scale of the MIR borrowck situation

@bors
Copy link
Collaborator

bors commented Nov 15, 2017

⌛ Testing commit 5144d32 with merge fa26421...

bors added a commit that referenced this pull request Nov 15, 2017
@bors
Auto merge of #45938 - vramana:fix-ice-45698, r=arielb1
fa26421 
Fix End-user description not implemented for field access on `TyClosure

- [x] Add Tests
@bors
Copy link
Collaborator

bors commented Nov 15, 2017

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

@bors bors merged commit 5144d32 into rust-lang:master Nov 15, 2017
@vramana vramana deleted the fix-ice-45698 branch December 26, 2017 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis requested changes

+1 more reviewer

@arielb1 arielb1 arielb1 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vramana @nikomatsakis @alexcrichton @arielb1 @bors @kennytm