Parenthesize interpolations containing global::

[ds5678]

Resolves #3447

Solution

• Any comments on the approach taken, its consistency with surrounding code, etc.
  • As recommended, I implemented the fix in the InsertParenthesesVisitor class.
• Which part of this PR is most in need of attention/improvement?
  • I used recursion to check if any child has global::. Is there a better (more performant) way to check?
• At least one test covering the code changed

[siegfriedpammer]

Ad recursion:

You can avoid stack overflows by using AstNode.Descendants, see

ILSpy/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs

Lines 291 to 337 in 03b7444

	public IEnumerable<AstNode> Descendants {
	get { return GetDescendantsImpl(false); }
	}
	/// <summary>
	/// Gets all descendants of this node (including this node itself) in pre-order.
	/// </summary>
	public IEnumerable<AstNode> DescendantsAndSelf {
	get { return GetDescendantsImpl(true); }
	}
	public IEnumerable<AstNode> DescendantNodes(Func<AstNode, bool>? descendIntoChildren = null)
	{
	return GetDescendantsImpl(false, descendIntoChildren);
	}
	public IEnumerable<AstNode> DescendantNodesAndSelf(Func<AstNode, bool>? descendIntoChildren = null)
	{
	return GetDescendantsImpl(true, descendIntoChildren);
	}
	IEnumerable<AstNode> GetDescendantsImpl(bool includeSelf, Func<AstNode, bool>? descendIntoChildren = null)
	{
	if (includeSelf)
	{
	yield return this;
	if (descendIntoChildren != null && !descendIntoChildren(this))
	yield break;
	}
	Stack<AstNode?> nextStack = new Stack<AstNode?>();
	nextStack.Push(null);
	AstNode? pos = firstChild;
	while (pos != null)
	{
	// Remember next before yielding pos.
	// This allows removing/replacing nodes while iterating through the list.
	if (pos.nextSibling != null)
	nextStack.Push(pos.nextSibling);
	yield return pos;
	if (pos.firstChild != null && (descendIntoChildren == null || descendIntoChildren(pos)))
	pos = pos.firstChild;
	else
	pos = nextStack.Pop();
	}
	}

[siegfriedpammer]

Note to self: do not review pull requests when not entirely awake :(

sorry, I gave wrong feedback. We cannot prevent stack overflows in this case, because the whole thing is using the Visitor pattern. So the problem remains and the performance might even be worse due to the use of LINQ.

So, is there actually anything we could do to limit the number of nodes to check? Can we, for example, stop at anonymous functions/lambdas? Do we still need to disambiguate inside invocation argument lists? Might we be able to just look at the top-level expression?

This is fine according to SharpLab...

    public string M(string a) {
        return $"Test: {M(global::System.Environment.CurrentDirectory + a)}";
    }
    public string M() {
        return $"Test: {delegate(string y) { return global::System.Environment.CurrentDirectory.Length > 0; }}";
    }

So I guess, we can stop descending as soon as we find:

• Parentheses
• An argument
• An anonymous method/lambda expression

What do you think?

[ds5678]

I implemented what you suggested. However, I encountered another issue that's preventing my tests from passing.

[siegfriedpammer]

While fixing #3464 I noticed that public static string Lambda1 => $"Prefix {() => global::System.DateTime.Now} suffix"; doesn't require a cast, but still requires parentheses.

[ds5678]

While fixing #3464 I noticed that public static string Lambda1 => $"Prefix {() => global::System.DateTime.Now} suffix"; doesn't require a cast, but still requires parentheses.

This is a recent change caused by you fixing #3464. Previously, the decompiler would emit the cast always. I added a pragma directive in the unit test to choose the appropriate format based on framework and C# version.

[ds5678]

Also, I notice that you seem to normal merge all pull requests, even though mine almost always have a few "junk" commits. Is there a reason you don't squash merge?

[siegfriedpammer]

This is a recent change caused by you fixing #3464.

Yes, my comment was imprecise: While playing around with your test case (in order to decide whether I can pass allowImplicitConversion: true to ConvertTo), I noticed that Visual Studio highlights the cast to Func<...> as unnecessary, so I tried removing it and it seemed to work, but the parenthesis was no longer optional, if the body of the lambda was just an expression containing global::.

Also, I notice that you seem to normal merge all pull requests, even though mine almost always have a few "junk" commits. Is there a reason you don't squash merge?

I usually organize my commits into smaller self-contained units/changes to make bisecting after bug reports easier. If a PR has somewhat organized commits I usually just do a normal merge, however, if there is junk in there, I do squash-merges too. It all the depends on the person creating the PR.

[ds5678]

Is there something else I need to do for this pull request to be merged?

[siegfriedpammer]

Is there something else I need to do for this pull request to be merged?

No, I just wasn't sure whether you are done yet.

[siegfriedpammer]

Thank you very much for your continued contributions!

[ds5678]

No problem. I appreciate your reviews.