Fix bug in xUnit2014 fixer to account for nested functions (xunit/xunit#2870)

[jared-chevalier]

Suggested fix for issue xunit/xunit#2870, for the xUnit2014 fixer.

Please let me know if you have any feedback about the design, implementation, tests, or code style.

Fixing anonymous functions in particular raises some additional complications as compared to local functions and methods, and though my current approach handles some simple cases, it has limitations. Let me know if there are additional edge cases you think I should handle, or if you think another approach would be better. Thanks.

Problem

The fixer only fixed the violating assertion (fixing the name and adding await) and fixed the containing method declaration (converting the return type to a Task and adding the async modifier).

This approach is adequate when the violating assertion is directly inside a method.

However, it is not appropriate when the assertion is inside an anonymous function or local function, nested inside a method. In this case, only the assertion invocation and the outermost method declaration would be fixed, leaving the inner functions unfixed.

Anonymous functions and local functions can be nested and mixed to an arbitrary depth inside any root method.

Function types

• AnonymousFunctionExpressionSyntax: Anonymous function base, including anonymous methods and lambda expressions
  • AnonymousMethodExpressionSyntax: Classic anonymous method expressions using the delegate keyword
  • LambdaExpressionSyntax: Lambda expressions, including simple and parenthesized
• LocalFunctionStatementSyntax: Local functions, only supported as of C# version 7
• MethodDeclarationSyntax: Methods (member functions)

Solution

1. Start by fixing the invocation
2. Then move up to fix its parent function
3. Then continue moving up to fix the next parent function as long as the child function has any invocations in the parent function, continuing until no invocations of a child function are found in the parent or the root method has been fixed

1. Always fix the violating invocation

1. Fix invoked method name by appending "Async"
2. Add await to the invocation

• Assert.Throws(typeof(Exception), ...) becomes await Assert.ThrowsAsync(typeof(Exception), ...) (test cases were missing for this assertion method, so I added them)
• Assert.Throws<Exception>(...) becomes await Assert.ThrowsAsync<Exception>(...)
• Assert.Throws<ArgumentException>("parameter", ...) becomes await Assert.ThrowsAsync<ArgumentException>("parameter", ...)
• Assert.ThrowsAny<Exception>(...) becomes await Assert.ThrowsAnyAsync<Exception>(...) (test cases were missing for this assertion method, so I added them)

2. Always fix the innermost function

The innermost function is the direct parent function of the violating invocation. In the simple case with no nested functions, this is the method itself. Otherwise, it is an anonymous function or local function.

1. Add the async keyword to the function's modifiers
2. Fix the function's declared return type (see below for how anonymous functions are handled as compared to local functions and methods)

As suggested by @bradwilson, await is not added to invocations of the function. This will result in errors and warnings, which the developer can fix manually.

See AssertThrowsShouldNotBeUsedForAsyncThrowsCheckFixer for details on how each type of function is fixed. Note the private interface and three implementing private classes used for fixing the functions:

• IFunctionFixer
• AnonymousFunctionFixer
• LocalFunctionFixer
• MethodFixer

Fixing return types for anonymous functions

If the anonymous function is directly used to initialize or assign to a local variable (without intermediate operations), and if the variable declaration's type is an explicit system delegate type (System.Action, System.Action<...>, or System.Func<...>) equivalent to the anonymous function's inferred converted type, then convert the declaration's type to the corresponding async system delegate type.

• Action becomes Func<Task>
• Action<T> becomes Func<T, Task>
• Func<TResult> becomes Func<Task<TResult>> (unless TResult is already a task)
• Func<T, TResult> becomes Func<T, Task<TResult>> (unless TResult is already a task)
• And so on, for any Action with arity up to 16 and for any Func with arity up to 17
• var is left unchanged
• Custom delegate types and other types are left unchanged

Limitation: I only handled the simple cases of direct initialization and direct assignment. I did not attempt to handle more complex cases where a local variable was initialized or assigned indirectly with the anonymous function wrapped in one or more intermediate operations.

Fixing return types for methods and local functions

Convert the return type to the corresponding Task or Task<T> (unless it is already a task).

3. Repeatedly fix the next parent function if applicable

Repeatedly fix the next parent function (if any) in the same way, but only if the child function is invoked at least once in the parent function. Otherwise, stop fixing, short-circuiting the upwards async cascade.

Counting anonymous function invocations

If the anonymous function is directly used to initialize or assign to a local variable (without intermediate operations), count the parent function's number of descendant invocation operations where the invocation's target method's method kind is MethodKind.DelegateInvoke and the invocation is applied to a local symbol that equals the variable declarator's local symbol.

This approach handles invocations of the form function(...), function.Invoke(...), and function?.Invoke(...).

Limitation: I only handled the simple cases of direct initialization and direct assignment to a local variable. I did not attempt to handle more complex cases where a local variable was initialized or assigned indirectly with the anonymous function wrapped in one or more intermediate operations.

Limitation: I also did not attempt to account for anonymous functions assigned to parameters, fields, properties, or events.

Counting local function invocations

Count the number of direct invocations of the local function child inside the parent function.

Limitation: This ignores indirect invocations via delegates created using a method reference to the local function.

[jared-chevalier]

@dotnet-policy-service agree

[bradwilson]

Thanks, this is excellent! The tests in particular are clear and thorough. And apologies for the conflict with my RS1035 cleanup work. (I'm hopeful that they'll eventually either provide formatting overrides, or just disable the CultureInfo.CurrentCulture prohibition in fixers.)

The only things I updated were my formatting/sorting nitpicks which I always fix myself since I never bothered to write them all down. 😂

[bradwilson]

Let me know if there are additional edge cases you think I should handle

I couldn't think of any. If there are some, people will find them and file them. In particular they'd probably already be experiencing problems with the existing fixer, so I think you'll be moving us forward significantly in compatibility.

[jared-chevalier]

Thank you for your quick response and for your feedback!

And apologies for the conflict with my RS1035 cleanup work.

No problem. Resolving the merge conflict was pretty trivial.

The only things I updated were my formatting/sorting nitpicks which I always fix myself since I never bothered to write them all down. 😂

Sounds good. I tried to follow the existing style, but wasn't always sure whether it was based on soft context-based preferences or hard universal rules, and I'm sure some of my idiosyncratic style slipped in. Judging from your commit and looking elsewhere in the repository, something like the following should be considered as rules? Anything else I missed?

• Always alphabetically order members, grouped by member type (fields, constructors, properties, methods, nested types), regardless of modifiers
• Always wrap and indent method parameters unless there are 0 or 1
• Always wrap and indent the entire right-hand side of a multi-line declaration/assignment, with subsequent chaining double-indented as a result
• Always use expression-bodied methods when possible
• Always include an empty line separating preprocessor directives and other code
• (Forgetting to remove the unused usings was an oversight, as I normally remove and sort usings before committing)

If this is correct, I can see a couple more small changes that would to make the code more consistent with the codebase style, plus a couple of other tweaks. Should I create another issue and then pull request for that? Or could I directly submit a pull request for small improvements like that?

[bradwilson]

You can just send a pull request, no issue required.

That list looks right at first glance (despite my reservation about using words like "Always" because I'm sure there must be some exceptions somewhere 😁). I try to keep the code as clean as I can, but even I have been guilty of violating these rules sometimes, usually by accident. It doesn't help that I changed the rules for v3 core framework source, so most of the v2 core framework source does not match everything else (the exception being the shared assertion library source).

[jared-chevalier]

You can just send a pull request, no issue required.

Ok, sounds good!