Skip to content

Migrate rustc_monomorphize to use SessionDiagnostic #100730

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 9 commits into from
Aug 31, 2022
Merged

Migrate rustc_monomorphize to use SessionDiagnostic #100730

merged 9 commits into from
Aug 31, 2022

Conversation

CleanCut
Copy link
Contributor

@CleanCut CleanCut commented Aug 18, 2022 •
edited
Loading

Description

  • Migrates diagnostics in rustc_monomorphize to use SessionDiagnostic
  • Adds an impl IntoDiagnosticArg for PathBuf

TODO / Help!

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2022
@rust-highfive
Copy link
Contributor

r? @estebank

(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 Aug 18, 2022
@CleanCut CleanCut mentioned this pull request Aug 18, 2022
Open
84 tasks
compiler-errors
compiler-errors reviewed Aug 18, 2022
View reviewed changes
@JeanCASPAR
Copy link
Contributor

I'm having trouble figuring out how to apply an optional note. 😕 Help!?

You can pute the #[note] attribute on a Option<()> field. It will be displayed only when the field is Some, like I did here :
https://github.com/rust-lang/rust/pull/100724/files#diff-5fdce00f0b9b04a72a8bf8d6a0b8866deddf41201e908831c6f7cd444c64bef3R175

@CleanCut CleanCut mentioned this pull request Aug 18, 2022
Merged
@CleanCut
Copy link
Contributor Author

CleanCut commented Aug 18, 2022 •
edited
Loading

@JeanCASPAR The problem turned out to be my relying on incorrectly-documented syntax! (It was on an Option<()> field already). It seems to work, now. Or at least, it's no longer in the list of lint errors. 😉

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CleanCut CleanCut force-pushed the diagnostics-rustc_monomorphize branch from 70d79b0 to 2984519 Compare August 18, 2022 23:41
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CleanCut CleanCut mentioned this pull request Aug 19, 2022
Merged
@CleanCut CleanCut marked this pull request as ready for review August 19, 2022 00:54
@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank

@CleanCut
Copy link
Contributor Author

I've taken this PR out of draft status, because I'm now stuck and need a helpful reviewer to guide me forward. 😄

compiler-errors
compiler-errors reviewed Aug 19, 2022
View reviewed changes
compiler-errors
compiler-errors reviewed Aug 19, 2022
View reviewed changes
compiler-errors
compiler-errors reviewed Aug 19, 2022
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@davidtwco
Copy link
Member

errors:RecursionLimit should be #[fatal ...], but that doesn't exist so it's #[error ...] at the moment.

Just skip this one for now and rebase after @finalchild's #100694 lands with #[fatal(..)] support.

How does one go about converting an error inside of a call to struct_span_lint_hir?

You can use emit_spanned_lint, just use the LintDiagnostic derive instead of SessionDiagnostic with #[lint(..)] instead of #[error(..)]. These don't need #[primary_span] attributes, which we don't currently prevent but I'm going to make a task about fixing now.

What placeholder do you use in the fluent template to refer to the value in a vector? It seems like this code ought to have the answer (or something near it)...but I can't figure it out.

We support Vec<Span> fields, if you want to have one message in a bunch of locations, but we otherwise don't have an amazing answer for how to write out lists of items in a translatable way. This is an open question, feel free to read about Fluent and try work out a nice way to do it, then we can follow your approach wherever else this comes up, but you can also just skip a diagnostic with this for now.

davidtwco
davidtwco reviewed Aug 19, 2022
View reviewed changes
@Xiretza Xiretza mentioned this pull request Aug 19, 2022
Merged
@CleanCut

This comment was marked as resolved.

Sign in to view
@CleanCut CleanCut force-pushed the diagnostics-rustc_monomorphize branch from 71ac469 to 82d609c Compare August 25, 2022 17:07
@rustbot
Copy link
Collaborator

rustbot commented Aug 25, 2022

Some changes occurred in src/tools/cargo

cc @ehuss

@CleanCut
Copy link
Contributor Author

I assume that the codegen failure is spurious because nothing you've changed should make a difference there - once you've rebased and fixed the conflicts, we can try merging this. Thanks for sticking with it :)

I hope so. The local codegen failure persists even after the rebase.

davidtwco
davidtwco reviewed Aug 26, 2022
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I think we're almost ready to land this - you've pushed a submodule update for Cargo though, which shouldn't be there.

@CleanCut
Copy link
Contributor Author

I think we're almost ready to land this - you've pushed a submodule update for Cargo though, which shouldn't be there.

✅ Fixed the errant submodule change. Ready for re-review.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 31, 2022

