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.

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

Use diagnostic namespace in stdlib #119216

Merged
merged 4 commits into from
Jan 6, 2024

Conversation

weiznich
Copy link
Contributor

@weiznich weiznich commented Dec 22, 2023

This required a minor fix to have the diagnostics shown in third party crates when the diagnostic_namespace feature is not enabled. See 5d63f5d for details. I've opted for having a single PR for both changes as it's really not that much code. If it is required it should be easy to split up the change into several PR's.

r? @compiler-errors

@rustbot rustbot added 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 22, 2023
@rust-log-analyzer

This comment has been minimized.

@weiznich weiznich force-pushed the use_diagnostic_namespace_in_stdlib branch from 69b1c88 to 3350e5b Compare December 22, 2023 13:54
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

compiler-errors commented Dec 22, 2023

Can you add a UI test with // aux-build to show that a downstream crate doesn't need the feature gate enabled? Also, we already have a test that shows that we need the feature gate to use #[diagnostic::on_unimplemented(..)], right?

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 Dec 22, 2023
@weiznich
Copy link
Contributor Author

I've pushed 4ed3257 to add the requested test.

There is already https://github.com/rust-lang/rust/blob/master/tests/ui/diagnostic_namespace/feature-gate-diagnostic_namespace.rs for testing that the feature gate is required for using the whole namespace.

@rustbot reviwer

@weiznich
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot 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 Dec 23, 2023
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

Can you find out what is up with that failing rustdoc test?

@rustbot author

