From 03787bfc700d8ef7b784198b80fa23ca3ad9d9cb Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Wed, 23 Sep 2020 18:44:31 +0200 Subject: [PATCH] Avoid parentheses around lambdas where possible. --- .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../Output/InsertParenthesesVisitorTests.cs | 488 ++++++++++++++++++ .../TestCases/Pretty/DelegateConstruction.cs | 16 +- .../TestCases/Pretty/ExpressionTrees.cs | 6 +- .../TestCases/Pretty/FixProxyCalls.cs | 4 +- .../OutputVisitor/InsertParenthesesVisitor.cs | 34 +- 6 files changed, 525 insertions(+), 24 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/Output/InsertParenthesesVisitorTests.cs diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 45335d194..4345dfb83 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -91,6 +91,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/Output/InsertParenthesesVisitorTests.cs b/ICSharpCode.Decompiler.Tests/Output/InsertParenthesesVisitorTests.cs new file mode 100644 index 000000000..f9bb5ddef --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/Output/InsertParenthesesVisitorTests.cs @@ -0,0 +1,488 @@ +// Copyright (c) 2010-2013 AlphaSierraPapa for the SharpDevelop Team +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System.IO; + +using ICSharpCode.Decompiler.CSharp.OutputVisitor; +using ICSharpCode.Decompiler.CSharp.Syntax; + +using NUnit.Framework; + +namespace ICSharpCode.Decompiler.Tests.Output +{ + [TestFixture] + public class InsertParenthesesVisitorTests + { + CSharpFormattingOptions policy; + + [SetUp] + public void SetUp() + { + policy = FormattingOptionsFactory.CreateMono(); + } + + string InsertReadable(Expression expr) + { + expr = expr.Clone(); + expr.AcceptVisitor(new InsertParenthesesVisitor { InsertParenthesesForReadability = true }); + StringWriter w = new StringWriter(); + w.NewLine = " "; + expr.AcceptVisitor(new CSharpOutputVisitor(new TextWriterTokenWriter(w) { IndentationString = "" }, policy)); + return w.ToString(); + } + + string InsertRequired(Expression expr) + { + expr = expr.Clone(); + expr.AcceptVisitor(new InsertParenthesesVisitor { InsertParenthesesForReadability = false }); + StringWriter w = new StringWriter(); + w.NewLine = " "; + expr.AcceptVisitor(new CSharpOutputVisitor(new TextWriterTokenWriter(w) { IndentationString = "" }, policy)); + return w.ToString(); + } + + [Test] + public void EqualityInAssignment() + { + Expression expr = new AssignmentExpression( + new IdentifierExpression("cond"), + new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.Equality, + new IdentifierExpression("b") + ) + ); + + Assert.AreEqual("cond = a == b", InsertRequired(expr)); + Assert.AreEqual("cond = a == b", InsertReadable(expr)); + } + + [Test] + public void LambdaInAssignment() + { + Expression expr = new AssignmentExpression( + new IdentifierExpression("p"), + new LambdaExpression { + Body = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.Add, + new IdentifierExpression("b") + ) + } + ); + + Assert.AreEqual("p = () => a + b", InsertRequired(expr)); + Assert.AreEqual("p = () => a + b", InsertReadable(expr)); + } + + [Test] + public void LambdaInDelegateAdditionRHS() + { + Expression expr = new BinaryOperatorExpression { + Left = new IdentifierExpression("p"), + Operator = BinaryOperatorType.Add, + Right = new LambdaExpression { + Body = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.Add, + new IdentifierExpression("b") + ) + } + }; + + Assert.AreEqual("p + () => a + b", InsertRequired(expr)); + Assert.AreEqual("p + (() => a + b)", InsertReadable(expr)); + } + + [Test] + public void LambdaInDelegateAdditionLHS() + { + Expression expr = new BinaryOperatorExpression { + Left = new LambdaExpression { + Body = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.Add, + new IdentifierExpression("b") + ) + }, + Operator = BinaryOperatorType.Add, + Right = new IdentifierExpression("p"), + }; + + Assert.AreEqual("(() => a + b) + p", InsertRequired(expr)); + Assert.AreEqual("(() => a + b) + p", InsertReadable(expr)); + } + + [Test] + public void TrickyCast1() + { + Expression expr = new CastExpression { + Type = new PrimitiveType("int"), + Expression = new UnaryOperatorExpression( + UnaryOperatorType.Minus, new IdentifierExpression("a") + ) + }; + + Assert.AreEqual("(int)-a", InsertRequired(expr)); + Assert.AreEqual("(int)(-a)", InsertReadable(expr)); + } + + [Test] + public void TrickyCast2() + { + Expression expr = new CastExpression { + Type = new SimpleType("MyType"), + Expression = new UnaryOperatorExpression( + UnaryOperatorType.Minus, new IdentifierExpression("a") + ) + }; + + Assert.AreEqual("(MyType)(-a)", InsertRequired(expr)); + Assert.AreEqual("(MyType)(-a)", InsertReadable(expr)); + } + + [Test] + public void TrickyCast3() + { + Expression expr = new CastExpression { + Type = new SimpleType("MyType"), + Expression = new UnaryOperatorExpression( + UnaryOperatorType.Not, new IdentifierExpression("a") + ) + }; + + Assert.AreEqual("(MyType)!a", InsertRequired(expr)); + Assert.AreEqual("(MyType)(!a)", InsertReadable(expr)); + } + + [Test] + public void TrickyCast4() + { + Expression expr = new CastExpression { + Type = new SimpleType("MyType"), + Expression = new PrimitiveExpression(int.MinValue), + }; + + Assert.AreEqual("(MyType)(-2147483648)", InsertRequired(expr)); + Assert.AreEqual("(MyType)(-2147483648)", InsertReadable(expr)); + } + + [Test] + public void TrickyCast5() + { + Expression expr = new CastExpression { + Type = new SimpleType("MyType"), + Expression = new PrimitiveExpression(-1.0), + }; + + Assert.AreEqual("(MyType)(-1.0)", InsertRequired(expr)); + Assert.AreEqual("(MyType)(-1.0)", InsertReadable(expr)); + } + + [Test] + public void TrickyCast6() + { + Expression expr = new CastExpression { + Type = new PrimitiveType("double"), + Expression = new PrimitiveExpression(int.MinValue), + }; + + Assert.AreEqual("(double)-2147483648", InsertRequired(expr)); + Assert.AreEqual("(double)(-2147483648)", InsertReadable(expr)); + } + + [Test] + public void CastAndInvoke() + { + Expression expr = new MemberReferenceExpression { + Target = new CastExpression { + Type = new PrimitiveType("string"), + Expression = new IdentifierExpression("a") + }, + MemberName = "Length" + }; + + Assert.AreEqual("((string)a).Length", InsertRequired(expr)); + Assert.AreEqual("((string)a).Length", InsertReadable(expr)); + } + + [Test] + public void DoubleNegation() + { + Expression expr = new UnaryOperatorExpression( + UnaryOperatorType.Minus, + new UnaryOperatorExpression(UnaryOperatorType.Minus, new IdentifierExpression("a")) + ); + + Assert.AreEqual("- -a", InsertRequired(expr)); + Assert.AreEqual("-(-a)", InsertReadable(expr)); + } + + [Test] + public void AdditionWithConditional() + { + Expression expr = new BinaryOperatorExpression { + Left = new IdentifierExpression("a"), + Operator = BinaryOperatorType.Add, + Right = new ConditionalExpression { + Condition = new BinaryOperatorExpression { + Left = new IdentifierExpression("b"), + Operator = BinaryOperatorType.Equality, + Right = new PrimitiveExpression(null) + }, + TrueExpression = new IdentifierExpression("c"), + FalseExpression = new IdentifierExpression("d") + } + }; + + Assert.AreEqual("a + (b == null ? c : d)", InsertRequired(expr)); + Assert.AreEqual("a + ((b == null) ? c : d)", InsertReadable(expr)); + } + + [Test] + public void TypeTestInConditional() + { + Expression expr = new ConditionalExpression { + Condition = new IsExpression { + Expression = new IdentifierExpression("a"), + Type = new ComposedType { + BaseType = new PrimitiveType("int"), + HasNullableSpecifier = true + } + }, + TrueExpression = new IdentifierExpression("b"), + FalseExpression = new IdentifierExpression("c") + }; + + Assert.AreEqual("a is int? ? b : c", InsertRequired(expr)); + Assert.AreEqual("(a is int?) ? b : c", InsertReadable(expr)); + + policy.SpaceBeforeConditionalOperatorCondition = false; + policy.SpaceAfterConditionalOperatorCondition = false; + policy.SpaceBeforeConditionalOperatorSeparator = false; + policy.SpaceAfterConditionalOperatorSeparator = false; + + Assert.AreEqual("a is int? ?b:c", InsertRequired(expr)); + Assert.AreEqual("(a is int?)?b:c", InsertReadable(expr)); + } + + [Test] + public void MethodCallOnQueryExpression() + { + Expression expr = new InvocationExpression(new MemberReferenceExpression { + Target = new QueryExpression { + Clauses = { + new QueryFromClause { + Identifier = "a", + Expression = new IdentifierExpression("b") + }, + new QuerySelectClause { + Expression = new InvocationExpression(new MemberReferenceExpression { + Target = new IdentifierExpression("a"), + MemberName = "c" + }) + } + } + }, + MemberName = "ToArray" + }); + + Assert.AreEqual("(from a in b select a.c ()).ToArray ()", InsertRequired(expr)); + Assert.AreEqual("(from a in b select a.c ()).ToArray ()", InsertReadable(expr)); + } + + [Test] + public void SumOfQueries() + { + QueryExpression query = new QueryExpression { + Clauses = { + new QueryFromClause { + Identifier = "a", + Expression = new IdentifierExpression("b") + }, + new QuerySelectClause { + Expression = new IdentifierExpression("a") + } + } + }; + Expression expr = new BinaryOperatorExpression( + query, + BinaryOperatorType.Add, + query.Clone() + ); + + Assert.AreEqual("(from a in b select a) + " + + "from a in b select a", InsertRequired(expr)); + Assert.AreEqual("(from a in b select a) + " + + "(from a in b select a)", InsertReadable(expr)); + } + + [Test] + public void QueryInTypeTest() + { + Expression expr = new IsExpression { + Expression = new QueryExpression { + Clauses = { + new QueryFromClause { + Identifier = "a", + Expression = new IdentifierExpression("b") + }, + new QuerySelectClause { + Expression = new IdentifierExpression("a") + } + } + }, + Type = new PrimitiveType("int") + }; + + Assert.AreEqual("(from a in b select a) is int", InsertRequired(expr)); + Assert.AreEqual("(from a in b select a) is int", InsertReadable(expr)); + } + + [Test] + public void PrePost() + { + Expression expr = new UnaryOperatorExpression( + UnaryOperatorType.Increment, + new UnaryOperatorExpression( + UnaryOperatorType.PostIncrement, + new IdentifierExpression("a") + ) + ); + + Assert.AreEqual("++a++", InsertRequired(expr)); + Assert.AreEqual("++(a++)", InsertReadable(expr)); + } + + [Test] + public void PostPre() + { + Expression expr = new UnaryOperatorExpression( + UnaryOperatorType.PostIncrement, + new UnaryOperatorExpression( + UnaryOperatorType.Increment, + new IdentifierExpression("a") + ) + ); + + Assert.AreEqual("(++a)++", InsertRequired(expr)); + Assert.AreEqual("(++a)++", InsertReadable(expr)); + } + + [Test] + public void Logical1() + { + Expression expr = new BinaryOperatorExpression( + new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.ConditionalAnd, + new IdentifierExpression("b") + ), + BinaryOperatorType.ConditionalAnd, + new IdentifierExpression("c") + ); + + Assert.AreEqual("a && b && c", InsertRequired(expr)); + Assert.AreEqual("a && b && c", InsertReadable(expr)); + } + + [Test] + public void Logical2() + { + Expression expr = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.ConditionalAnd, + new BinaryOperatorExpression( + new IdentifierExpression("b"), + BinaryOperatorType.ConditionalAnd, + new IdentifierExpression("c") + ) + ); + + Assert.AreEqual("a && (b && c)", InsertRequired(expr)); + Assert.AreEqual("a && (b && c)", InsertReadable(expr)); + } + + [Test] + public void Logical3() + { + Expression expr = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.ConditionalOr, + new BinaryOperatorExpression( + new IdentifierExpression("b"), + BinaryOperatorType.ConditionalAnd, + new IdentifierExpression("c") + ) + ); + + Assert.AreEqual("a || b && c", InsertRequired(expr)); + Assert.AreEqual("a || (b && c)", InsertReadable(expr)); + } + + [Test] + public void Logical4() + { + Expression expr = new BinaryOperatorExpression( + new IdentifierExpression("a"), + BinaryOperatorType.ConditionalAnd, + new BinaryOperatorExpression( + new IdentifierExpression("b"), + BinaryOperatorType.ConditionalOr, + new IdentifierExpression("c") + ) + ); + + Assert.AreEqual("a && (b || c)", InsertRequired(expr)); + Assert.AreEqual("a && (b || c)", InsertReadable(expr)); + } + + [Test] + public void ArrayCreationInIndexer() + { + Expression expr = new IndexerExpression { + Target = new ArrayCreateExpression { + Type = new PrimitiveType("int"), + Arguments = { new PrimitiveExpression(1) } + }, + Arguments = { new PrimitiveExpression(0) } + }; + + Assert.AreEqual("(new int[1]) [0]", InsertRequired(expr)); + Assert.AreEqual("(new int[1]) [0]", InsertReadable(expr)); + } + + [Test] + public void ArrayCreationWithInitializerInIndexer() + { + Expression expr = new IndexerExpression { + Target = new ArrayCreateExpression { + Type = new PrimitiveType("int"), + Arguments = { new PrimitiveExpression(1) }, + Initializer = new ArrayInitializerExpression { + Elements = { new PrimitiveExpression(42) } + } + }, + Arguments = { new PrimitiveExpression(0) } + }; + + Assert.AreEqual("new int[1] { 42 } [0]", InsertRequired(expr)); + Assert.AreEqual("(new int[1] { 42 }) [0]", InsertReadable(expr)); + } + } +} diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs index fdb31a779..f03a062cc 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs @@ -247,14 +247,14 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } - public static Func test0 = ((string a, string b) => string.IsNullOrEmpty(a) || string.IsNullOrEmpty(b)); - public static Func test1 = ((string a, string b) => string.IsNullOrEmpty(a) || !string.IsNullOrEmpty(b)); - public static Func test2 = ((string a, string b) => !string.IsNullOrEmpty(a) || string.IsNullOrEmpty(b)); - public static Func test3 = ((string a, string b) => !string.IsNullOrEmpty(a) || !string.IsNullOrEmpty(b)); - public static Func test4 = ((string a, string b) => string.IsNullOrEmpty(a) && string.IsNullOrEmpty(b)); - public static Func test5 = ((string a, string b) => string.IsNullOrEmpty(a) && !string.IsNullOrEmpty(b)); - public static Func test6 = ((string a, string b) => !string.IsNullOrEmpty(a) && string.IsNullOrEmpty(b)); - public static Func test7 = ((string a, string b) => !string.IsNullOrEmpty(a) && !string.IsNullOrEmpty(b)); + public static Func test0 = (string a, string b) => string.IsNullOrEmpty(a) || string.IsNullOrEmpty(b); + public static Func test1 = (string a, string b) => string.IsNullOrEmpty(a) || !string.IsNullOrEmpty(b); + public static Func test2 = (string a, string b) => !string.IsNullOrEmpty(a) || string.IsNullOrEmpty(b); + public static Func test3 = (string a, string b) => !string.IsNullOrEmpty(a) || !string.IsNullOrEmpty(b); + public static Func test4 = (string a, string b) => string.IsNullOrEmpty(a) && string.IsNullOrEmpty(b); + public static Func test5 = (string a, string b) => string.IsNullOrEmpty(a) && !string.IsNullOrEmpty(b); + public static Func test6 = (string a, string b) => !string.IsNullOrEmpty(a) && string.IsNullOrEmpty(b); + public static Func test7 = (string a, string b) => !string.IsNullOrEmpty(a) && !string.IsNullOrEmpty(b); public static void Test(this string a) { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs index 0ecf7cb8c..7985a917f 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs @@ -607,7 +607,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty 2012 }.All(set.Add)); - Func, bool> sink = ((Func f) => f(null, null)); + Func, bool> sink = (Func f) => f(null, null); ToCode(X(), () => sink(object.Equals)); } @@ -623,7 +623,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty public void NestedLambda() { - Func, int> call = ((Func f) => f()); + Func, int> call = (Func f) => f(); //no params ToCode(X(), () => call(() => 42)); //one param @@ -1072,4 +1072,4 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty return default(DateTime); } } -} \ No newline at end of file +} diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/FixProxyCalls.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/FixProxyCalls.cs index b71edc02d..0675b7778 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/FixProxyCalls.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/FixProxyCalls.cs @@ -57,7 +57,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.ILPretty { protected internal override string Test(string test) { - Func func = ((string a) => base.Test(a)); + Func func = (string a) => base.Test(a); test = string.Join(test, "aa"); return func(test); } @@ -164,4 +164,4 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.ILPretty yield return base.Test(base.Test(0).GetEnumerator().Current).GetEnumerator().Current; } } -} \ No newline at end of file +} diff --git a/ICSharpCode.Decompiler/CSharp/OutputVisitor/InsertParenthesesVisitor.cs b/ICSharpCode.Decompiler/CSharp/OutputVisitor/InsertParenthesesVisitor.cs index 45ff80826..4b706e127 100644 --- a/ICSharpCode.Decompiler/CSharp/OutputVisitor/InsertParenthesesVisitor.cs +++ b/ICSharpCode.Decompiler/CSharp/OutputVisitor/InsertParenthesesVisitor.cs @@ -67,7 +67,7 @@ namespace ICSharpCode.Decompiler.CSharp.OutputVisitor static PrecedenceLevel GetPrecedence(Expression expr) { // Note: the operator precedence table on MSDN is incorrect - if (expr is QueryExpression) + if (expr is QueryExpression || expr is LambdaExpression) { // Not part of the table in the C# spec, but we need to ensure that queries within // primary expressions get parenthesized. @@ -147,14 +147,14 @@ namespace ICSharpCode.Decompiler.CSharp.OutputVisitor throw new NotSupportedException("Invalid value for BinaryOperatorType"); } } + if (expr is SwitchExpression) + return PrecedenceLevel.Switch; if (expr is IsExpression || expr is AsExpression) return PrecedenceLevel.RelationalAndTypeTesting; if (expr is ConditionalExpression || expr is DirectionExpression) return PrecedenceLevel.Conditional; - if (expr is AssignmentExpression || expr is LambdaExpression) + if (expr is AssignmentExpression) return PrecedenceLevel.Assignment; - if (expr is SwitchExpression) - return PrecedenceLevel.Switch; // anything else: primary expression return PrecedenceLevel.Primary; } @@ -456,17 +456,29 @@ namespace ICSharpCode.Decompiler.CSharp.OutputVisitor // However, the end of the query is greedy. So their start sort of has a high precedence, // while their end has a very low precedence. We handle this by checking whether a query is used // as left part of a binary operator, and parenthesize it if required. - if (queryExpression.Role == BinaryOperatorExpression.LeftRole) - Parenthesize(queryExpression); - if (queryExpression.Parent is IsExpression || queryExpression.Parent is AsExpression) - Parenthesize(queryExpression); + HandleLambdaOrQuery(queryExpression); + base.VisitQueryExpression(queryExpression); + } + + public override void VisitLambdaExpression(LambdaExpression lambdaExpression) + { + // Lambdas are greedy in the same way as query expressions. + HandleLambdaOrQuery(lambdaExpression); + base.VisitLambdaExpression(lambdaExpression); + } + + void HandleLambdaOrQuery(Expression expr) + { + if (expr.Role == BinaryOperatorExpression.LeftRole) + Parenthesize(expr); + if (expr.Parent is IsExpression || expr.Parent is AsExpression) + Parenthesize(expr); if (InsertParenthesesForReadability) { // when readability is desired, always parenthesize query expressions within unary or binary operators - if (queryExpression.Parent is UnaryOperatorExpression || queryExpression.Parent is BinaryOperatorExpression) - Parenthesize(queryExpression); + if (expr.Parent is UnaryOperatorExpression || expr.Parent is BinaryOperatorExpression) + Parenthesize(expr); } - base.VisitQueryExpression(queryExpression); } public override void VisitNamedExpression(NamedExpression namedExpression)