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

Clean emitted diagnostics when reset_err_count is called. #47231

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Clean emitted diagnostics when reset_err_count is called. #47231

merged 1 commit into from
Jan 9, 2018

Conversation

ereslibre
Copy link
Contributor

When external tools like rustfmt calls to reset_err_count for handler
reusing, it will set the error count on the handler to 0, but since
#47146 the handler will contain
status that will prevent the error count to be bumped if this handler is
reused.

This caused rustfmt idempotency tests to fail:
rust-lang/rustfmt#2338

Fixes: rust-lang/rustfmt#2338

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@ereslibre
Copy link
Contributor Author

I'm not sure if reset_err_count should be renamed to something more meaningful, but it may break external tools using this method and I'm not sure about the rust policy here.

@nikomatsakis
Copy link
Contributor

It would be nice if you could add some kind of rustdoc comment explaining the purpose of that function -- right now the only comment tells you not to call it, but doesn't explain when or why a tool would call it...

When external tools like `rustfmt` calls to `reset_err_count` for handler
reusing, it will set the error count on the handler to 0, but since
#47146 the handler will contain
status that will prevent the error count to be bumped if this handler is
reused.

This caused `rustfmt` idempotency tests to fail:
rust-lang/rustfmt#2338

Fixes: rust-lang/rustfmt#2338
@ereslibre
Copy link
Contributor Author

@nikomatsakis done, let me know if needs more polishing :)

@nrc
Copy link
Member

nrc commented Jan 9, 2018

@bors: r+ p=1 (fixes rustfmt)

Thanks @ereslibre ! (I think keeping the name reset_err_count is fine).

@bors
Copy link
Collaborator

bors commented Jan 9, 2018

📌 Commit b48d944 has been approved by nrc

@bors
Copy link
Collaborator

bors commented Jan 9, 2018

⌛ Testing commit b48d944 with merge 2e33c89...

bors added a commit that referenced this pull request Jan 9, 2018
Clean emitted diagnostics when `reset_err_count` is called.

When external tools like `rustfmt` calls to `reset_err_count` for handler
reusing, it will set the error count on the handler to 0, but since
#47146 the handler will contain
status that will prevent the error count to be bumped if this handler is
reused.

This caused `rustfmt` idempotency tests to fail:
rust-lang/rustfmt#2338

Fixes: rust-lang/rustfmt#2338
@bors
Copy link
Collaborator

bors commented Jan 9, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 2e33c89 to master...

@bors bors merged commit b48d944 into rust-lang:master Jan 9, 2018
kennytm-githubbot added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 9, 2018
Tested on commit rust-lang/rust@2e33c89.

🎉 rustfmt on windows: test-fail → test-pass.
🎉 rustfmt on linux: test-fail → test-pass.
@ereslibre ereslibre deleted the clean-emitted-diagnostics branch January 30, 2018 15:24
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.

5 participants