Migrate rustdoc diagnostics to translatable diagnostics
#112807
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @jsha (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@rustbot label +A-translation |
rustbot
added
the
A-translation
This comment has been minimized.
This comment has been minimized.
src/librustdoc/lib.rs
Outdated
| let reported = diag.struct_err(err).emit(); | ||
| Err(reported) | ||
| } | ||
| Err(error) => Err(diag.emit_err(MainError { error })), |
There was a problem hiding this comment.
I don't think it makes sense to "translate" this -- it's not any better in this current state, IMO. Can you just add diagnostic_outside_of_impl for this function and remove MainError?
Ideally we'd change Result<(), String> into something that's like Result<(), T> where T: IntoDiagnostic or something...
There was a problem hiding this comment.
After some thought, realized that the error string that we got here comes from Result, which is in English.
If we would like to make this section translatable, then we need to support translations of Result::Err. Is this even feasible?
src/librustdoc/messages.ftl
Outdated
| couldn't generate documentation: {$error} | ||
| .note = failed to create or modify "{$file}" | ||
|
|
||
| rustdoc_compilation_failed = Compilation failed, aborting rustdoc |
There was a problem hiding this comment.
I know this is pre-existing, but it's conventional for diagnostics to be lowercase. Not sure if rustdoc is special in this regard or if this was just an oversight.
Suggested change
| rustdoc_compilation_failed = Compilation failed, aborting rustdoc | |
| rustdoc_compilation_failed = compilation failed, aborting rustdoc |
There was a problem hiding this comment.
Most of the diagnostics in rustdoc are in lowercase (around < 5 that are not).
Shall we set all them to lowercase? @jsha
src/librustdoc/lib.rs
Outdated
| } | ||
| Err(e) => Err(tcx.sess.emit_err(CouldntGenerateDocumentation { | ||
| error: e.error, | ||
| file: e.file.display().to_string(), |
There was a problem hiding this comment.
Is file ever actually empty in practice? If so, I think it may be better to represent this as Option<String> so you can just annotate it with #[note] instead of having a custom impl IntoDiagnostic<'_> for CouldntGenerateDocumentation.
There was a problem hiding this comment.
This is another alternative that I found, it requires adding another struct for a Subdiagnostic note.
#[derive(Diagnostic)]
#[diag(rustdoc_couldnt_generate_documentation)]
pub(crate) struct CouldntGenerateDocumentation {
pub(crate) error: String,
#[subdiagnostic]
pub(crate) file: Option<FailedToCreateOrModifyFile>,
}
#[derive(Subdiagnostic)]
#[note(rustdoc_failed_to_create_or_modify_file)]
pub(crate) struct FailedToCreateOrModifyFile {
pub(crate) file: String,
}There was a problem hiding this comment.
Having Option<String> for the file field (in the version specified in this review comment) and adding #[note] to the struct causes compilation failures.
This is because Option<String> doesn't implement IntoDiagnosticArgs. Implementing IntoDiagnosticArgs for Option<String> is also not allowed due to the orphan rules. Creating a new (wrapped) type for Option<String>, and then implementing the trait wouldn't work because the Option is "gone" from the struct field.
CMIIW here :)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nicklimmm
force-pushed
the
rustdoc-translatable-diagnostics-migration
branch
from
05b2de3 to
d0cf772
Compare
This comment has been minimized.
This comment has been minimized.
nicklimmm
force-pushed
the
rustdoc-translatable-diagnostics-migration
branch
from
d0cf772 to
b5be644
Compare
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #114905) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@nicklimmm this is waiting on you to resolve the conflicts and the CI error. Let me know if you have any updates |
Will catch up on this soon, currently packed with other stuff. |
|
@nicklimmm any updates on this? thanks |
fmease
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Diagnostic translation tracking issue: #100717