dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.43k stars 4.76k forks source link

Marshaling string.Empty across the C#<->JS boundary may be broken #69002

Closed kg closed 2 years ago

kg commented 2 years ago

It seems like at some point recently we may have broken the ability to properly marshal string.Empty across the language boundary. Blazor has some various bits that do things like str ?? string.Empty or str ?? "" and those used to work but no longer do, for some reason they're printing null. If you replace them with $"{str}" that doesn't work either, but $"'{str}'" will produce '' like you would expect. The problem seems to be specific to 0-character strings.

I dug through the related code and tried some various changes but I can't figure out what's broken. At this point I also don't see anything wrong with the C# responsible for it in their implementation, but I may be missing something because it's very weird.

cc @pavelsavara @maraf since we've all been touching some related stuff lately, maybe one of you has ideas?

I can reproduce this locally in a built aspnetcore checkout via: aspnetcore\src\Components\test\E2ETest> dotnet test -c Debug --filter Microsoft.AspNetCore.Components.E2ETest.Tests.RoutingTest.CanArriveAtPageWithOptionalParametersNotProvided

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

Tagging subscribers to 'arch-wasm': @lewing See info in area-owners.md if you want to be subscribed.

Issue Details
It seems like at some point recently we may have broken the ability to properly marshal string.Empty across the language boundary. Blazor has some various bits that do things like ```str ?? string.Empty``` or ```str ?? ""``` and those used to work but no longer do, for some reason they're printing ```null```. If you replace them with ```$"{str}"``` that doesn't work either, but ```$"'{str}'"``` will produce ```''``` like you would expect. The problem seems to be specific to 0-character strings. I dug through the related code and tried some various changes but I can't figure out what's broken. At this point I also don't see anything wrong with the C# responsible for it in their implementation, but I may be missing something because it's very weird. cc @pavelsavara @maraf since we've all been touching some related stuff lately, maybe one of you has ideas? I can reproduce this locally in a built aspnetcore checkout via: ```aspnetcore\src\Components\test\E2ETest> dotnet test -c Debug --filter Microsoft.AspNetCore.Components.E2ETest.Tests.RoutingTest.CanArriveAtPageWithOptionalParametersNotProvided```
Author: kg
Assignees: -
Labels: `bug`, `arch-wasm`, `blazor-wasm`
Milestone: -
kg commented 2 years ago

I'll note that copy_root erroneously does

        if (pLengthBytes && pChars) {

instead of

        if (lengthBytes && pChars) {

but that doesn't actually fix it.

pavelsavara commented 2 years ago

@kg could you please describe expected vs actual behavior you observe ?

kg commented 2 years ago

The test fails because a "" becomes "null" instead of "", producing "your age is null." In the test output

On May 8, 2022 12:22:42 AM PDT, Pavel Savara @.> wrote: @. could you please describe expected vs actual behavior you observe ?

-- Reply to this email directly or view it on GitHub: https://github.com/dotnet/runtime/issues/69002#issuecomment-1120365566 You are receiving this because you were mentioned.

Message ID: @.***>

  • kg (mobile)
pavelsavara commented 2 years ago

I run into similar issue recently. My root cause was that JavaScript

var a=""
if (!a){
  throw new Error("this could be null, undefined or empty")
}

the proper test is

var a=""
if (a===undefined || a===null){
  throw new Error("this could be null or undefined but not empty")
}
pavelsavara commented 2 years ago

Probably https://github.com/dotnet/runtime/blob/24714ef76bad58d04e014f9a7496035a407ed800/src/mono/wasm/runtime/strings.ts#L62 And https://github.com/dotnet/runtime/blob/24714ef76bad58d04e014f9a7496035a407ed800/src/mono/wasm/runtime/strings.ts#L126

kg commented 2 years ago

Part of the problem was how I was using the string decoder in blazor. Going to open a PR to address our side