Skip to content

infer: remove distinction for "simple type errors" - #33367

Closed
birkenfeld wants to merge 1 commit into
rust-lang:masterfrom
birkenfeld:issue-33366
Closed

infer: remove distinction for "simple type errors"#33367
birkenfeld wants to merge 1 commit into
rust-lang:masterfrom
birkenfeld:issue-33366

Conversation

@birkenfeld

Copy link
Copy Markdown
Contributor

Previously, the second "expected X, found Y" in parens was omitted for simple errors because it added no information.

Now that this is always omitted, there is no need for a distinction anymore. This change restores the "expected, found" message for such errors.

Fixes: #33366

Previously, the second "expected X, found Y" in parens was omitted
for simple errors because it added no information.
Now that this is always omitted, there is no need for a distinction
anymore. This change restores the "expected, found" message for
such errors.
Fixes: rust-lang#33366
@rust-highfive

Copy link
Copy Markdown
Contributor

r? @arielb1

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

}
err = err.note_expected_found(&"type", &expected, &found);

err = err.span_label(trace.origin.span(), &terr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm. I'm surprised we're not seeing the span_label actually, even in old mode output.

@nikomatsakis

Copy link
Copy Markdown
Contributor

@birkenfeld so actually I see the problem now. I think we give special treatment to labels attached to the primary span, which we ignore (for...reasonably good reasons). After some discussion with @jonathandturner I think the easiest fix is to make the check_old_skool helper in syntax::errors public, and then invoke it here sort of like so:

// this would replace line 561ifcheck_old_skool(){
err.note(&format("{}", terr));}else{
err.span_label(trace.origin.span(),&terr);}

Basically the problem here is that we don't have QUITE enough info to know whether to convert the "span label" into a note or not. We could augment the span_label call with some extra information, but given that we hope the "backwards compat" mode will be fairly temporary, it feels a bit like overkill. I guess it depends how often this scenario will arise.

@birkenfeld

Copy link
Copy Markdown
ContributorAuthor

Right, if you are confident that the backwards compatibility mode will be removed soon, I agree that this can be ignored.

While at it, are you aware that the new error format breaks default editor/IDE "go to error" mechanics (because the file/line is not at BOL, but after a -->)?

@nikomatsakis

Copy link
Copy Markdown
Contributor

@birkenfeld

While at it, are you aware that the new error format breaks default editor/IDE "go to error" mechanics (because the file/line is not at BOL, but after a -->)?

Indeed, this is the primary reason that the new format is not yet the default. I plan to submit a PR for emacs rust-mode in a bit, at least (I have a local fix, or at least a mostly fix). We have to work with other editors. (Which editor are you using?)

@nikomatsakis

Copy link
Copy Markdown
Contributor

@birkenfeld

Right, if you are confident that the backwards compatibility mode will be removed soon, I agree that this can be ignored.

I didn't meant to suggest we should ignore it, I want the "default output" to be good to be sure. I was just suggesting we might fix this a different way. Basically, in old mode, we can move the content of the "span_label" call into the main message (this isn't quite what I wrote before, which suggested we should add a new note, but now I think that's probably wrong, or at least not what we did before).

@birkenfeld

Copy link
Copy Markdown
ContributorAuthor

(Which editor are you using?)

I'm also using Emacs (and used to customizing it), so personally I'm fine, just wanted to make sure you're aware.

I was just suggesting we might fix this a different way.

Sure. Should I try to make this change here, or do you want to fix it while you work on the new errors anyway?

@sophiajt

sophiajt commented May 3, 2016

Copy link
Copy Markdown
Contributor

@birkenfeld yeah should be fair game if there are parts of the old-style errors that got worse with the recent PR. I was poking around a bit. If we started with this PR, but changed your:

if !is_simple_error {

to:

if !is_simple_error || check_old_skool(){

And then plumb through check_old_skool. I think that gets the old behavior you want to turn back on, while leaving it off in the new style errors where we use labels.

@nikomatsakis

Copy link
Copy Markdown
Contributor

closing in favor of #33367

@birkenfeld
birkenfeld deleted the issue-33366 branch May 13, 2016 13:30
Sign up for freeto 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.

5 participants

@birkenfeld@rust-highfive@nikomatsakis@sophiajt@arielb1