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

Remove FIXME since there is nothing to be fixed #89907

Merged
merged 1 commit into from
Oct 17, 2021
Merged

Remove FIXME since there is nothing to be fixed #89907

merged 1 commit into from
Oct 17, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 15, 2021 •
edited by camelid
Loading

Resolves #88593.

The errors are deduplicated when displayed to users. They only appear
multiple times in UI tests.

cc @jyn514
r? @camelid

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-error-handling Area: Error handling labels Oct 15, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2021
jyn514
jyn514 approved these changes Oct 15, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Haven't looked at the test case, but the abort_if_errors() call looks right.

@camelid
Copy link
Member

camelid commented Oct 15, 2021

It has one downsize though: rustdoc errors are now run in a "different pass", which I find not great...

What does this mean? Do you mean that if there was a rustc error, no rustdoc errors will be reported until the rustc error is fixed?

@GuillaumeGomez
Copy link
Member Author

Exactly. For example, in the test I splitted in two parts, it's because the rustdoc lints weren't emitted because we aborted before them.

@Mark-Simulacrum
Copy link
Member

rustc itself generally deduplicates errors, that's just turned off for UI tests. Does this duplication reproduce outside of UI tests? If not, it may be preferable to avoid the early abort, which we're generally trying to move away from where possible.

camelid
camelid reviewed Oct 15, 2021
View reviewed changes
src/librustdoc/core.rs Outdated
@@ -343,6 +343,7 @@ crate fn run_global_ctxt(
let access_levels = AccessLevels {
map: tcx.privacy_access_levels(()).map.iter().map(|(k, v)| (k.to_def_id(), *v)).collect(),
};
tcx.sess.abort_if_errors();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tcx.sess.abort_if_errors();
// Avoid emitting duplicate errors later on.
tcx.sess.abort_if_errors();

src/test/rustdoc-ui/check-fail.stderr Outdated
Comment on lines 1 to 2
error: missing documentation for a function
--> $DIR/check-fail.rs:11:1
error: missing documentation for the crate
--> $DIR/check-fail.rs:3:1
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unrelated to this change, but why did it not emit missing_docs for the crate before?

Seems like the situation is this:

Before

  • function

After

  • crate
  • function

Copy link
Member Author

Choose a reason for hiding this comment

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

The rustc lint passes come afterwards (unless I missed something?).

@camelid
Copy link
Member

camelid commented Oct 15, 2021

rustc itself generally deduplicates errors, that's just turned off for UI tests. Does this duplication reproduce outside of UI tests? If not, it may be preferable to avoid the early abort, which we're generally trying to move away from where possible.

Huh, you're right; it doesn't seem to reproduce outside of UI tests. I thought I had found it "in the wild", but perhaps not.

@camelid
Copy link
Member

camelid commented Oct 15, 2021

I guess this PR and associated issue can be closed then?

@camelid
Copy link
Member

camelid commented Oct 15, 2021

Or maybe just repurpose this PR so it removes the FIXME in the test.

@GuillaumeGomez
Copy link
Member Author

As you prefer. I personally don't have preference here. Just like the borrow checker comes after the other checks, I don't see it abnormal to have the rustdoc checks only if rustc is happy.

@camelid
Copy link
Member

camelid commented Oct 15, 2021 •
edited
Loading

As you prefer. I personally don't have preference here. Just like the borrow checker comes after the other checks, I don't see it abnormal to have the rustdoc checks only if rustc is happy.

It'd be better to have as many errors as possible be reported in one build, as long as none are spurious. So I don't see why we should add this early abort, especially if T-compiler (or whichever team Mark was referring to) is moving away from it.

The borrow checker not running if earlier stages failed is just a technical limitation IIUC.

@GuillaumeGomez
Copy link
Member Author

Then I'll update the PR to remove the FIXME. :)

@GuillaumeGomez
Copy link
Member Author

And done!

@camelid camelid changed the title Abort when error happens and not later on Remove FIXME since there is nothing to be fixed Oct 16, 2021
@camelid
Copy link
Member

camelid commented Oct 16, 2021

r=me with the commit message amended to include something like this:

The errors are deduplicated when displayed to users. They only appear
multiple times in UI tests.

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2021
@GuillaumeGomez
Remove FIXME since there is nothing to be fixed.
7bad85e 
The errors are deduplicated when displayed to users. They only appear
multiple times in UI tests.
@GuillaumeGomez
Copy link
Member Author

Done!

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2021
@camelid
Copy link
Member

camelid commented Oct 16, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 16, 2021

📌 Commit 7bad85e has been approved by camelid

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 16, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the correctly-emit-errors branch 2 times, most recently from 629ec2a to 7bad85e Compare October 16, 2021 20:33
@camelid
Copy link
Member

camelid commented Oct 16, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Oct 16, 2021

📌 Commit 7bad85e has been approved by camelid

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 16, 2021
@GuillaumeGomez
Rollup merge of rust-lang#89907 - GuillaumeGomez:correctly-emit-error…
4163a39 
…s, r=camelid

Remove FIXME since there is nothing to be fixed

Resolves rust-lang#88593.

The errors are deduplicated when displayed to users. They only appear
multiple times in UI tests.

cc `@jyn514`
r? `@camelid`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 16, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Oct 16, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2021
@bors
Auto merge of rust-lang#89968 - JohnTitor:rollup-z51n967, r=JohnTitor
12b5bce 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#89507 (Add `#[repr(i8)]` to `Ordering`)
 - rust-lang#89849 (CI: Selecting the Xcode version no longer needed with the macos-11 runners.)
 - rust-lang#89886 (Update the wasi-libc built with the wasm32-wasi target)
 - rust-lang#89907 (Remove FIXME since there is nothing to be fixed)
 - rust-lang#89943 (clippy::complexity fixes)
 - rust-lang#89953 (Make Option::as_mut const)
 - rust-lang#89958 (Correct small typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c393f33 into rust-lang:master Oct 17, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 17, 2021
@GuillaumeGomez GuillaumeGomez deleted the correctly-emit-errors branch October 17, 2021 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@jyn514 jyn514 jyn514 approved these changes

Assignees

@camelid camelid

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-error-handling Area: Error handling S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

rustdoc reports error twice for ambiguous (inherent) associated type
7 participants
@GuillaumeGomez @camelid @Mark-Simulacrum @bors @jyn514 @rust-highfive @rustbot