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

Update E0220 message to new format #35412

Closed
wants to merge 1 commit into from
Closed

Update E0220 message to new format #35412

wants to merge 1 commit into from

Conversation

chamoysvoice
Copy link
Contributor

Part of #35233 .
Fixes #35385.

r? @jonathandturner

Should it keep E0191?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sophiajt
Copy link
Contributor

sophiajt commented Aug 6, 2016

Looks good. Before we take the PR, please squash the 2 commits down to 1 commit.

@chamoysvoice chamoysvoice reopened this Aug 7, 2016
@sophiajt
Copy link
Contributor

sophiajt commented Aug 7, 2016

Great!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 7, 2016

📌 Commit 0e13b63 has been approved by jonathandturner

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 7, 2016
Update E0220 message to new format

Part of rust-lang#35233 .
Fixes rust-lang#35385.

r? @jonathandturner

Should it keep E0191?
@TimNN
Copy link
Contributor

TimNN commented Aug 7, 2016

From a local build of the rollup #35462 which included this PR, I believe that this PR causes the compile-fail tests E0220.rs and unboxed-closure-sugar-wrong-trait.rs to fail due to missing notes, see #35462 (comment) for the full test output.

Also, @jonathandturner from looking at the diff, the span_label does not seem to include any additional information (same span, only slightly reworded message), does it even make sense to have a span_label in that case?

@sophiajt
Copy link
Contributor

sophiajt commented Aug 7, 2016

@bors r-

Looks like there might be some additional unit tests that need to be updated with this PR. @TimNN mentions them above.

@TimNN - labels are good for readability, even if they just reiterate the error title. In our tests, people tend to see the label first.

@TimNN
Copy link
Contributor

TimNN commented Aug 7, 2016

@jonathandturner Thanks for the info, I just noticed and wanted to ask about it :)

However shouldn't the span_label better be associated **type** {} [...]? The current message like associated F not doesn't read very well to me.

@sophiajt
Copy link
Contributor

sophiajt commented Aug 7, 2016

@TimNN - it reads better, but it's also longer. Perhaps a phrasing like "associated type {} not found" without the "in {}", since it's already on the title, might work better.

@TimNN
Copy link
Contributor

TimNN commented Aug 7, 2016

@jonathandturner I (personally) like that suggestion very much!

@chamoysvoice
Copy link
Contributor Author

@jonathandturner so this PR needs to be built after E0191 right? or is it another problem?

Can I help?

Also I'm taking you suggestion of removing "in {}" into the E0220 phrase

@sophiajt
Copy link
Contributor

@chamoysvoice - sorry, I missed this question. And yes, please take the suggestion to shorten the label

@chamoysvoice
Copy link
Contributor Author

OOOPS! missing parenthesis, for some reason my local branch has that parenthesis, somehow I didn't merge it, no worries I'll fix it.

@sophiajt
Copy link
Contributor

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 11, 2016

📌 Commit 1c37892 has been approved by jonathandturner

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 13, 2016
Update E0220 message to new format

Part of rust-lang#35233 .
Fixes rust-lang#35385.

r? @jonathandturner

Should it keep E0191?
eddyb added a commit to eddyb/rust that referenced this pull request Aug 14, 2016
Update E0220 message to new format

Part of rust-lang#35233 .
Fixes rust-lang#35385.

r? @jonathandturner

Should it keep E0191?
@eddyb
Copy link
Member

eddyb commented Aug 14, 2016

@bors r- failed on travis

@sophiajt
Copy link
Contributor

@chamoysvoice - You'll need to fix some of the test failures before this PR can be accepted.

error: /build/src/test/compile-fail/E0220.rs:15: unexpected "error": '15:12: 15:24: the value of the associated type `Bar` (from the trait `Trait`) must be specified [E0191]'
error: /build/src/test/compile-fail/E0220.rs:15: unexpected "note": 'missing associated type `Bar` value'
error: /build/src/test/compile-fail/E0220.rs:16: expected error not found: E0191

error: 2 unexpected errors found, 1 expected errors not found

error: /build/src/test/compile-fail/unboxed-closure-sugar-wrong-trait.rs:15: unexpected "note": 'associated type `Output` not found'

error: 1 unexpected errors found, 0 expected errors not found

failures:
    [compile-fail] compile-fail/E0220.rs
    [compile-fail] compile-fail/unboxed-closure-sugar-wrong-trait.rs

test result: FAILED. 2443 passed; 2 failed; 12 ignored; 0 measured

/build/mk/tests.mk:769: recipe for target 'tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-cfail.ok' failed

make: *** [tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-cfail.ok] Error 101

You can run the compile-fail test suite to make sure you've fixed everything with:

python src/bootstrap/bootstrap.py --step check-cfail --stage 1

@chamoysvoice
Copy link
Contributor Author

@jonathandturner This build is failing because E0191 is throwing an error right? E0191 should't be updated first?

I can't find my mistake

@sophiajt
Copy link
Contributor

sophiajt commented Aug 24, 2016

@chamoysvoice - have you tried updating with a git pull --rebase, building the compiler and trying one of those two errors by hand?

It's possible that another PR changed things since then, so you might have to update these tests to work with your fixes.

@chamoysvoice
Copy link
Contributor Author

@jonathandturner E0191 #35396 is still failing the build, but is already merged to master, should I pull from master then?

@eddyb
Copy link
Member

eddyb commented Aug 24, 2016

@jonathandturner NB: One needs to use git pull --rebase on a PR branch to avoid merge commits.

@sophiajt
Copy link
Contributor

@eddyb - updated

@chamoysvoice
Copy link
Contributor Author

@jonathandturner So, this need to be rebuilt and thats all right?

@sophiajt
Copy link
Contributor

@chamoysvoice - yes, your rebase should be on top of master. That should let you pull in the new fixes that are causing the issue travis-ci is seeing.

@bors
Copy link
Contributor

bors commented Oct 2, 2016

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

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.

7 participants