Skip to content

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)

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.

@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!

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

@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

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.

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.

Ezrashaw and others added 2 commits January 2, 2023 16:14
Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com>
@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
Collaborator

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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2023
…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
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Jan 8, 2023
…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
…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
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
Development

Successfully merging this pull request may close these issues.

7 participants