-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Remove uses of StringLiteral in format strings. #4416
Conversation
f4f7dee
to
ff3c526
Compare
Per discussion on #toolchain, add "s" as a special-case for the common plural format. Note this removes periods from a few diagnostics; the periods shouldn't be there per message style. Also, while I'm ignoring llvm::StringLiteral uses, those should be addressed as #4416 -- this'll probably conflict and make me clean up one or the other.
ff3c526
to
929b5ca
Compare
NB, rebased due to conflicting changes in #4423, figured you probably hadn't looked at this so probably easier to merge now. |
CARBON_DIAGNOSTIC(InCallToEntity, Note, "calling {0} declared here", | ||
llvm::StringLiteral); | ||
"{0} argument(s) passed to " | ||
"{1:=0:function|=1:generic class|=2:generic interface}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I'm not sure I agree with the thesis that the string "should probably be written in the format string instead of separately". This change makes it possible to spot bugs in the diagnostic's phrasing by local inspection, but the tradeoff is that it makes it noticeably harder for me to read the format string as a whole, and it introduces a new point of failure: the int -> kind mapping in the format string might not match the kind -> int mapping in the enum
(which can't be spotted by local inspection).
If you want to make sure phrasing problems can be spotted locally, I'd suggest that we stick with the previous format string, but map the enum to a string literal locally. That way all the components of the message are available locally, and there's no potentially error-prone indirection through integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a phrasing issue; it's one of translatability. For example, let's say someone wants to translate diagnostics to French. If "generic" is part of the hardcoded inputs, how does someone provide a translation for it?
In this model, someone just needs to provide an appropriate format string, and there should be sufficient context that they can adjust ordering as needed.
To be clear, it's not the ability to spot bugs. It's avoiding fundamentally English-centric design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note too, we'd still need to add the ability to replace [ed: format] strings (that's just not here right now). But the intent here is to stop [ed: or at least reduce] building up technical debt in terms of diagnostics that would be incompatible with translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; that makes sense. I'm still concerned about the risk of bugs from the indirection through integers, and I feel like we can address that in a way that's consistent with translatability, but I'm happy to treat that as future work.
toolchain/diagnostics/diagnostic.h
Outdated
static_assert((... && !(std::is_same_v<Args, llvm::StringRef> || | ||
std::is_same_v<Args, llvm::StringLiteral>)), | ||
"For diagnostics, use a format provider (see " | ||
"toolchain/diagnostics/format_providers.h) or std::string to " | ||
"avoid lifetime issues."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String literals don't have lifetime issues, so we probably need a separate error message for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the error message to just point at the diagnostic parameter type advice.
And fixed the StringLiteral note there.
CARBON_DIAGNOSTIC(InCallToEntity, Note, "calling {0} declared here", | ||
llvm::StringLiteral); | ||
"{0} argument(s) passed to " | ||
"{1:=0:function|=1:generic class|=2:generic interface}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; that makes sense. I'm still concerned about the risk of bugs from the indirection through integers, and I feel like we can address that in a way that's consistent with translatability, but I'm happy to treat that as future work.
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
397f422
to
d21fe19
Compare
Building on #4411, avoid using StringLiteral in format strings. This includes a diagnostic check to prevent regressions (which is also how I gathered issues).
Note, I haven't looked at
std::string
uses yet, but we might need things like that to be able to pass strings in code back to the user. StringLiteral though means that it's literally written down in the toolchain, at which point it should probably be written in the format string instead of separately.