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.

Sign up for GitHub

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

Jump to bottom

breaking a string on many lines produce incorrect result if followed by a call #96948

Merged
merged 2 commits into from
Feb 14, 2024
Merged

breaking a string on many lines produce incorrect result if followed by a call #96948

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08b078b
breaking a string on many lines produce incorrect result if followed …
pedrobsaila Jan 13, 2024
2e97fef
fix failing test
pedrobsaila Jan 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/libraries/System.CodeDom/src/Microsoft/CSharp/CSharpCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ private string QuoteSnippetStringCStyle(string value)

b.Append('\"');

bool isStringMultiline = false;
int i = 0;
while (i < value.Length)
{
Expand Down Expand Up @@ -148,12 +149,23 @@ private string QuoteSnippetStringCStyle(string value)
b.Append(Environment.NewLine);
b.Append(indentObj.IndentationString);
b.Append('\"');

if (!isStringMultiline)
danmoseley marked this conversation as resolved.
Show resolved Hide resolved
{
isStringMultiline = true;
}
}
++i;
}

b.Append('\"');

if (isStringMultiline)
{
b.Insert(0, '(');
b.Append(')');
Comment on lines +164 to +165
Copy link
Member

Choose a reason for hiding this comment

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

@pedrobsaila I am not expert here and do not understand how wrapping in a bracket would help, could you explain or is there a doc/reference I could check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bracket forces the next function expression to be evaluated on the whole string instead of just the last substring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the easy road by doing this, I could :

  • improve it by doing a sort of lookup so i add the bracket to a splitted string only if it is used as argument to the next function expression but the code does not pass context information to the process of code generation
  • or just stop splitting strings

If you see better solutions, let me know

Copy link
Member

Choose a reason for hiding this comment

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

I see, thank you for explanation, I was expecting we just stop splitting strings, but I guess that is better for styling. I have no better suggestion, the fix LGTM.

I see @steveharter mentioned in the issue string concat using + when it shouldn't be doing that, not sure what else we could use other than +, but what he might have a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

I think the bracket approach is fine.

}

return b.ToString();
}

