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

refactor: clean up errors.rs and error_codes_check.rs #106341

Merged
merged 8 commits into from
Jan 7, 2023
Merged

refactor: clean up errors.rs and error_codes_check.rs #106341

merged 8 commits into from
Jan 7, 2023

Conversation

Ezrashaw
Copy link
Contributor

@Ezrashaw Ezrashaw commented Jan 1, 2023

errors.rs is basically unused now, error_codes_check.rs is useful but not well commented, etc. It also doesn't check certain things which are certainly not correct. For example, E0505 has a UI test in src/test/ui/error-codes/ but that test actually outputs E0504?! Other issues like these exist. I've implemented these with "warnings" which are a bit rough around the edges but should be removed eventually.

r? @GuillaumeGomez (again not sure if you want to review but its relevant to you)

@Ezrashaw
refactor: clean up errors.rs and error_codes_check.rs
04b9038 
Move them into new `error_codes.rs` tidy check.
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2023

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@Ezrashaw
Copy link
Contributor Author

Ezrashaw commented Jan 1, 2023

The CI is failing because of a check ensuring error code explanations are not removed. I'm not a big fan of this, what help can outdated documentation be? I want guidance on this and then I'll remove that CI check.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

This is a really nice improvements, thanks a lot for working on this. Once my comment is fixed, it'll be good to go!

mejrs
mejrs reviewed Jan 1, 2023
View reviewed changes
Copy link
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

I think the error messages are generally good 👍 I have some comments though

src/tools/tidy/src/error_codes.rs Show resolved Hide resolved
src/tools/tidy/src/error_codes.rs Show resolved Hide resolved
src/tools/tidy/src/error_codes.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/error_codes.rs Outdated Show resolved Hide resolved
@mejrs
Copy link
Contributor

mejrs commented Jan 1, 2023

I checked it out locally and tidy spits out the following:

WARNING: 10 error codes are undocumented. This *will* become a hard error.
Found 505 error codes
WARNING: 36 error codes use the ignore header. This should not be used, add the error codes to the `IGNORE_DOCTEST_CHECK` constant instead. This *will* become a hard error.
WARNING: 36 error codes don't have a code example, all error codes are expected to have one (even if untested). This *will* become a hard error.
WARNING: 68 error codes are no longer emitted and should be removed entirely. This *will* become a hard error.
WARNING: 8 error codes have a UI test file, but don't contain their own error code!
WARNING: 272 error codes need to have at least one UI test in the `src/test/ui/error-codes/` directory`! This *will* become a hard error.
WARNING: 6 error codes are used when they are marked as "no longer emitted"
* 392 features
fmt check
Build completed successfully in 0:01:09

We shouldn't emit warnings like that unless it's the user's fault. There should be no "WARNING" in all caps and no "This will become a hard error."

I think it would be fine to have something like this as an opt-in (that would definitely be helpful for your followup PRs and anyone helping), but not by default.

@Ezrashaw
Copy link
Contributor Author

Ezrashaw commented Jan 1, 2023 •
edited
Loading

I think it would be fine to have something like this as an opt-in (that would definitely be helpful for your followup PRs and anyone helping), but not by default.

Could that be hardcoded? Nevermind, we can use --verbose. I'll also fix up all poor counter logic and stuff, we can just print 100s of messages if its opt-in.

@mejrs
Copy link
Contributor

mejrs commented Jan 1, 2023

Nevermind, we can use --verbose

Or you could make a --pedantic setting. It's generally understood that if you enable something called pedantic that the errors may be terse.

@Ezrashaw
Copy link
Contributor Author

Ezrashaw commented Jan 1, 2023

tidy already has a bool for verbose in main.rs so I'll just use that. Pedantic implies failing the build but this is just supplementary information.

mejrs
mejrs approved these changes Jan 1, 2023
View reviewed changes
Copy link
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

It looks good, thanks for making the changes, I just have one final nit.

src/tools/tidy/src/error_codes.rs Outdated Show resolved Hide resolved
Ezrashaw and others added 2 commits January 2, 2023 16:14
@Ezrashaw @mejrs
pattern destructure has_test
1f1dd5f 
Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com>
@Ezrashaw
fix CI
b7341db
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks for working on this! Do you see anything else @mejrs and @klensy ?

@mejrs
Copy link
Contributor

mejrs commented Jan 6, 2023

Looks good to me, thanks for working on this! Do you see anything else @mejrs and @klensy ?

I'm happy with it :)

@GuillaumeGomez
Copy link
Member

Then let's go!

@bors r=mejrs,klensy,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jan 6, 2023

📌 Commit c00ab4b has been approved by mejrs,klensy,GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 6, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2023
@bors
Auto merge of rust-lang#106544 - matthiaskrgr:rollup-e9prjed, r=matth…
84f22e4 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#106287 (Add some docs to `bug`, `span_bug` and `delay_span_bug`)
 - rust-lang#106341 (refactor: clean up `errors.rs` and `error_codes_check.rs`)
 - rust-lang#106453 (Improve include macro documentation)
 - rust-lang#106466 (Fix rustdoc source code rendering for `#[path = "../path/to/mod.rs"]` links)
 - rust-lang#106528 (Tiny formatting fix)
 - rust-lang#106534 (rustdoc-gui: Use new block syntax for define-function in goml scripts)
 - rust-lang#106542 (Add default and latest stable edition to --edition in rustc (attempt 2))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 498216e into rust-lang:master Jan 7, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 7, 2023
@Ezrashaw Ezrashaw deleted the refactor-error-code-tidy-check branch January 7, 2023 04:10
@Ezrashaw Ezrashaw mentioned this pull request Jan 7, 2023
Merged
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Jan 8, 2023
Rollup merge of rust-lang#106557 - Ezrashaw:ui-test-fixups-1, r=Guill…
fddd27b 
…aumeGomez

Add some UI tests and reword error-code docs

Added UI tests for `E0013` and `E0015`. Error code docs for `E0015` were a bit unclear (they referred to all non-const errors in const context, when only non-const functions applied), so I touched them up a bit.

I also fixed up some issues in the new `error_codes.rs` tidy check (linked rust-lang#106341), that I overlooked previously.

r? `@GuillaumeGomez`
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Jan 8, 2023
Rollup merge of rust-lang#106557 - Ezrashaw:ui-test-fixups-1, r=Guill…
3b5afa5 
…aumeGomez

Add some UI tests and reword error-code docs

Added UI tests for `E0013` and `E0015`. Error code docs for `E0015` were a bit unclear (they referred to all non-const errors in const context, when only non-const functions applied), so I touched them up a bit.

I also fixed up some issues in the new `error_codes.rs` tidy check (linked rust-lang#106341), that I overlooked previously.

r? ``@GuillaumeGomez``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klensy klensy klensy left review comments

@mejrs mejrs mejrs approved these changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Ezrashaw @rustbot @rust-log-analyzer @GuillaumeGomez @mejrs @bors @klensy