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

Add documentation for error E0208 #98011

Merged
merged 1 commit into from Jun 13, 2022
Merged

Add documentation for error E0208 #98011

merged 1 commit into from Jun 13, 2022

Conversation

onlineSoftwareDevOK
Copy link
Contributor

Related to #61137

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2022
GuillaumeGomez
GuillaumeGomez reviewed Jun 11, 2022
View reviewed changes
compiler/rustc_error_codes/src/error_codes/E0208.md Outdated
fn main() {}
```

Below find a table mapping the symbol shown to the variance:
Copy link
Member

Choose a reason for hiding this comment

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

There is really nothing more to be explained about this error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. What else should be explained about this error code?

Copy link
Member

Choose a reason for hiding this comment

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

Try to adopt this point of view: you don't know what this error code is about at all. What information does the reader need to understand why they got this error?

In the current case, you only show a table of variance, but that's far from enough to understand what's going on in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. A link to the following page would be enough? https://doc.rust-lang.org/nomicon/subtyping.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just realized it's already there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt the link to the documentation was enough @mbartlett21 What else can I add here?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@onlineSoftwareDevOK
Copy link
Contributor Author

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@GuillaumeGomez Can you explain why this failed, please? A similar sentence ("For more information...") was added in the following PR but it worked there #94449

compiler-errors
compiler-errors requested changes Jun 11, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I don't think that an internal error needs error documentation.

The feature gate specifically says "the #[rustc_error] attribute is just used for rustc unit tests and will never be stable", and I don't think people working within the compiler need this documentation.

@onlineSoftwareDevOK
Copy link
Contributor Author

I don't think that an internal error needs error documentation.

The feature gate specifically says "the #[rustc_error] attribute is just used for rustc unit tests and will never be stable", and I don't think people working within the compiler need this documentation.

I prepared this PR because this error code is mentioned here #61137 (comment)

@compiler-errors
Copy link
Member

compiler-errors commented Jun 12, 2022 •
edited

@GuillaumeGomez, perhaps that one should also be marked off the list as unneeded? Edit: Same for E0640, E0711, E0717

@GuillaumeGomez
Copy link
Member

I made the list based on the undocumented error codes but didn't make difference with internal ones. We can just add a comment like // internal error code behind it and not add a long error explanation. Same for the errors listed by @compiler-errors.

@onlineSoftwareDevOK
Copy link
Contributor Author

I made the list based on the undocumented error codes but didn't make difference with internal ones. We can just add a comment like // internal error code behind it and not add a long error explanation. Same for the errors listed by @compiler-errors.

Inside compiler/rustc_error_codes/src/error_codes.rs, right?

@GuillaumeGomez
Copy link
Member

Yes. Something like this:

E0208, // internal error code

@onlineSoftwareDevOK
Copy link
Contributor Author

Yes. Something like this:

E0208, // internal error code

Done

@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 12, 2022

📌 Commit 99672fb has been approved by GuillaumeGomez

@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 Jun 12, 2022
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jun 12, 2022
Closed
100 tasks
@compiler-errors compiler-errors mentioned this pull request Jun 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2022
@bors
Auto merge of rust-lang#98037 - compiler-errors:rollup-fbvy456, r=com…
c570ab5 
…piler-errors

Rollup of 3 pull requests

Successful merges:

 - rust-lang#97920 (Fix some test annotations)
 - rust-lang#97950 (Clarify `#[derive(PartialEq)]` on enums)
 - rust-lang#98011 (Add documentation for error E0208)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ac73b19 into rust-lang:master Jun 13, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

@compiler-errors compiler-errors Awaiting requested review from compiler-errors

Assignees

@lcnr lcnr

Labels
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.63.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@onlineSoftwareDevOK @rust-highfive @rust-log-analyzer @compiler-errors @GuillaumeGomez @bors @lcnr @rustbot