fix locale-sensitive comparison of UnmatchedArgument error strings #29559
Conversation
Co-authored-by: Amaury Levé <amaury.leve@gmail.com>
|
Just added a test that
This test works with en-US and de-DE cultures, which seem to cover a number of the differences in culture strings here. |
|
@baronfel any news on this one? |
…f format string and check for ordered presence of those in the formatted message.
Adds a helper method for setting culture to the test framework, driven from a shared reference to the hard-coded UI environment variable.
baronfel
force-pushed
the
fix-locale-comparison-bug
branch
from
cc4445e to
5bb33ac
Compare
|
I think we're in a good space now with the algorithm and the tests for this check. I'll want to get another check from @dsplaisted to make sure he's comfortable with the implementation, but then we're ready to go. A note about the timing of this fix: we are already beyond the merge window for the January servicing release of .NET 7 (which would be 7.0.102) due to the holiday season. As a result, this fix (when approved and merged) will only be available in the Februrary servicing release at earliest (so 7.0.103). This feels really bad, and isn't a great response time given the very fast time to reporting that users had, but it's the best I can do. In the meantime, the best thing I can suggest for users that are hitting this error is to force the CLI to the English local for |
|
@baronfel thanks for the fix! |
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
|
@baronfel this was approved for February. Please ensure the checks get resolved and this gets merged by eow. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Both of these check failures are due to the macos helix image expiration, which I think has already been handled in an earlier PR. Trying to kick these off again to see. |
Description
Closes #29543
Closes #28975
by fixing a helper method comparison that was intended to filter out unmatched token errors. Since errors in System.CommandLine parsing are strings, discovering the source of a particular string is hard. This method attempted to see if a 'masked' error string was entirely contained in the error string. This works for languages and grammars that put the argument name in single quotes at the end of the string, but for locales like
de-DEboth of these invariants were broken.The new method breaks the format string at the .NET format placeholders, then ensures that each of those literal portions exists in order in the final localized error message string.
This issue was customer reported.
Customer Impact
This potentially impacts build, pack, test, restore, list references, and other CLI Commands that use this same call to check for unparsed arguments. Users that used these commands in locales with non-English arrangements of locale strings would not be able to execute those commands. One workaround would be to force the use of English (or similarly-structured) locales.
Regression
Yes, but hidden under a couple other issues that we fixed in 7.0.101.
Risk
Low - the new checking logic seems to be stable under different locales, and has tests covering the locales that users have reported failing.
Testing
I locally tested this by using the DOTNET_CLI_UI_LANGUAGE environment variable to setup the triggering situation.
I added automated testing of different locales to validate the change.
Follow up
This kind of string manipulation is very error-prone, so I've reached out to the System.CommandLine API design group to consider a more structured approach.