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 DiagnosticBuilder.quiet #93229

Merged
merged 1 commit into from
Jan 24, 2022
Merged

Remove DiagnosticBuilder.quiet #93229

merged 1 commit into from
Jan 24, 2022

Conversation

mark-i-m
Copy link
Member

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2022
@mark-i-m mark-i-m mentioned this pull request Jan 23, 2022
Merged
@GuillaumeGomez
Copy link
Member

Like I said, please provide an alternative before removing this.

@GuillaumeGomez
Copy link
Member

Talked with @oli-obk and the simplest solution is to just fix errors reported by the compiler (some missing cfg in the std modules). All good for me then!

@eddyb
Copy link
Member

eddyb commented Jan 23, 2022

If it's an acceptable tradeoff, I would use the typical pattern of requesting TypeckResults when visiting each HIR body, effectively switching the rustdoc default of omitting typeck on anything that isn't requested by something else (e.g. a const fn being called from an array length).

It might even be easier to not engage in those name resolution / typeck omissions in the first place, if it produces better errors overall.
So basically when this annotating feature is being used, rustdoc would stop being "lenient" with regards to cfg'd out code, and start being "strict", in order to have as much information as possible.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 Commit cf382de has been approved by eddyb

@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 23, 2022
@Manishearth
Copy link
Member

Manishearth commented Jan 23, 2022 •
edited
Loading

Like I said, please provide an alternative before removing this.

@GuillaumeGomez my understanding is that this feature is causing soundness issues (because it turns errors off completely). From @eddyb's reaction when talking to me about it it seems like this very much should never have happened.

As such I see zero reason why an experimental rustdoc feature should delay the fixing of such a bug.

Like, I know the issue got resolved already, but I want to make sure our priorities are straight here.

@mark-i-m
Copy link
Member Author

@Manishearth Perhaps you (like me) missed their most recent message?

@Manishearth
Copy link
Member

Manishearth commented Jan 23, 2022 •
edited
Loading

@mark-i-m No, I saw the updates (and there's a line in my comment acknowledging the issue got resolved already), from the rustdoc side I'm very strongly against bugs in this experimental feature blocking soundness fixes (or really, any compiler work) and want to push back against our team adopting such an attitude.

@GuillaumeGomez
Copy link
Member

I don't think what I asked was unreasonable. This has been around for months and it seems normal for me to worry about how to go around this limitation.

No, I saw the updates (and there's a line in my comment acknowledging the issue got resolved already), from the rustdoc side I'm very strongly against bugs in this experimental feature blocking soundness fixes (or really, any compiler work) and want to push back against our team adopting such an attitude.

Humans make mistakes. Please scold me in private next time. There is nothing about "such an attitude", just a legitimate concern.

@eddyb
Copy link
Member

eddyb commented Jan 23, 2022 •
edited
Loading

This has been around for months

To be fair, it's been around for months with rustc changes unreviewed by @rust-lang/compiler - I haven't been that active and only noticed because @mark-i-m pinged me on a PR that seems unsound (or rather, reveals pre-existing risks) in the presence of such a mechanism to silence even the compiler-observable effects of errors (e.g. ty::Error). It could've been an year (or more) later, if nobody noticed.
The situation is similar to #59789, and there might be others. I'm not sure what to do, other than impose stricter review requirements for changes in the compiler directory but... that's not also foolproof.

@GuillaumeGomez
Copy link
Member

As far as I know it's only used in rustdoc but I understand your problem with it. As for this, since it was in the error system, I thought that @estebank was the one responsible. Sorry for the noise.

@matthiaskrgr matthiaskrgr mentioned this pull request Jan 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2022
@bors
Auto merge of rust-lang#93245 - matthiaskrgr:rollup-djsi6jr, r=matthi…
42313dd 
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91526 (rustc_lint: Some early linting refactorings)
 - rust-lang#92555 (Implement RFC 3151: Scoped threads.)
 - rust-lang#93213 (Fix `let_chains` and `if_let_guard` feature flags)
 - rust-lang#93219 (Add preliminary support for inline assembly for msp430.)
 - rust-lang#93226 (Normalize field access types during borrowck)
 - rust-lang#93227 (Liberate late bound regions when collecting GAT substs in wfcheck)
 - rust-lang#93229 (Remove DiagnosticBuilder.quiet)
 - rust-lang#93234 (rustc_mir_itertools: Avoid needless `collect` with itertools)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a1645e5 into rust-lang:master Jan 24, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 24, 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

@Manishearth Manishearth Manishearth approved these changes

Assignees

@eddyb eddyb

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@mark-i-m @GuillaumeGomez @eddyb @bors @Manishearth @rust-highfive @rustbot