Expand Down
48 changes: 39 additions & 9 deletions src/libraries/System.CodeDom/tests/System/CodeDom/Compiler/CSharpCodeGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,11 +539,11 @@ public static IEnumerable<object[]> GenerateCodeFromExpression_TestData()
yield return new object[] { new CodePrimitiveExpression("\uDC00"), null, "\"\uDC00\"" };
yield return new object[] { new CodePrimitiveExpression("\uD800"), null, "\"\uD800\"" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789"), null, $"\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\"" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800"), null, $"\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\" +{nl} \"\"" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\uDC00"), null, $"\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\uDC00\" +{nl} \"\"" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800a"), null, $"\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\" +{nl} \"a\"" };
yield return new object[] { new CodePrimitiveExpression("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"), null, $"\"012345678901234567890123456789012345678901234567890123456789012345678901234567890\" +{nl} \"123456789\"" };
yield return new object[] { new CodePrimitiveExpression("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"), customOptions, $"\"012345678901234567890123456789012345678901234567890123456789012345678901234567890\" +{nl}$\"123456789\"" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800"), null, $"(\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\" +{nl} \"\")" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\uDC00"), null, $"(\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\uDC00\" +{nl} \"\")" };
yield return new object[] { new CodePrimitiveExpression("01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800a"), null, $"(\"01234567890123456789012345678901234567890123456789012345678901234567890123456789\uD800\" +{nl} \"a\")" };
yield return new object[] { new CodePrimitiveExpression("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"), null, $"(\"012345678901234567890123456789012345678901234567890123456789012345678901234567890\" +{nl} \"123456789\")" };
yield return new object[] { new CodePrimitiveExpression("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"), customOptions, $"(\"012345678901234567890123456789012345678901234567890123456789012345678901234567890\" +{nl}$\"123456789\")" };
yield return new object[] { new CodePrimitiveExpression(new string('a', 256)), null, $"@\"{new string('a', 256)}\"" };
yield return new object[] { new CodePrimitiveExpression("\"" + new string('a', 254) + "\""), null, $"@\"\"\"{new string('a', 254)}\"\"\"" };
yield return new object[] { new CodePrimitiveExpression("\"" + new string('a', 1498) + "\""), null, $"@\"\"\"{new string('a', 1498)}\"\"\"" };
Expand Down Expand Up @@ -918,7 +918,7 @@ public static IEnumerable<object[]> GenerateCodeFromStatement_TestData()
new CodeConditionStatement(
new CodePrimitiveExpression(1),
new CodeExpressionStatement(new CodePrimitiveExpression(new string('a', 82)))
), null, $"if (1) {{{nl} \"{new string('a', 81)}\" +{nl} \"a\";{nl}}}{nl}"
), null, $"if (1) {{{nl} (\"{new string('a', 81)}\" +{nl} \"a\");{nl}}}{nl}"
};
yield return new object[]
{
Expand All @@ -928,7 +928,7 @@ public static IEnumerable<object[]> GenerateCodeFromStatement_TestData()
new CodePrimitiveExpression(2),
new CodeExpressionStatement(new CodePrimitiveExpression(new string('a', 82)))
)
), null, $"if (1) {{{nl} if (2) {{{nl} \"{new string('a', 81)}\" +{nl} \"a\";{nl} }}{nl}}}{nl}"
), null, $"if (1) {{{nl} if (2) {{{nl} (\"{new string('a', 81)}\" +{nl} \"a\");{nl} }}{nl}}}{nl}"
};
yield return new object[]
{
Expand All @@ -941,7 +941,7 @@ public static IEnumerable<object[]> GenerateCodeFromStatement_TestData()
new CodeExpressionStatement(new CodePrimitiveExpression(new string('a', 82)))
)
)
), null, $"if (1) {{{nl} if (2) {{{nl} if (3) {{{nl} \"{new string('a', 81)}\" +{nl} \"a\";{nl} }}{nl} }}{nl}}}{nl}"
), null, $"if (1) {{{nl} if (2) {{{nl} if (3) {{{nl} (\"{new string('a', 81)}\" +{nl} \"a\");{nl} }}{nl} }}{nl}}}{nl}"
};
yield return new object[]
{
Expand All @@ -957,7 +957,7 @@ public static IEnumerable<object[]> GenerateCodeFromStatement_TestData()
)
)
)
), null, $"if (1) {{{nl} if (2) {{{nl} if (3) {{{nl} if (4) {{{nl} \"{new string('a', 81)}\" +{nl} \"a\";{nl} }}{nl} }}{nl} }}{nl}}}{nl}"
), null, $"if (1) {{{nl} if (2) {{{nl} if (3) {{{nl} if (4) {{{nl} (\"{new string('a', 81)}\" +{nl} \"a\");{nl} }}{nl} }}{nl} }}{nl}}}{nl}"
};

yield return new object[]
Expand Down Expand Up @@ -1308,6 +1308,7 @@ public static IEnumerable<object[]> GenerateCodeFromStatement_TestData()

[Theory]
[MemberData(nameof(GenerateCodeFromStatement_TestData))]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET Framework has different string breakup handling")]
public void GenerateCodeFromStatement_Invoke_Success(CodeStatement e, CodeGeneratorOptions o, string expected)
{
ICodeGenerator generator = GetGenerator();
Expand Down Expand Up @@ -2708,6 +2709,35 @@ public void ValidateIdentifier_InvokeInvalid_ThrowsArgumentException(string valu
AssertExtensions.Throws<ArgumentException>("value", null, () => generator.ValidateIdentifier(value));
}

[Fact]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET Framework has different string breakup handling")]
public void LineBreaksShouldPreserveTheWholeStringAsOneValue()
{
CodeStatement e = new CodeAssignStatement(
new CodeFieldReferenceExpression
{
FieldName = "Value",
TargetObject = new CodeTypeReferenceExpression("PF")
},
new CodeMethodInvokeExpression
{
Parameters =
{
new CodePrimitiveExpression('|')
},
Method = new CodeMethodReferenceExpression
{
MethodName = "MethodName",
TargetObject = new CodePrimitiveExpression(new string('*', 82))
}
}
);
ICodeGenerator generator = GetGenerator();
var writer = new StringWriter();
generator.GenerateCodeFromStatement(e, writer, new CodeGeneratorOptions());
AssertEqualLong("PF.Value = (\"*********************************************************************************\" +" + writer.NewLine + " \"*\").MethodName('|');" + writer.NewLine, writer.ToString());
}

private static ICodeGenerator GetGenerator()
{
#pragma warning disable 0618
Expand Down
Loading