Skip to content

Conversation

@rluble
Copy link
Contributor

@rluble rluble commented Mar 19, 2024

No description provided.

@kripken kripken enabled auto-merge (squash) March 19, 2024 21:45
@kripken kripken merged commit f9dc569 into WebAssembly:main Mar 19, 2024
@tlively
Copy link
Member

tlively commented Mar 20, 2024

Is this PR necessary? I was under the impression that string.concat blindly concatenates the strings, no matter what their contents are.

@rluble
Copy link
Contributor Author

rluble commented Mar 20, 2024

Is this PR necessary?

Necessary, yes. If you have malformed UTF-8 the first byte(s) of the second string might become a valid escape; so in general you would need to backoff to avoid that. However my thinking was that if the UTF-8 string came from a Java wtf-16 then it should be ok to concat, since in Java it just concats the chars, but alas this was tripping our test with invalid WTF-16.

@tlively
Copy link
Member

tlively commented Mar 20, 2024

Can you share the test case that was failing? I just want to make sure this isn't masking another bug somewhere else. As far as I can tell, as long as the strings are valid WTF-8, the only problem concatenation can cause is an explicit surrogate sequence that should be a single WTF-8 code point. We'll warn about that in string lowering, but the resulting JSON should still be ok.

@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants