Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for UTF8 string literals #2940
base: master
Are you sure you want to change the base?
Add support for UTF8 string literals #2940
Changes from all commits
705bc66
4430981
b09a3a8
efcfa2f
c73643b
bf247d2
9ac57f4
6841729
54653f8
318b569
5ed4a7f
e8e2a3e
6d7678d
3574f4d
d7d764b
a9fe1a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run the integration test locally and saw this line isn't mutated correctly. The mutation causes compile errors. I think this isn't related to the mutator but related to the mutation placing logic. I'd say we create a separate defect for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in this PR, not as a separate defect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've investigated why this test is failing. It fails during mutated code compilation. Stryker by default add ternary expression to each operand:
So this statement
is transformed into this:
It boxes each UTF-8 value to
ReadOnlySpan<byte>()
type. The problem lies with C# and how it allows to concatenate UTF-8 string literals. It allows to concatenate UTF-8 string directly:"Hello"u8 + " "u8 + "World!"u8
, but not when they are represented asReadOnlySpan<byte>
:.new ReadOnlySpan<byte>() + new ReadOnlySpan<byte>()
Hence, mutated code fails to compile with:
Do you think it's worth fixing this bug in current PR? It also touches core
MutantControl
injection logic. If so, any new ideas are welcome here. So far, I've only thought of converting UTF-8 strings into arrays and concatenating them manually using LINQ when ternary conditions are used:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. In that case, we could maybe include some extra code in the compilation that contains an operator overload for
+
onReadOnlySpan<byte>
. During the compilation we already pass some custom code to be compiled together with the source project, so that should be fairly easy to do.Yes, as this seems like an easy fix we should add to this PR. Otherwise, we risk that this will never be fixed once this PR has been merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure adding this operator is needed.
"Hello"u8 + " "u8 + "World!"u8;
is actually a compile time constant so the compiler concatenate the strings and does not compile any expression. See the actual result:internal static readonly __StaticArrayInitTypeSize=13 5A09E8FA9C77807B24E99C9CF999DEBFAD8441E269EB960E201F61FC3DE20D5A/* Not supported: data(48 65 6C 6C 6F 20 57 6F 72 6C 64 21 00) */;
An array of 13 bytes. This is seen as a single string here. As such, it does not really make sense to mutate sub parts.
Hello
is killed by a failed test, that very same test will kill removing the space orworld!
. So there is no benefit keeping each mutation. Note that this remarks is also valid for classical constant string concatenations.using
statement at the start of any files needing it; meaning it would have to be added everywhere. Furthermore, it could result in compilation errors (ambiguous call
) that would require a new rollback logic (to detect it and remove the unnecessaryusing
statement). It does not appear easy to me.TLDR;
I recommend: doing nothing for now and contemplate detecting concatenated constant strings (u8 or otherwise) to be mutated at once. But it requires specific logic within the
ExpressionOrchestrator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with utf8 strings and can be removed