Skip to content

minicore: use core's diagnostic::on_unimplemented messages #143329

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
Merged
merged 1 commit into from
Jul 3, 2025
Merged

minicore: use core's diagnostic::on_unimplemented messages #143329

merged 1 commit into from
Jul 3, 2025
+67 −21

Conversation

folkertdev
Copy link
Contributor

Without these attributes, the error message is different. Keeping the diagnostics up-to-date seems related to #137531.

The modified test files are reported in #143319 as failing for --target=riscv64gc-unknown-linux-gnu. Using minicore for them makes it easier to troubleshoot this sort of issue.

r? @jieyouxu
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2025

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.
@rustbot rustbot added A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` 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 Jul 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2025

This PR modifies tests/auxiliary/minicore.rs.

cc @jieyouxu
jieyouxu
jieyouxu reviewed Jul 2, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Note that #137531 is only intended for the type signature (and lang items and whatnot), wasn't really intended for diagnostics attributes.

As for this, I'm not super sure. Why does the diagnostics difference only affect the riscv target?
tests/auxiliary/minicore.rs
Comment on lines +44 to +48
#[diagnostic::on_unimplemented(
message = "values of type `{Self}` may or may not have a size",
label = "may or may not have a known size"
)]
Copy link
Member

@jieyouxu jieyouxu Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Discussion [DIAG 1/2]: hm, this is kinda annoying because then you'd have to keep the diagnostic attribute content in sync, but I'm not sure of a better solution so this seems reasonable.
tests/auxiliary/minicore.rs
Comment on lines +44 to +48
#[diagnostic::on_unimplemented(
message = "values of type `{Self}` may or may not have a size",
label = "may or may not have a known size"
)]
Copy link
Member

@jieyouxu jieyouxu Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion [DIAG 2/2]: if we decide to go with this, can you make sure to leave a comment mentioning needing to sync the diagnostics attribute in the real core?
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 added a note at the top of minicore.rs, or did you mean to add the note in library/core?
Copy link
Member

Choose a reason for hiding this comment

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

To make this easier to discover, can you also add this note additionally to:

  1. The real library/core and say something like "please keep the #[diagnostic::on_unimplemented] attributes in sync with tests/auxiliary/minicore.rs
  2. And also add a quick section about the sync requirement to rustc-dev-guide's minicore page https://rustc-dev-guide.rust-lang.org/tests/minicore.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.

library/core/lib.rs didn't really have a place where that comment felt right (and would be seen), so I added a note to library/core/src/marker.rs, where the diagnostics we now replicate actually live.
@folkertdev
Copy link
Contributor Author

Note that #137531 is only intended for the type signature (and lang items and whatnot), wasn't really intended for diagnostics attributes.

I know, it's just that the diagnostics are, as you note, now something to also keep up-to-date.

As for this, I'm not super sure. Why does the diagnostics difference only affect the riscv target?

It doesn't, the difference is between with and without the real core. I only observed this difference by testing with riscv, but the discrepancy it's not target-specific, see https://godbolt.org/z/Een3KqjKj.
@folkertdev folkertdev force-pushed the minicore-diagnostic-on-unimplemented branch from bc5bca7 to 62b64f5 Compare July 2, 2025 14:51
@jieyouxu
Copy link
Member

jieyouxu commented Jul 3, 2025

Thanks, the changes look good, just #143329 (comment) to help other contributors discover this is a thing.

@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 Jul 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.
@folkertdev folkertdev force-pushed the minicore-diagnostic-on-unimplemented branch from 62b64f5 to 8512e39 Compare July 3, 2025 08:23
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang
@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jul 3, 2025
@folkertdev
Copy link
Contributor Author

the notes were added

@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 Jul 3, 2025
jieyouxu
jieyouxu approved these changes Jul 3, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, r=me once PR CI is green.
@jieyouxu
Copy link
Member

jieyouxu commented Jul 3, 2025

@bors delegate+ rollup
@bors
Copy link
Collaborator

bors commented Jul 3, 2025

✌️ @folkertdev, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu
@folkertdev
Copy link
Contributor Author

@bors r=@jieyouxu
@bors
Copy link
Collaborator

bors commented Jul 3, 2025

📌 Commit 8512e39 has been approved by jieyouxu

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 Jul 3, 2025
jdonszelmann added a commit to jdonszelmann/rust that referenced this pull request Jul 3, 2025
@jdonszelmann
Rollup merge of rust-lang#143329 - folkertdev:minicore-diagnostic-on-…
b5a835d 
…unimplemented, r=jieyouxu

minicore: use core's `diagnostic::on_unimplemented` messages

Without these attributes, the error message is different. Keeping the diagnostics up-to-date seems related to rust-lang#137531.

The modified test files are reported in rust-lang#143319 as failing for `--target=riscv64gc-unknown-linux-gnu`. Using `minicore` for them makes it easier to troubleshoot this sort of issue.

r? `@jieyouxu`
This was referenced Jul 3, 2025
Closed
Merged
bors added a commit that referenced this pull request Jul 3, 2025
@bors
Auto merge of #143363 - jdonszelmann:rollup-7cv1kql, r=jdonszelmann
a413f77 
Rollup of 6 pull requests

Successful merges:

 - #134006 (setup typos check in CI)
 - #142876 (Port `#[target_feature]` to new attribute parsing infrastructure)
 - #143038 (avoid suggesting traits from private dependencies)
 - #143083 (Fix rustdoc not correctly showing attributes on re-exports)
 - #143283 (document optional jobs)
 - #143329 (minicore: use core's `diagnostic::on_unimplemented` messages)

Failed merges:

 - #143237 (Port `#[no_implicit_prelude]` to the new attribute parsing infrastructure)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f85283b into rust-lang:master Jul 3, 2025
10 checks passed
rust-timer added a commit that referenced this pull request Jul 3, 2025
@rust-timer
Unrolled build for #143329
fc57c8a 
Rollup merge of #143329 - folkertdev:minicore-diagnostic-on-unimplemented, r=jieyouxu

minicore: use core's `diagnostic::on_unimplemented` messages

Without these attributes, the error message is different. Keeping the diagnostics up-to-date seems related to #137531.

The modified test files are reported in #143319 as failing for `--target=riscv64gc-unknown-linux-gnu`. Using `minicore` for them makes it easier to troubleshoot this sort of issue.

r? ``@jieyouxu``
@rustbot rustbot added this to the 1.90.0 milestone Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` 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.
4 participants
@folkertdev @rustbot @jieyouxu @bors