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

fix locale-sensitive comparison of UnmatchedArgument error strings #29559

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Dec 14, 2022

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-DE both 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.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Shall we add the test with dotnet test and non-US locale as part of this PR or a follow-up one?

src/Cli/dotnet/ParseResultExtensions.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/ParseResultExtensions.cs Outdated Show resolved Hide resolved
baronfel and others added 2 commits December 14, 2022 14:58
@baronfel
Copy link
Member Author

Just added a test that

  • sets a locale
  • parses a dotnet test command with an unmatched argument
  • asserts that parse errors exist, and
  • asserts that our throw helper doesn't throw (because the errors are UnmatchedToken errors

This test works with en-US and de-DE cultures, which seem to cover a number of the differences in culture strings here.

@MarcoRossignoli
Copy link
Member

@baronfel any news on this one?

…f format string and check for ordered presence of those in the formatted message.
@baronfel baronfel requested a review from a team as a code owner December 19, 2022 19:39
Adds a helper method for setting culture to the test framework, driven from a shared reference to the hard-coded UI environment variable.
@baronfel
Copy link
Member Author

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 dotnet test. The easiest way to do that is to set the DOTNET_CLI_UI_LANGUAGE environment variable in your shell to en-US for the duration of the test command. e.g. in Bash you'd run DOTNET_CLI_UI_LANGUAGE='en-us' dotnet test ...... Full documentation for this environment variable can be found here.

@MarcoRossignoli
Copy link
Member

@baronfel thanks for the fix!

src/Cli/dotnet/ParseResultExtensions.cs Show resolved Hide resolved
src/Cli/dotnet/ParseResultExtensions.cs Outdated Show resolved Hide resolved
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@rbhanda rbhanda added this to the 7.0.3 milestone Jan 5, 2023
@marcpopMSFT
Copy link
Member

@baronfel this was approved for February. Please ensure the checks get resolved and this gets merged by eow.

@baronfel
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@baronfel
Copy link
Member Author

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.

@baronfel baronfel merged commit 9e80076 into dotnet:release/7.0.1xx Jan 12, 2023
@baronfel baronfel deleted the fix-locale-comparison-bug branch January 12, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants