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

Try to fix compilation error on rustc 1.19.0-nightly 4ed2edaaf #1808

Merged
merged 4 commits into from Jun 4, 2017
Merged

Try to fix compilation error on rustc 1.19.0-nightly 4ed2edaaf #1808

merged 4 commits into from Jun 4, 2017

Conversation

messense
Copy link
Contributor

@messense messense commented Jun 2, 2017

Address #1807

self.tables
.adjustments
.get(&borrow_id)
.map(|a| &a.kind) {
if LoanCause::AutoRef == loan_cause {
// x.foo()
if autoderefs == 0 {
if overloaded == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Adjust::Deref related codes are not fixed yet. I am not sure how to fix it as I am a newbie to rustc internals.

Copy link
Member

Choose a reason for hiding this comment

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

This one checks for no deref adjustments.

@messense
Copy link
Contributor Author

messense commented Jun 2, 2017

I'll be out on vacation tomorrow so I won't have time working on this for the next couple of days. If anyone wants to pick it up, feel free to do so. :-)

@eddyb
Copy link
Member

eddyb commented Jun 3, 2017

FWIW it's a good idea to ask the person who made the Rust PR when you get stuck.

@@ -41,7 +41,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBorrow {
}
if let ExprAddrOf(MutImmutable, ref inner) = e.node {
if let ty::TyRef(..) = cx.tables.expr_ty(inner).sty {
if let Some(&ty::adjustment::Adjust::DerefRef { autoderefs, autoref, .. }) =
Copy link
Member

Choose a reason for hiding this comment

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

What you have now instead is a vector of adjustments, each autoderef is now one entry in that vector, and autoref is another entry. So this check looks for an autoref that is preceded by 2 or more derefs.

let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen.");
check_arguments(cx, arguments, method_type.ty, &name.node.as_str())
let method_type = borrowed_table.expr_ty(e);
check_arguments(cx, arguments, method_type, &name.node.as_str())
Copy link
Member

Choose a reason for hiding this comment

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

expr_ty(e) gives you the returned type not the method type (which you have to get from its DefId now).

self.tables
.adjustments
.get(&self.tcx
.hir
.get_parent_node(borrow_id))
.map(|a| &a.kind) {
if autoderefs <= 1 {
if overloaded <= 1 {
// foo(&x) where no extra autoreffing is happening
Copy link
Member

Choose a reason for hiding this comment

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

This check is weird, since it doesn't check for an autoref. Either way, it wants at most one deref adjustment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there were false posived when generics are involved. But I don't remember the details. Please add a FIXME(oli-obk) so I'll check it again in the future

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2017

I'm on vacation without a PC. Can someone else please take it?

@mcarton
Copy link
Member

mcarton commented Jun 4, 2017

I gave it a try but it's a nasty one, especially without compiler docs.

@eddyb
Copy link
Member

eddyb commented Jun 4, 2017

@mcarton Are you on IRC? I could mentor this, or maybe do it myself, but I'd have to be able to locally test clippy and my setup is funky enough that this is no fun.

@@ -81,7 +81,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
let fn_def_id = cx.tcx.hir.local_def_id(node_id);
let region_maps = &cx.tcx.region_maps(fn_def_id);
{
let mut vis = ExprUseVisitor::new(&mut v, region_maps, &infcx);
let def_id = cx.tcx.hir.body_owner_def_id(body.id());
let param_env = cx.tcx.param_env(def_id);
Copy link
Member

Choose a reason for hiding this comment

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

You can pass fn_def_id. Also, never use borrowck_fake_infer_ctxt. It's literally a hack for borrowck. Bad for everything else.

@eddyb
Copy link
Member

eddyb commented Jun 4, 2017

@mcarton I talked to @Manishearth and I'll try to take over, let me know when you've pushed what you've got so far.

@mcarton
Copy link
Member

mcarton commented Jun 4, 2017

@eddyb Thanks! I pushed where I've got so far, and I don't think I'll have much time to work on that today, so go ahead 😄

@@ -267,7 +259,6 @@ pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool {
}

/// Get the definition associated to a path.
/// TODO: investigate if there is something more efficient for that.
Copy link
Member

Choose a reason for hiding this comment

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

Why was this TODO removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc you told me on IRC that this impl is pretty sensible ;)

Copy link
Member

Choose a reason for hiding this comment

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

Oh was this you?

@eddyb
Copy link
Member

eddyb commented Jun 4, 2017

Please pull in eddyb@010974f.

@mcarton mcarton merged commit 5a6e48f into rust-lang:master Jun 4, 2017
@mcarton
Copy link
Member

mcarton commented Jun 4, 2017

Thanks to you @messense and @eddyb!

@messense messense deleted the feature/try-fix-nightly branch June 5, 2017 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants