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.

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

[beta] backport fix for #40951 #41269

Closed
wants to merge 3 commits into from
Closed

[beta] backport fix for #40951 #41269

wants to merge 3 commits into from

Conversation

nikomatsakis
Copy link
Contributor

cc @arielb1 -- this is a backport of just the fix needed for #40951. Thoughts?

@nikomatsakis
backport solution for rust-lang#40951
b0345d1
@nikomatsakis
fix tests
b6da61f
@nikomatsakis
fix a bug in compiletest JSON parsing for duplicate errors
d45f62f 
In some cases, we give multiple primary spans, in which case we would
report one `//~` annotation per primary span. That was very confusing
because these things are reported to the user as a single error.

UI tests would be better here.
@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@nikomatsakis nikomatsakis changed the title backport fix for #40951 [beta] backport fix for #40951 Apr 13, 2017
@nikomatsakis
Copy link
Contributor Author

cc @brson @alexcrichton

@nikomatsakis nikomatsakis added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2017
@arielb1
Copy link
Contributor

arielb1 commented Apr 13, 2017

This feels a little risky to me - who knows if this can trigger another (performance or functionality) edge case. How bad is #40951? It looks like most of the #40319 regressions are either "bad annotation" or "NLL".

@alexcrichton
Copy link
Member

r=me if the compiler team is good with this

@nikomatsakis
Copy link
Contributor Author

@arielb1

This feels a little risky to me - who knows if this can trigger another (performance or functionality) edge case.

I also have mixed feelings.

@nikomatsakis nikomatsakis mentioned this pull request Apr 13, 2017
Closed
@nikomatsakis
Copy link
Contributor Author

We discussed this in the @rust-lang/compiler meeting and decided not to backport. This change is a bit more complex than we are comfortable with, and the fallout seems quite minimal. The regression is fixed on nightly.

@nikomatsakis nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arielb1 arielb1

Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nikomatsakis @rust-highfive @arielb1 @alexcrichton