[GB18030] Fix "Unable to translate Unicode character" #10063

ilonatommy · 2024-04-24T07:56:30Z

Fixes #10066
[GB18030] Failed to run the WebAPI project that named as GB18030 provided level3 strings

Context

OS: Windows 11 Enterprise 22H2 ZH-CN
Affected Build: 9.0.0-preview.4.24209.5, Aso repro on 8.0.300-preview.24203.14(8.0.2)

Steps to reproduce:

Use CLI to create WebAPI project copying GB18030 characters, for example:

dotnet new webapi -controllers -o 㐇𠁠𪨰𫠊𫦠𮚮⿕#
cd 㐇𠁠𪨰𫠊𫦠𮚮⿕#
dotnet run

Without this PR:

dotnet run
Building...
Restore complete (0.5s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  㐇𠁠𪨰𫠊𫦠��⿕# failed with 2 error(s) (2.9s) → bin/Debug/net9.0/㐇𠁠𪨰𫠊𫦠��⿕#.dll
    /workspaces/runtime/.dotnet/sdk/9.0.100-preview.3.24204.13/Microsoft.Common.CurrentVersion.targets(5578,5): error MSB3491: Could not write lines to file "obj/Debug/net9.0/㐇𠁠𪨰𫠊𫦠��⿕#.csproj.FileListAbsolute.txt". Unable to translate Unicode character \\uD86E at index 2368 to specified code page.
    /workspaces/runtime/.dotnet/sdk/9.0.100-preview.3.24204.13/Microsoft.Common.CurrentVersion.targets(5823,5): error MSB3491: Could not write lines to file "obj/Debug/net9.0/㐇𠁠𪨰𫠊𫦠��⿕#.csproj.FileListAbsolute.txt". Unable to translate Unicode character \\uD86E at index 2368 to specified code page.

Build failed with 2 error(s) in 3.8s

The build failed. Fix the build errors and run again.

With this PR:

dotnet run
Building...
Restore complete (0.5s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  㐇𠁠𪨰𫠊𫦠��⿕# succeeded (0.6s) → bin/Debug/net9.0/㐇𠁠𪨰𫠊𫦠��⿕#.dll

Build succeeded in 1.5s
info: Microsoft.Hosting.Lifetime[14]
      Now listening on: http://localhost:5210
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: /workspaces/reproGB/㐇𠁠𪨰𫠊𫦠��⿕#
^Cinfo: Microsoft.Hosting.Lifetime[0]
      Application is shutting down...

Changes Made

Changed the way we create a substring to prevent cutting surrogates in half.

Testing

Added a test for new substring function.

Regerssion

Yes, after #9346.

ladipro · 2024-04-24T11:50:05Z

This change looks more like a workaround. Do we understand why \uD86E fails to be encoded to UTF-8? For what it's worth, this test program runs fine for me:

var defaultEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);
File.WriteAllText("C:\\temp\\encoded.txt", "\u3407\ud840\udc60\ud86a\ude30\ud86e\udc0a\ud86e\udda0\ud879\udeae\u2fd5\u0023", defaultEncoding);

defaultEncoding is the default used by the WriteLinesToFile task and the string is equivalent to the problematic project name, as far as I can tell.

ilonatommy · 2024-04-24T12:04:48Z

True, actually smallest repro does not help here either:

  <Target Name="MyTask" AfterTargets="Build">
    <WriteLinesToFile
      File="㐇𠁠𪨰𫠊𫦠𮚮⿕#\msEncoding.txt"
      Lines="㐇𠁠𪨰𫠊𫦠𮚮⿕#"
      Overwrite="true"/>
  </Target>

works.
Do you have an idea how to add a test to this repo to debug it better?

Edit:
it's comes down to a problem with MSBuildCopyMarkerName contents. You're right that the fix was not addressing the underlying issue. I will update the PR when I find the proper fix.

ilonatommy · 2024-04-24T13:24:47Z

If we moved

<MSBuildCopyMarkerName Condition="'$(MSBuildCopyMarkerName.Length)' &gt; '17'">$(MSBuildProjectFile.Substring(0,8)).$([MSBuild]::StableStringHash($(MSBuildProjectFile)).ToString("X8"))</MSBuildCopyMarkerName>

to C#:

public sealed class ShortenAndHashProjectName : Task
    {
        [Required]
        public string ProjectName { get; set; }

        [Output]
        public string ShortProjectName { get; set; }

        public override bool Execute()
        {
            if (ProjectName.Length <= 17)
            {
                ShortProjectName = ProjectName;
                return true;
            }

            // if the last char of string is a surrogate, cutting it in half would confuse encoder
            int length = char.IsHighSurrogate(ProjectName[7]) ? 9 : 8;
            string truncatedProjectName = ProjectName.Substring(0, length);
            string originalProjectNameHash = StableStringHash(ProjectName);
            ShortProjectName = $"{truncatedProjectName}.{originalProjectNameHash}".ToString("X8");
            return true;
    }

it should fix the issue. MsBuild does not provide a way to detect surrogates and if the last char is a beginning of surrogate, it cuts it in half.
Why is shortening the project name necessary?

ladipro · 2024-04-24T13:27:36Z

Tagging @JanKrivanek as this is an unfortunate regression introduced with the recent stable hash changes. Jan, do you think we can get away without shortening? if not, it would probably make sense to add a string shortening property function which would be safe with respect to surrogates.

JanKrivanek · 2024-04-24T13:31:15Z

Oh good find!

Shortening was one of the requests that the change was addressing. Let's move it to prop function

ladipro · 2024-04-24T13:36:52Z

@ilonatommy, rather than a task, we would prefer the functionality to be exposed as an intrinsic property function. Here's a PR where one was recently added: #9665

ilonatommy · 2024-04-24T13:36:58Z

@JanKrivanek could you please take over? I'm not sure how to test the changes and how to access StableStringHash method correctly.

JanKrivanek

Thnak you!

src/Build/Evaluation/IntrinsicFunctions.cs

ladipro

Thank you!

ladipro · 2024-04-25T08:48:08Z

The new prop function should be added to the doc page: https://learn.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-property-functions. Not urgent but we should make sure we don't forget.

KalleOlaviNiemitalo · 2024-04-25T11:12:43Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-    <!-- For a long MSBuildProjectFile let's shorten this to 17 chars - using the first 8 chars of the filename and a filename hash. -->
-    <MSBuildCopyMarkerName Condition="'$(MSBuildCopyMarkerName.Length)' &gt; '17'">$(MSBuildProjectFile.Substring(0,8)).$([MSBuild]::StableStringHash($(MSBuildProjectFile)).ToString("X8"))</MSBuildCopyMarkerName>
+    <!-- For a long MSBuildProjectFile let's shorten this to 17 chars - using the first 8 codepoints of the filename and a filename hash. -->
+    <MSBuildCopyMarkerName Condition="'$(MSBuildCopyMarkerName.Length)' &gt; '17'">$([MSBuild]::SubstringByTextElements($(MSBuildProjectFile), 0, 8)).$([MSBuild]::StableStringHash($(MSBuildProjectFile)).ToString("X8"))</MSBuildCopyMarkerName>


Those are now text elements rather than codepoints. Which has consequences:

$(MSBuildCopyMarkerName.Length) can still be greater than 17 after this shortening.

If $(MSBuildProjectFile) is 👨‍👨‍👦‍👦👨‍👨‍👦‍👦.proj, then StringInfo.SubstringByTextElements(0, 8) will throw ArgumentOutOfRangeException. That string has 27 UTF-16 code units but only 7 text elements.

If you're going to have an [MSBuild]::SubstringByTextElements intrinsic function, then either it should check for out-of-range arguments itself, or there should be an [MSBuild]::CountTextElements function as well. Otherwise it's too difficult to use reliably in projects.

But I think what this really needs is a property function that returns all the text elements that entirely fit within the specified substring, with arguments given in UTF-16 code units. So that the resulting string never has more UTF-16 code units than requested. Because those are what matters for file name lengths in NTFS.

These are excellent points. We may get away with this functionality to be just a best-effort "make the file name reasonably long" but we definitely must fix the possible ArgumentOutOfRangeException and it's worth thinking about a more useful property function too.

Should we consider moving the entire operation to a new, dedicated property function so we could just do

<MSBuildCopyMarkerName>$([MSBuild]::ShortUniqueProjectName())</MSBuildCopyMarkerName>

or similar?

I'd still keep it general for other strings as well - there seems to be other usages in org on path args that might need revision: https://github.com/search?q=repo%3Adotnet%2Fsdk+path%3A**%2F*.targets+substring&type=code

After re-checking the list of places where we use substring, none of them require a PR.

code in net5 - we do not backport to it

https://github.com/dotnet/sdk/blob/52acdcaad568cc56ca67b33b6b2ea7fa1e489d24/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L301 - the substring is done on a hash of a string, so it's always in a form e.g. _NormalizedPublishDirHash = 13cbcd0c3203541f1c707c5ea0b45e9f20f4d3cf71f0e2b68e723cab7e9427c3 - no surrogates.

https://github.com/dotnet/sdk/blob/1f91c5339897b46f5119ed5d43d9b29be237aca5/src/BuiltInTools/dotnet-watch/DotNetWatch.targets#L87 - the substring is executed only if the Identity starts with a 7-char string: wwwroot/, so the Substring(8) will never cut a surrogate in half.

Fix "Unable to translate Unicode character"

d9ad357

ilonatommy requested a review from maririos April 24, 2024 07:57

ilonatommy self-assigned this Apr 24, 2024

maridematte requested review from rainersigwald and ladipro April 24, 2024 09:28

Half-fix

e0fea3a

JanKrivanek mentioned this pull request Apr 24, 2024

[GB18030] UpToDate marker file name shortening doesn't respect surrogate chars #10066

Closed

JanKrivanek marked this pull request as draft April 24, 2024 13:42

Expose SubstringByTextElements in IntrinsicFunctions + test.

8fc3eed

ilonatommy removed the request for review from maririos April 24, 2024 15:20

JanKrivanek approved these changes Apr 24, 2024

View reviewed changes

ladipro reviewed Apr 24, 2024

View reviewed changes

src/Build/Evaluation/IntrinsicFunctions.cs Show resolved Hide resolved

Feedback.

259728c

ilonatommy marked this pull request as ready for review April 25, 2024 08:05

ladipro approved these changes Apr 25, 2024

View reviewed changes

Merge branch 'main' into fix-2031987

a7fca5a

JanKrivanek mentioned this pull request Apr 25, 2024

[GB18030] Audit shipped props and targets for usage of Substring on paths #10075

Closed

ladipro merged commit d4b9bd8 into dotnet:main Apr 25, 2024
10 checks passed

KalleOlaviNiemitalo reviewed Apr 25, 2024

View reviewed changes

ilonatommy mentioned this pull request Apr 29, 2024

[GB18030] Fix SubstringByTextElements - avoid ArgumentOutOfRangeException #10080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GB18030] Fix "Unable to translate Unicode character" #10063

[GB18030] Fix "Unable to translate Unicode character" #10063

ilonatommy commented Apr 24, 2024 •

edited by ladipro

Loading

ladipro commented Apr 24, 2024

ilonatommy commented Apr 24, 2024 •

edited

Loading

ilonatommy commented Apr 24, 2024 •

edited

Loading

ladipro commented Apr 24, 2024

JanKrivanek commented Apr 24, 2024

ladipro commented Apr 24, 2024

ilonatommy commented Apr 24, 2024

JanKrivanek left a comment

ladipro left a comment

ladipro commented Apr 25, 2024

KalleOlaviNiemitalo Apr 25, 2024

KalleOlaviNiemitalo Apr 25, 2024

KalleOlaviNiemitalo Apr 25, 2024

ladipro Apr 25, 2024

rainersigwald Apr 25, 2024

JanKrivanek Apr 29, 2024

ilonatommy May 22, 2024 •

edited

Loading

[GB18030] Fix "Unable to translate Unicode character" #10063

[GB18030] Fix "Unable to translate Unicode character" #10063

Conversation

ilonatommy commented Apr 24, 2024 • edited by ladipro Loading

Context

Changes Made

Testing

Regerssion

ladipro commented Apr 24, 2024

ilonatommy commented Apr 24, 2024 • edited Loading

ilonatommy commented Apr 24, 2024 • edited Loading

ladipro commented Apr 24, 2024

JanKrivanek commented Apr 24, 2024

ladipro commented Apr 24, 2024

ilonatommy commented Apr 24, 2024

JanKrivanek left a comment

Choose a reason for hiding this comment

ladipro left a comment

Choose a reason for hiding this comment

ladipro commented Apr 25, 2024

KalleOlaviNiemitalo Apr 25, 2024

Choose a reason for hiding this comment

KalleOlaviNiemitalo Apr 25, 2024

Choose a reason for hiding this comment

KalleOlaviNiemitalo Apr 25, 2024

Choose a reason for hiding this comment

ladipro Apr 25, 2024

Choose a reason for hiding this comment

rainersigwald Apr 25, 2024

Choose a reason for hiding this comment

JanKrivanek Apr 29, 2024

Choose a reason for hiding this comment

ilonatommy May 22, 2024 • edited Loading

Choose a reason for hiding this comment

ilonatommy commented Apr 24, 2024 •

edited by ladipro

Loading

ilonatommy commented Apr 24, 2024 •

edited

Loading

ilonatommy commented Apr 24, 2024 •

edited

Loading

ilonatommy May 22, 2024 •

edited

Loading