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

Add Three point error handling to borrowck #27

Merged
merged 4 commits into from
Dec 16, 2017

Conversation

spastorino
Copy link
Collaborator

This is for rust-lang#45988

Copy link
Owner

@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.

Let's fix the pub thing -- as for the UI tests, if you modify the code in region_infer/mod.rs and find the place where it enables tracing only if nll_dump_cause is true -- just make it enable tracing unconditionally, then we don't need to pass -Znll-dump-cause

@@ -2693,7 +2678,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
struct Context {
pub struct Context {
Copy link
Owner

Choose a reason for hiding this comment

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

this probably does not need to be pub...? in particular, submodules can still access it when it is priv

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

error[E0446]: private type `borrow_check::Context` in public interface
  --> src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs:21:5
   |
21 | /     pub fn explain_why_borrow_contains_point(
22 | |         &self,
23 | |         context: Context,
24 | |         borrow: &BorrowData<'_>,
...  |
78 | |         }
79 | |     }
   | |_____^ can't leak private type

error: aborting due to previous error

Copy link
Owner

Choose a reason for hiding this comment

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

change that pub fn explain to pub(super) fn explain

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

if let Some(cause) = regioncx.why_region_contains_point(borrow.region, context.loc) {
let mir = self.mir;

cause.label_diagnostic(mir, err);
Copy link
Owner

Choose a reason for hiding this comment

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

maybe we should move this call to only execute if cause.root_cause() returns something other than LiveVar or DropVar... that would avoid dump extra causal information when we can give a more user-friendly version. But really I guess we just want to make the "cause-gathering" stuff enabled or lazy, so never mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍, so should just leave as is ?

@spastorino
Copy link
Collaborator Author

Let's fix the pub thing -- as for the UI tests, if you modify the code in region_infer/mod.rs and find the
place where it enables tracing only if nll_dump_cause is true -- just make it enable tracing
unconditionally, then we don't need to pass -Znll-dump-cause

I don't understand what you mean here. Enabling this unconditionally won't in some way rollback your work? in that case isn't it better to remove the commit that adds is from code

@nikomatsakis
Copy link
Owner

I don't understand what you mean here. Enabling this unconditionally won't in some way rollback your work? in that case isn't it better to remove the commit that adds is from code

Sure. Just make a single commit and I'll rebase it later and fixup.

@spastorino spastorino force-pushed the three-point-error branch 2 times, most recently from 4ae56b3 to 5df1de2 Compare December 13, 2017 21:02
@nikomatsakis nikomatsakis force-pushed the nll-master branch 4 times, most recently from 3e45be8 to 4661540 Compare December 15, 2017 16:58
@nikomatsakis nikomatsakis merged commit 6bc4ea1 into nikomatsakis:nll-master Dec 16, 2017
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.

2 participants