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

More translatable diagnostics #123822

Closed
wants to merge 12 commits into from

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented Apr 11, 2024

cc #100717

@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 11, 2024
@pnkfelix
Copy link
Member

This looks fine to me from what I understand about it, but I am not an expert on the fluent machinery. The review should perhaps be assigned to @davidtwco or someone else who is more knowledgable there.

In any case, since this is marked as draft PR, I'm going to switch it back to waiting-on-author, to get it out of my review queue while it is in draft.

@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 Apr 12, 2024
@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch 2 times, most recently from ca845fe to 6d0b4f2 Compare April 12, 2024 19:55
@rust-log-analyzer

This comment has been minimized.

f: F,
) {
if let Some(issue) = self.issue {
issue.add_to_diag_with(diag, |diag, msg| f(diag, msg));
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'm not happy about this, ideally F would be a reference so it can be copied into nested add_to_diag_with() calls instead of being wrappen in another closure.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch 4 times, most recently from b41bc78 to 0094065 Compare April 16, 2024 20:28
@rust-log-analyzer

This comment has been minimized.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from 0094065 to a1d7d38 Compare April 17, 2024 20:49
@rust-log-analyzer

This comment has been minimized.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from a1d7d38 to 99f8205 Compare April 21, 2024 20:58
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Apr 21, 2024
@rust-log-analyzer

This comment has been minimized.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from 99f8205 to b1ae167 Compare April 23, 2024 16:19
@rust-log-analyzer

This comment has been minimized.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from b1ae167 to fb1b758 Compare April 24, 2024 21:30
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 27, 2024

☔ The latest upstream changes (presumably #124424) made this pull request unmergeable. Please resolve the merge conflicts.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from fb1b758 to f887d9f Compare May 20, 2024 17:20
@bors
Copy link
Contributor

bors commented May 20, 2024

☔ The latest upstream changes (presumably #125219) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@Xiretza can you resolve the latest conflicts? then we can push this further after that :) thanks

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from da8f1bd to a8265ed Compare August 23, 2024 18:16
@Xiretza Xiretza marked this pull request as ready for review August 27, 2024 16:40
@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2024

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

@bors
Copy link
Contributor

bors commented Aug 27, 2024

☔ The latest upstream changes (presumably #129665) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned cjgillot and unassigned pnkfelix Aug 29, 2024
@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from a8265ed to f490819 Compare August 29, 2024 18:03
@bors
Copy link
Contributor

bors commented Sep 11, 2024

☔ The latest upstream changes (presumably #130050) made this pull request unmergeable. Please resolve the merge conflicts.

@Xiretza Xiretza force-pushed the more-translatable-diagnostics branch from f490819 to bd98f08 Compare September 13, 2024 20:21
@Xiretza
Copy link
Contributor Author

Xiretza commented Sep 13, 2024

@cjgillot ping

mod fallback_supplement_co_v1;
#[doc(inline)]
pub use __impl_fallback_supplement_co_v1 as impl_fallback_supplement_co_v1;
#[macro_use]
#[path = "macros/list_and_v1.data.rs"]
#[path = "macros/list_and_v1.rs.data"]
mod list_and_v1;
#[doc(inline)]
pub use __impl_list_and_v1 as impl_list_and_v1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this sentence in the commit message:

The "pub use __impliterable*" lines in macros.rs need to be removed manually to avoid unused reexport errors.

Could an #[allow(unused_something)] help?

if sess.psess.unstable_features.is_nightly_build() {
if feature_from_cli {
err.subdiagnostic(CliFeatureDiagnosticHelp { feature });
let (enable_feature, upgrade_compiler) = if sess.psess.unstable_features.is_nightly_build() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split this in two? That will be more readable. Maybe cache is_nightly_build() as it looks at env vars.

self.dcx().create_err(errors::BadReturnTypeNotation::NeedsDots {
span: data.inputs_span,
})
errors::BadReturnTypeNotation::NeedsDots { span: data.inputs_span }
Copy link
Contributor

Choose a reason for hiding this comment

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

No subdiagnostic for this one?

@@ -987,6 +988,8 @@ pub(crate) struct MissingTraitItemUnstable {
pub some_note: bool,
#[note(hir_analysis_none_note)]
pub none_note: bool,
#[subdiagnostic]
pub subdiag: FeatureGateSubdiagnostic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a more descriptive name?

@@ -386,12 +387,16 @@ pub(crate) enum BadReturnTypeNotation {
#[primary_span]
#[suggestion(code = "()", applicability = "maybe-incorrect")]
span: Span,
#[subdiagnostic]
subdiag: Option<FeatureGateSubdiagnostic>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a more descriptive name?

},
#[diag(ast_lowering_bad_return_type_notation_output)]
Output {
#[primary_span]
#[suggestion(code = "", applicability = "maybe-incorrect")]
span: Span,
#[subdiagnostic]
subdiag: Option<FeatureGateSubdiagnostic>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a more descriptive name?

DiagArgValue::Str(Cow::Owned(pprust::tt_to_string(&self)))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put this change in the commit where you actually use it?

DiagArgValue::Str(Cow::from(self.to_string()))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put this change in the commit where you actually use it?

#[primary_span]
pub span: Span,
#[subdiagnostic]
pub subdiag: FeatureGateSubdiagnostic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a more descriptive name?

#[primary_span]
pub span: Span,
#[subdiagnostic]
pub subdiag: FeatureGateSubdiagnostic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a more descriptive name?

@cjgillot cjgillot 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 Sep 14, 2024
@bors
Copy link
Contributor

bors commented Sep 23, 2024

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@Xiretza any updates on this? thanks

@Xiretza
Copy link
Contributor Author

Xiretza commented Oct 26, 2024

I haven't had the required motivation to work on this, and now that the translation effort seems to have somwhat failed anyway, I doubt I will find it in the future.

@Xiretza Xiretza closed this Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
Development

Successfully merging this pull request may close these issues.

8 participants