From 2f13d9f77ab67a0695e0b5209fef146cecf41983 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Mon, 20 Apr 2026 20:33:17 -0500 Subject: [PATCH 1/2] Fix PrettyPrint corruption when user-definition header has a trailing // comment PrettyPrintVisitor.FormatUserDefinitions joined the trimmed declaration with the assignment operator using a single-space separator. When the original source placed a // line comment on the header line and the '=' body on the next line, Trim() stripped the newline that terminated the comment, and the subsequent " = " pushed the operator inside the // comment, corrupting the output so it could no longer be parsed back. Detect when the trimmed declaration ends inside an unterminated // line comment (tracking /*..*/ nesting so a // inside a block comment is ignored) and, in that case, insert a newline + 4-space indent before the operator. Applied uniformly to NamedFormula, UDF, and DefinedType branches. Adds two xUnit InlineData cases in TestUserDefinitionsPrettyPrint: the exact repro from the issue and a mixed /*..*/ + // UDF header that covers the generalization named in the issue title. Issue: https://github.com/microsoft/Power-Fx/issues/2998 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Syntax/TexlPretty.cs | 117 +++++++++++++----- .../FormatterTests.cs | 10 ++ 2 files changed, 93 insertions(+), 34 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/TexlPretty.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/TexlPretty.cs index 1ec2333f90..565eadf8e2 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Syntax/TexlPretty.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Syntax/TexlPretty.cs @@ -66,19 +66,19 @@ public override LazyList Visit(NumLitNode node, Precedence parentPrecede { Contracts.AssertValue(node); - var nlt = node.Value; - - // $$$ can't use current culture + var nlt = node.Value; + + // $$$ can't use current culture return LazyList.Of(nlt != null ? nlt.ToString() : node.NumValue.ToString("R", CultureInfo.CurrentCulture)); - } - + } + public override LazyList Visit(DecLitNode node, Precedence parentPrecedence) { Contracts.AssertValue(node); - var nlt = node.Value; - - // $$$ can't use current culture + var nlt = node.Value; + + // $$$ can't use current culture return LazyList.Of(nlt != null ? nlt.ToString() : node.DecValue.ToString("G29", CultureInfo.CurrentCulture)); } @@ -250,7 +250,7 @@ public override LazyList Visit(VariadicOpNode node, Precedence parentPre switch (node.Op) { - case VariadicOp.Chain: + case VariadicOp.Chain: // $$$ can't use current culture var op = SpacedOper(TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorChainingSeparator); var count = node.Count; @@ -261,7 +261,7 @@ public override LazyList Visit(VariadicOpNode node, Precedence parentPre result = result .With(node.Children[i].Accept(this, Precedence.None)); if (i != count - 1) - { + { // $$$ can't use current culture result = result.With(SpacedOper(TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorChainingSeparator)); } @@ -330,7 +330,7 @@ public override LazyList Visit(CallNode node, Precedence parentPrecedenc public override LazyList Visit(ListNode node, Precedence parentPrecedence) { Contracts.AssertValue(node); - + // $$$ can't use current culture var listSep = TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorListSeparator + " "; var result = LazyList.Empty; @@ -350,7 +350,7 @@ public override LazyList Visit(ListNode node, Precedence parentPrecedenc public override LazyList Visit(RecordNode node, Precedence parentPrecedence) { Contracts.AssertValue(node); - + // $$$ can't use current culture var listSep = TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorListSeparator + " "; var result = LazyList.Empty; @@ -382,9 +382,9 @@ public override LazyList Visit(RecordNode node, Precedence parentPrecede public override LazyList Visit(TableNode node, Precedence parentPrecedence) { - Contracts.AssertValue(node); - - // $$$ can't use current culture + Contracts.AssertValue(node); + + // $$$ can't use current culture var listSep = TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorListSeparator + " "; var result = LazyList.Empty; for (var i = 0; i < node.Children.Count; ++i) @@ -399,10 +399,10 @@ public override LazyList Visit(TableNode node, Precedence parentPreceden result = LazyList.Of(TexlLexer.PunctuatorBracketOpen, " ") .With(result) .With(" ", TexlLexer.PunctuatorBracketClose); - + return result; - } - + } + public override LazyList Visit(TypeLiteralNode node, Precedence parentPrecedence) { Contracts.AssertValue(node); @@ -506,8 +506,8 @@ public static string Format(TexlNode node, SourceList before, SourceList after, .Replace("\n\n", "\n"); return new Regex(@"\n +(\n +)").Replace(preRegex, (Match match) => match.Groups[1].Value); - } - + } + public static string FormatUserDefinitions(ParseUserDefinitionResult result, string formulasScript) { Contracts.AssertValue(result); @@ -518,6 +518,9 @@ public static string FormatUserDefinitions(ParseUserDefinitionResult result, str foreach (var info in result.UserDefinitionSourceInfos) { var declaration = info.Declaration.Trim(); + + // If a // line comment is the last thing in the declaration, Trim() ate the newline that terminated it — emit a line break before '=' so the operator doesn't land inside the comment. + var preAssignSeparator = EndsInsideLineComment(declaration) ? "\n " : " "; var name = info.Name; var before = info.Before; var after = info.After; @@ -526,7 +529,7 @@ public static string FormatUserDefinitions(ParseUserDefinitionResult result, str case UserDefinitionType.NamedFormula: var nf = result.NamedFormulas.First(nf => nf.Ident == name); - definitions.Add(declaration + $" {(nf.ColonEqual ? TexlLexer.PunctuatorColonEqual : TexlLexer.PunctuatorEqual)} " + string.Concat(visitor.CommentsOf(before).With(nf.Formula.ParseTree.Accept(visitor, new Context(0)).With(visitor.CommentsOf(after))))); + definitions.Add(declaration + $"{preAssignSeparator}{(nf.ColonEqual ? TexlLexer.PunctuatorColonEqual : TexlLexer.PunctuatorEqual)} " + string.Concat(visitor.CommentsOf(before).With(nf.Formula.ParseTree.Accept(visitor, new Context(0)).With(visitor.CommentsOf(after))))); break; case UserDefinitionType.UDF: var udf = result.UDFs.First(udf => udf.Ident == name); @@ -535,22 +538,68 @@ public static string FormatUserDefinitions(ParseUserDefinitionResult result, str $"\n{{\n\t{string.Concat(visitor.CommentsOf(before).With(udf.Body.Accept(visitor, new Context(1)).With(visitor.CommentsOf(after)))).Trim()}\n}}" : string.Concat(visitor.CommentsOf(before).With(udf.Body.Accept(visitor, new Context(0)).With(visitor.CommentsOf(after)))); - definitions.Add(declaration + $" {TexlLexer.PunctuatorEqual} " + udfBody); + definitions.Add(declaration + $"{preAssignSeparator}{TexlLexer.PunctuatorEqual} " + udfBody); break; case UserDefinitionType.DefinedType: var type = result.DefinedTypes.First(type => type.Ident == name); - definitions.Add(declaration + $" {TexlLexer.PunctuatorColonEqual} " + string.Concat(visitor.CommentsOf(before).With(type.Type.Accept(visitor, new Context(0))).With(visitor.CommentsOf(after)))); + definitions.Add(declaration + $"{preAssignSeparator}{TexlLexer.PunctuatorColonEqual} " + string.Concat(visitor.CommentsOf(before).With(type.Type.Accept(visitor, new Context(0))).With(visitor.CommentsOf(after)))); break; default: continue; } } - return string.Join($"{TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorChainingSeparator}\n", definitions) + + return string.Join($"{TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorChainingSeparator}\n", definitions) + TexlLexer.GetLocalizedInstance(CultureInfo.CurrentCulture).LocalizedPunctuatorChainingSeparator; } + private static bool EndsInsideLineComment(string text) + { + var inLineComment = false; + var inBlockComment = false; + for (var i = 0; i < text.Length; i++) + { + var c = text[i]; + if (inLineComment) + { + if (c == '\n') + { + inLineComment = false; + } + + continue; + } + + if (inBlockComment) + { + if (c == '*' && i + 1 < text.Length && text[i + 1] == '/') + { + inBlockComment = false; + i++; + } + + continue; + } + + if (c == '/' && i + 1 < text.Length) + { + if (text[i + 1] == '/') + { + inLineComment = true; + i++; + } + else if (text[i + 1] == '*') + { + inBlockComment = true; + i++; + } + } + } + + return inLineComment; + } + private LazyList CommentsOf(SourceList list) { if (list == null) @@ -627,8 +676,8 @@ public override LazyList Visit(NumLitNode node, Context context) Contracts.AssertValue(node); return Single(node); - } - + } + public override LazyList Visit(DecLitNode node, Context context) { Contracts.AssertValue(node); @@ -750,16 +799,16 @@ public override LazyList Visit(VariadicOpNode node, Context context) .With(GetNewLine(context.IndentDepth + 1)); previousWasNewLine = true; } - else if (source is TokenSource tokenSource2 && + else if (source is TokenSource tokenSource2 && tokenSource2.Token is CommentToken commentToken) - { + { var singleLineComment = commentToken.Value.Trim().StartsWith("//", StringComparison.Ordinal); result = result - .With(GetScriptForToken(commentToken).Trim()); - if (singleLineComment) - { + .With(GetScriptForToken(commentToken).Trim()); + if (singleLineComment) + { result = result.With(GetNewLine(context.IndentDepth + 1)); - previousWasNewLine = true; + previousWasNewLine = true; } } else @@ -1007,8 +1056,8 @@ public override LazyList Visit(AsNode node, Context context) Contracts.AssertValue(node); return Basic(node, context); - } - + } + public override LazyList Visit(TypeLiteralNode node, Context context) { Contracts.AssertValue(node); diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs index ba7bed37e5..4a2098463c 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs @@ -324,6 +324,16 @@ public void TestPrettyPrint(string script, string expected) [InlineData( "// Math\nAdd(x:Number,y:Number):Number= /*c1*/ x+y;\n// Trig\nCosine(x:Number):Number=Cos(x);\n", "// Math\nAdd(x:Number,y:Number):Number = /*c1*/x + y;\n// Trig\nCosine(x:Number):Number = Cos(x);")] + + // Trailing // line comment on the header line must not be merged with the '=' body (issue #2998). + [InlineData( + "MyNF // My Named Formula\n = Pi()/2;", + "MyNF // My Named Formula\n = Pi() / 2;")] + + // Mixing /*..*/ block and // line comments in a UDF declaration must also preserve the line break. + [InlineData( + "Add(x:Number,y:Number):Number /* rt */ // trailing\n = x+y;", + "Add(x:Number,y:Number):Number /* rt */ // trailing\n = x + y;")] public void TestUserDefinitionsPrettyPrint(string script, string expected) { var parserOptions = new ParserOptions() From 9f775b761fd15dd7fc7ac888f1ab6832a150d927 Mon Sep 17 00:00:00 2001 From: Anderson Silva Date: Wed, 22 Apr 2026 11:40:47 -0500 Subject: [PATCH 2/2] Update print statement from 'Hello' to 'Goodbye' --- src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs index b844066f80..a60d7ae3ee 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/FormatterTests.cs @@ -334,6 +334,7 @@ public void TestPrettyPrint(string script, string expected) [InlineData( "Add(x:Number,y:Number):Number /* rt */ // trailing\n = x+y;", "Add(x:Number,y:Number):Number /* rt */ // trailing\n = x + y;")] + // Issue #2997: trailing comment on the last user definition must be preserved. [InlineData("MyNF=Pi()/2; // Half PI", "MyNF = Pi() / 2; // Half PI")] [InlineData("MyNF=Pi()/2; /* Half PI */", "MyNF = Pi() / 2; /* Half PI */")]