📌 Commit 845d567 has been approved by davidtwco

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 Aug 31, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 31, 2022
@GuillaumeGomez
Rollup merge of rust-lang#100730 - CleanCut:diagnostics-rustc_monomor…
ac7705f 
…phize, r=davidtwco

Migrate rustc_monomorphize to use SessionDiagnostic

### Description

- Migrates diagnostics in `rustc_monomorphize` to use `SessionDiagnostic`
- Adds an `impl IntoDiagnosticArg for PathBuf`

### TODO / Help!
- [x] I'm having trouble figuring out how to apply an optional note. 😕  Help!?
  - Resolved. It was bad docs. Fixed in https://github.com/rust-lang/rustc-dev-guide/pull/1437/files
- [x] `errors:RecursionLimit` should be `#[fatal ...]`, but that doesn't exist so it's `#[error ...]` at the moment.
  - Maybe I can switch after this is merged in? --> rust-lang#100694
  - Or maybe I need to manually implement `SessionDiagnostic` instead of deriving it?
- [x] How does one go about converting an error inside of [a call to struct_span_lint_hir](https://github.com/rust-lang/rust/blob/8064a495086c2e63c0ef77e8e82fe3b9b5dc535f/compiler/rustc_monomorphize/src/collector.rs#L917-L927)?
- [x] ~What placeholder do you use in the fluent template to refer to the value in a vector? It seems like [this code](https://github.com/rust-lang/rust/blob/0b79f758c9aa6646606662a6d623a0752286cd17/compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs#L83-L114) ought to have the answer (or something near it)...but I can't figure it out.~ You can't. Punted.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Aug 31, 2022
Closed
@klensy
Copy link
Contributor

klensy commented Aug 31, 2022

Submodule changes still in commits history, can you actually rebase?

@RalfJung RalfJung mentioned this pull request Aug 31, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2022
@bors
Auto merge of rust-lang#101238 - RalfJung:rollup-bzcmobj, r=RalfJung
4fd4de7 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#90946 (Ignore `reference`s in "Type::inner_def_id")
 - rust-lang#100730 (Migrate rustc_monomorphize to use SessionDiagnostic)
 - rust-lang#100753 (translations(rustc_session): migrates `rustc_session` to use `SessionDiagnostic` - Pt. 1)
 - rust-lang#100831 (Migrate `symbol_mangling` module to new diagnostics structs)
 - rust-lang#101204 (rustdoc: Resugar async fn return type in `clean`, not `html`)
 - rust-lang#101216 (Use in-page links for sanitizer docs.)
 - rust-lang#101237 (fix into_iter on ZST)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6c4bda6 into rust-lang:master Aug 31, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 31, 2022
@CleanCut CleanCut deleted the diagnostics-rustc_monomorphize branch August 31, 2022 16:53
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Oct 23, 2022
@RalfJung
Rollup merge of rust-lang#100730 - CleanCut:diagnostics-rustc_monomor…
d60e930 
…phize, r=davidtwco

Migrate rustc_monomorphize to use SessionDiagnostic

### Description

- Migrates diagnostics in `rustc_monomorphize` to use `SessionDiagnostic`
- Adds an `impl IntoDiagnosticArg for PathBuf`

### TODO / Help!
- [x] I'm having trouble figuring out how to apply an optional note. 😕  Help!?
  - Resolved. It was bad docs. Fixed in https://github.com/rust-lang/rustc-dev-guide/pull/1437/files
- [x] `errors:RecursionLimit` should be `#[fatal ...]`, but that doesn't exist so it's `#[error ...]` at the moment.
  - Maybe I can switch after this is merged in? --> rust-lang#100694
  - Or maybe I need to manually implement `SessionDiagnostic` instead of deriving it?
- [x] How does one go about converting an error inside of [a call to struct_span_lint_hir](https://github.com/rust-lang/rust/blob/8064a495086c2e63c0ef77e8e82fe3b9b5dc535f/compiler/rustc_monomorphize/src/collector.rs#L917-L927)?
- [x] ~What placeholder do you use in the fluent template to refer to the value in a vector? It seems like [this code](https://github.com/rust-lang/rust/blob/0b79f758c9aa6646606662a6d623a0752286cd17/compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs#L83-L114) ought to have the answer (or something near it)...but I can't figure it out.~ You can't. Punted.
@rust-timer rust-timer mentioned this pull request Oct 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@davidtwco davidtwco davidtwco approved these changes

+1 more reviewer

@finalchild finalchild finalchild left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@davidtwco davidtwco

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.65.0
Development

Successfully merging this pull request may close these issues.