Remove uses of StringLiteral in format strings. #4416
Conversation
jonmeow
force-pushed
the
string-whackamole
branch
from
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.
jonmeow
force-pushed
the
string-whackamole
branch
from
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.
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.
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.
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.
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.
String literals don't have lifetime issues, so we probably need a separate error message for that.
There was a problem hiding this comment.
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.
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.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
jonmeow
force-pushed
the
string-whackamole
branch
from
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::stringuses 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.