(also you can use rustbot ready to mark the PR as ready -- it's a shorthand for label: +S-waiting-on-review -S-waiting-on-author)

@rustbot rustbot 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 Dec 25, 2023
@weiznich weiznich force-pushed the use_diagnostic_namespace_in_stdlib branch from 4ed3257 to cc31adc Compare December 29, 2023 14:10
@weiznich
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Dec 29, 2023
@@ -10,7 +10,7 @@ where

// @has no_redundancy/struct.Outer.html
// @has - '//*[@id="synthetic-implementations-list"]//*[@class="impl"]//h3[@class="code-header"]' \
// "impl<T> Send for Outer<T>where T: Send + Copy"
// "impl<T> Send for Outer<T>where T: Copy + Send"
Copy link
Member

Choose a reason for hiding this comment

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

Why does this fix a test?

Copy link
Member

@fmease fmease Jan 4, 2024

Choose a reason for hiding this comment

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

Me being quite familiar with src/librustdoc/clean/, I presume that this is rustdoc's fault.
rustdoc still uses a lot of FxHash{Set,Map}s in its code base (20 occurrences of them in total in clean/auto_trait.rs which is responsible for generating this Send impl).

I'm not sure why exactly it manifests like that but FxHash{Set,Map} (while stable under re-runs & recompilations of core and rustdoc) is “unstable under rmeta changes”. Since this PR has modified core (more specificallySend), core's rmeta file has changed and rustdoc being rustdoc now generates different output. Mind you, this will stay like that, it's not a flaky test (at least if my theory checks out).

To give you another example, in PR #116388 I was able to get rid of several cases of this “instability” in rustdoc. Check out the Example <details/> in the PR description.


@compiler-errors, since this is a bug in rustdoc and we can't do anything short-term here, this PR shouldn't be blocked on it. I officially approve the rustdoc test changes, so to say ^^.

Copy link
Member

Choose a reason for hiding this comment

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

I've just opened #119597 for that.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit cc31adc has been approved by compiler-errors

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 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
…in_stdlib, r=compiler-errors

Use diagnostic namespace in stdlib

This required a minor fix to have the diagnostics shown in third party crates when the `diagnostic_namespace` feature is not enabled. See rust-lang@5d63f5d for details. I've opted for having a single PR for both changes as it's really not that much code. If it is required it should be easy to split up the change into several PR's.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118680 (Add support for shell argfiles)
 - rust-lang#119216 (Use diagnostic namespace in stdlib)
 - rust-lang#119350 (Imply outlives-bounds on lazy type aliases)
 - rust-lang#119538 (Cleanup error handlers: round 5)
 - rust-lang#119563 (Check yield terminator's resume type in borrowck)
 - rust-lang#119577 (Migrate memory overlap check from validator to lint)
 - rust-lang#119589 (cstore: Remove unnecessary locking from `CrateMetadata`)

Failed merges:

 - rust-lang#119591 (rustc_mir_transform: Make DestinationPropagation stable for queries)

r? `@ghost`
`@rustbot` modify labels: rollup
@fmease
Copy link
Member

fmease commented Jan 5, 2024

Oh no, the order swapped again in a rollup, see #119609 (comment). Since I still assume that this isn't a flaky test, some PR inside that rollup might have had an effect?

Unapproving for now, maybe until all the PRs from that rollup have been merged? @weiznich, in the meantime you could rebase this branch to see if this issue has fixed itself or if CI still fails (maybe my theory #119597 doesn't check out 😅).

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 5, 2024
@fmease
Copy link
Member

fmease commented Jan 5, 2024

If it comes to the worst and CI still fails you can also // ignore-test with a // FIXME(fmease, #119216): Re-enable this test! and I will bors-reapprove the PR on behalf of compiler-errors.

Sorry for the bad contributor experience :/

nightly feature

(Using this attribute still requires a nightly feature, this just
enables that this feature does not need to be enabled on the child crate
as well)
`#[diagnostic::on_unimplemented]`

This commit replaces those `#[rustc_on_unimplemented]` attributes with
their equivalent `#[diagnostic::on_unimplemented]` where this is
supported (So no filter or any extended option)
@weiznich weiznich force-pushed the use_diagnostic_namespace_in_stdlib branch from cc31adc to 7a6c740 Compare January 5, 2024 14:29
@weiznich
Copy link
Contributor Author

weiznich commented Jan 5, 2024

I've pushed a rebased version, but at least according to my local tests that does not change anything (the test continues to pass without any change compared to the old PR version).

@fmease
Copy link
Member

fmease commented Jan 5, 2024

Alright, let me take over for a sec (i.e., no pulls from and pushes to this branch). Imma run an experiment...

@fmease
Copy link
Member

fmease commented Jan 5, 2024

@bors try

@bors
Copy link
Contributor

bors commented Jan 5, 2024

⌛ Trying commit 7a6c740 with merge acc01ee...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
…_stdlib, r=<try>

Use diagnostic namespace in stdlib

This required a minor fix to have the diagnostics shown in third party crates when the `diagnostic_namespace` feature is not enabled. See rust-lang@5d63f5d for details. I've opted for having a single PR for both changes as it's really not that much code. If it is required it should be easy to split up the change into several PR's.

r? `@compiler-errors`
@bors
Copy link
Contributor

bors commented Jan 5, 2024

☀️ Try build successful - checks-actions
Build commit: acc01ee (acc01ee1539a447c0ecf28ef41eea721270afedc)

@fmease fmease force-pushed the use_diagnostic_namespace_in_stdlib branch from 7a6c740 to 54967d7 Compare January 5, 2024 19:35
@fmease
Copy link
Member

fmease commented Jan 5, 2024

Okay, so apparently these fluctuations can be unstable under architecture. Here: x86_64-gnu-llvm-16 vs. aarch-64-gnu. I've added ignore-test to the rustdoc test and self-assigned #119597.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jan 5, 2024

📌 Commit 54967d7 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 6, 2024
…in_stdlib, r=compiler-errors

Use diagnostic namespace in stdlib

This required a minor fix to have the diagnostics shown in third party crates when the `diagnostic_namespace` feature is not enabled. See rust-lang@5d63f5d for details. I've opted for having a single PR for both changes as it's really not that much code. If it is required it should be easy to split up the change into several PR's.

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2024
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119208 (coverage: Hoist some complex code out of the main span refinement loop)
 - rust-lang#119216 (Use diagnostic namespace in stdlib)
 - rust-lang#119414 (bootstrap: Move -Clto= setting from Rustc::run to rustc_cargo)
 - rust-lang#119420 (Handle ForeignItem as TAIT scope.)
 - rust-lang#119468 (rustdoc-search: tighter encoding for f index)
 - rust-lang#119628 (remove duplicate test)
 - rust-lang#119638 (fix cyle error when suggesting to use associated function instead of constructor)
 - rust-lang#119640 (library: Fix warnings in rtstartup)
 - rust-lang#119642 (library: Fix a symlink test failing on Windows)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d90c702 into rust-lang:master Jan 6, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2024
Rollup merge of rust-lang#119216 - weiznich:use_diagnostic_namespace_in_stdlib, r=compiler-errors

Use diagnostic namespace in stdlib

This required a minor fix to have the diagnostics shown in third party crates when the `diagnostic_namespace` feature is not enabled. See rust-lang@5d63f5d for details. I've opted for having a single PR for both changes as it's really not that much code. If it is required it should be easy to split up the change into several PR's.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants