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

Ref suggestion #37658

Merged
merged 4 commits into from
Apr 22, 2017
Merged

Ref suggestion #37658

merged 4 commits into from
Apr 22, 2017

Conversation

GuillaumeGomez
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@GuillaumeGomez
Copy link
Member Author

r? @nikomatsakis
cc @eddyb

Sub part of #37388.

@Mark-Simulacrum
Copy link
Member

Will this suggest &mut foo where foo is not mutable? If yes, perhaps we should phrase the suggestion differently in that case.

@eddyb
Copy link
Member

eddyb commented Nov 9, 2016

@Mark-Simulacrum To me getting the suggestion then, after making that change, an error about the variable not being mutable (with a suggestion to add mut to the let in that error) seems excellent.

@@ -14,6 +14,5 @@ fn main() {
let _: &[i32] = [0];
//~^ ERROR mismatched types
//~| expected type `&[i32]`
//~| found type `[{integer}; 1]`
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this a regression in error reporting?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have absolutely no clue from where this change comes since my code just added suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably a stray side-effect? Try wrapping in probe if you haven't already. In fact, you probably want a version of try_coerce with no side-effects (call it probe_coerce).

let suggestions = self.check_ref(expr, checked_ty, expected);
let mut err = self.report_mismatched_types(origin, expected, expr_ty, e);
if let Some(suggestions) = suggestions {
err.help(&suggestions);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not report a suggestion instead of a help?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed!

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, such a thing doesn't exist. (Or at least I didn't find it...).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I was looking for .suggest... You bet I didn't find it haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a span_suggestion method on DiagnosticBuilder:

    /// Prints out a message with a suggested edit of the code.
    ///
    /// See `diagnostic::RenderSpan::Suggestion` for more information.
    pub fn span_suggestion<S: Into<MultiSpan>>(&mut self,
                                               sp: S,
                                               msg: &str,
                                               suggestion: String)
                                               -> &mut Self {

I think the idea is that you replace the given span with the text suggestion, and also display the help message msg. Something like:

err.span_suggestion(span, "consider using the `&` operator", format!("&{}", span_to_string(span))

@GuillaumeGomez
Copy link
Member Author

@nikomatsakis: So, with the current code, it panics it certain cases. Here is the stack trace: https://gist.github.com/GuillaumeGomez/097fec3b6ac4257419fdf100115535f8

@eddyb: proposed a few things:

so instead of this https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/coercion.rs#L372

or rather, in addition to

you'd have to do something like https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/coercion.rs#L493 and rename unsizing_obligations to obligations

However he said to consult you first. Any opinion/direction to give me? :-/

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.

@GuillaumeGomez sorry for the delayed review, thanks for breaking this out into a separate PR! I'd say it's close, but for a few concerns:

  1. Can you change to use span_suggestion?
  2. Also, can you create ui tests instead of compile-fail tests for those cases where you are showing off what your error message does?
  3. I'd like to get to the bottom of the unexpected changes as well; I think that coercion may be registering trait obligations or something like that? (But this is supposed to result in assertion failures, I thought.)

Do you want me to dig into that last point, @GuillaumeGomez?

let suggestions = self.check_ref(expr, checked_ty, expected);
let mut err = self.report_mismatched_types(origin, expected, expr_ty, e);
if let Some(suggestions) = suggestions {
err.help(&suggestions);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a span_suggestion method on DiagnosticBuilder:

    /// Prints out a message with a suggested edit of the code.
    ///
    /// See `diagnostic::RenderSpan::Suggestion` for more information.
    pub fn span_suggestion<S: Into<MultiSpan>>(&mut self,
                                               sp: S,
                                               msg: &str,
                                               suggestion: String)
                                               -> &mut Self {

I think the idea is that you replace the given span with the text suggestion, and also display the help message msg. Something like:

err.span_suggestion(span, "consider using the `&` operator", format!("&{}", span_to_string(span))

@nikomatsakis
Copy link
Contributor

Oh, @GuillaumeGomez, just saw your comment, let me process it.

@nikomatsakis
Copy link
Contributor

Yes, ok, those panics are sort of what I expected. =) I'm not sure what is the most elegant fix here. We've been dancing around this problem for a bit.

In this particular case, I wonder if we should just add a way to clone the fulfillment context in its entirety, and then just restore it. After all, this is the error path, so performance isn't especially important. This would be like a poor man's persistent data structure.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 10, 2016

Well, performance is important even in case of failure when there are a lot of them (just imagine a code with more than 100 errors in it, it'd be a shame if it took a few seconds to run).

Also, considering my very poor background on the rust's internals, I'll need more explanation.

@eddyb
Copy link
Member

eddyb commented Nov 10, 2016

@nikomatsakis But most of the laziness is already there, shouldn't we do a bit a plumbing instead?
Coercions already have to deal with obligations from unsizing.
Or am I misunderstanding how far lazy normalization goes?

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016
@nikomatsakis
Copy link
Contributor

@GuillaumeGomez

sorry, didn't get a chance to try out my sample patch today, will do monday

@eddyb

But most of the laziness is already there, shouldn't we do a bit a plumbing instead?

I'm not quite sure what you are thinking here...

@eddyb
Copy link
Member

eddyb commented Nov 12, 2016

I mean that autoderef already has an obligations list registered late and so does coercion (but only for unsizing). By combining the two we might not even need the ability to register obligations inside a snapshot!

@nikomatsakis
Copy link
Contributor

@eddyb perhaps; I'll have to look into the code in a bit more detail to see what you mean.


// Test that we cannot convert from *-ptr to &S and &T (mut version)
let x: *mut S = &mut S;
let y: &S = x; //~ ERROR mismatched types
let y: &T = x; //~ ERROR mismatched types
//~^ ERROR the trait bound `*mut S: T` is not satisfied
Copy link
Contributor

@brson brson Nov 16, 2016

Choose a reason for hiding this comment

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

These new cases also don't seem to be reflected by the content of the patch, which doesn't add these errors.

Copy link
Member

Choose a reason for hiding this comment

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

They're fallout from bounds that leak out of a successful coercion attempt.
I've explained in my previous comments a path towards avoiding this side-effect.

@brson
Copy link
Contributor

brson commented Nov 16, 2016

It's not obvious what the purpose of this patch is. When adding features, consider explaining in the PR what the feature is and why it is important.

@GuillaumeGomez
Copy link
Member Author

@brson: I'll, but first I need to remove side-effects and need @nikomatsakis' help. :)

@bors
Copy link
Contributor

bors commented Nov 17, 2016

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

@nikomatsakis
Copy link
Contributor

OK, so, I just pushed a (hacky) commit that removes the side-effects. I'm not sure yet if I'm comfortable landing this or not =) @eddyb, did suggest he thought a cleaner path might be easy to get, and naturally I'd prefer that -- but this seems...not too terrible, for a focused hack.

Though it raises the question of how much value these suggestions have in the first place.

@eddyb
Copy link
Member

eddyb commented Nov 22, 2016

@nikomatsakis r- on the hack, it does too much for me to be comfortable with it.

@nikomatsakis
Copy link
Contributor

ok, yeah, so on IRC @eddyb elaborated a touch more on what he was thinking (propagating obligations up in a nicer way). Maybe one of us can find time to tinker with it soon.

@GuillaumeGomez
Copy link
Member Author

Let me know if I can help in any way.

bors added a commit that referenced this pull request Apr 19, 2017
Ban registering obligations during InferCtxt snapshots.

Back in #33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse.

But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from #37658, which *have to* be rolled back *even* if they succeed.

We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse.
Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions.

As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned.

r? @nikomatsakis @arielb1
@GuillaumeGomez
Copy link
Member Author

Ok, now I need to fix ui tests. But otherwise seems all good.

use rustc::infer::type_variable::TypeVariableOrigin;
use rustc::traits::{self, ObligationCause, ObligationCauseCode};
use rustc::infer::type_variable::{TypeVariableOrigin};
use rustc::traits::{self, /*FulfillmentContext,*/ ObligationCause, ObligationCauseCode};
Copy link
Member

Choose a reason for hiding this comment

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

Comment still here.

/*let saved_fulfillment_cx =
mem::replace(
&mut *self.inh.fulfillment_cx.borrow_mut(),
FulfillmentContext::new());*/
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Also not a hack right now.

@GuillaumeGomez
Copy link
Member Author

Tests passed! \o/

cc @eddyb @nikomatsakis

@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@GuillaumeGomez You have comments to remove and a rebase fixup commit to squash.

@GuillaumeGomez
Copy link
Member Author

Updated.

@@ -65,7 +65,7 @@ use check::{Diverges, FnCtxt};
use rustc::hir;
use rustc::hir::def_id::DefId;
use rustc::infer::{Coercion, InferResult, InferOk, TypeTrace};
use rustc::infer::type_variable::TypeVariableOrigin;
use rustc::infer::type_variable::{TypeVariableOrigin};
Copy link
Member

Choose a reason for hiding this comment

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

Heh, leftover edit.

pub fn can_coerce(&self,
expr_ty: Ty<'tcx>,
target: Ty<'tcx>)
-> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this signature fit on one line?

let coerce = Coerce::new(self, cause);
let result = self.probe(|_| coerce.coerce::<hir::Expr>(&[], source, target)).is_ok();

result
Copy link
Member

Choose a reason for hiding this comment

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

Can just have self.probe(...) as the returned expression.

@GuillaumeGomez
Copy link
Member Author

@bors: r=nikomatsakis,eddyb

@bors
Copy link
Contributor

bors commented Apr 21, 2017

📌 Commit 7ce1eb7 has been approved by nikomatsakis,eddyb

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 22, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 22, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 22, 2017
bors added a commit that referenced this pull request Apr 22, 2017
Rollup of 4 pull requests

- Successful merges: #37658, #41405, #41432, #41435
- Failed merges:
@bors bors merged commit 7ce1eb7 into rust-lang:master Apr 22, 2017
@GuillaumeGomez GuillaumeGomez deleted the ref_suggestion branch April 22, 2017 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.