AddParameterCodeFixProvider: Add support for method invocations. #22314
Conversation
…use insert position logic. First simple C# tests.
|
Remarks to the current state of the PR Method invocations equivalent to
Generic Methods Commit 6dfd11e added support for type parameters and generalization for generic methods (Invocation part) and classes (ObjectCreation part). void M1(int i) { }
void M2()
{
M1<bool>(1, true);
} Fix: void M1<T>(int i, T bool) { }
void M2()
{
M1<bool>(1, true);
} Same approach for classes: class C1 {
C1() { }
}
var c = new C1<bool>(true); Fix: class C1<T> {
C1(T v) { }
}
var c = new C1<bool>(true); TODO: Interface inheritance and method overriding Methods that are declared by an contract and are implemented/overridden are breaking the code because the refactoring should be fixing the contract and the implementation but does only either.
In my opinion this is fine because adding a parameter to a method is always a breaking change.
I tend to say 'Let the fix do what it is supposed to do and let the user fix the mess this fix almost always introduces.' Discussion needed. Delegate invocation e.g.: Action a = () => { };
a(2);Delegate invocations are different because the fix needs to change the delegate type and the method declaration: Action<int> a = (int v) => { };Fixing the delegate type is not easy but could be achieved for some delegates:
I was able to partially add support for delegates. Currently the above code is fixed to this broken code: Discussion needed. Partial methods Commit d300c80 introduced support for partial methods. While technically correct this is likely the wrong behavior because partial methods are mostly related to generated code and the fix isn't really helpful. This would be a use case for #4530. CS0539 'member' in explicit interface declaration is not a member of interface e.g.: interface I1
{
void M1();
}
class C1 : I1
{
void I1.M1() { }
void I1.[|M1|](int i) { }
} This one is different from the other supported cases because the diagnostic is reported on the method declaration and not on the invocation of a method. I think this case can not be supported by this analyzer as it heavily relies on type information from the invocation. Test coverage Test for VB are missing. Before I proceed I need some advice to this points:
If these questions are addressed I could proceed to increase the code coverage. |
|
I went to all the Summary:
|
sharwell
added
the
Community
|
📝 Based on a discussion with @MaStr11, I've assigned this to the 15.7 milestone which gives time to review the edge cases and test coverage without excessive delays. 👍 |
|
I updated my previous two comments to reflect the current state of the PR. |
MaStr11
changed the title
|
Could I get some feedback please? |
| private static readonly ImmutableArray<string> AddParameterFixableDiagnosticIds = | ||
| GenerateConstructorDiagnosticIds.AllDiagnosticIds.Union( | ||
| GenerateMethodDiagnosticIds.FixableDiagnosticIds).Union( | ||
| Enumerable.Repeat("CS1593", 1)). // C# Delegate 'Action' does not take 1 arguments |
There was a problem hiding this comment.
Just use normal ImmutableArray methods like Add if possible
| var syntaxFacts = document.GetLanguageService<ISyntaxFactsService>(); | ||
|
|
||
| var expression = syntaxFacts.GetExpressionOfInvocationExpression(invocationExpression); | ||
| if (expression == null) |
There was a problem hiding this comment.
should be impossible (as long as invocationExpression itself is non-null).
| if (candidates.Length == 0) | ||
| { | ||
| // Invocation might be on an delegate. We try to find the declaration of the delegate. | ||
| // This only fixes the declaration but not the delegate type. This ode is for test purposes only and will likely be removed. |
There was a problem hiding this comment.
is this code needed anymore?
There was a problem hiding this comment.
No. I just kept it to illustrate that delegate invocations are going to be much more complicated and proposed in one of the comments to not taken those into account for this PR.
|
|
||
| var arguments = (SeparatedSyntaxList<TArgumentSyntax>)syntaxFacts.GetArgumentsOfInvocationExpression(invocationExpression); | ||
| var typeArguments = (SeparatedSyntaxList<TTypeSyntax>)syntaxFacts.GetTypeArgumentsOfInvocationExpression(invocationExpression); | ||
| var argumentInsertPositionInMethodCandidates = GetArgumentInsertPositionAndTypeArgumentForMethodCandidates(semanticModel, syntaxFacts, candidates, arguments, argumentOpt, typeArguments); |
There was a problem hiding this comment.
wrap lines longer than github width plz.
|
|
||
| var comparer = syntaxFacts.StringComparer; | ||
| var constructorsAndArgumentToAdd = ArrayBuilder<(IMethodSymbol constructor, TArgumentSyntax argument, int index)>.GetInstance(); | ||
| private void RegisterFixForMethodOverloads(CodeFixContext context, SeparatedSyntaxList<TArgumentSyntax> arguments, ImmutableArray<(IMethodSymbol method, TArgumentSyntax argument, int index, TTypeSyntax typeArgument)> methodsAndArgumentToAdd) |
There was a problem hiding this comment.
wrap parameter lists longer than github width plz.
There was a problem hiding this comment.
consider renaming methodsAndArgumentToAdd to insertionData.
consider using a real named type here as excessively long tuples end up being hard to read.
| Enumerable.Repeat(method, 1) | ||
| .Union(Enumerable.Repeat(method.PartialDefinitionPart, 1) | ||
| .Union(Enumerable.Repeat(method.PartialImplementationPart, 1))) | ||
| .Where(methodSymbol => methodSymbol != null) |
| //https://stackoverflow.com/a/38294041 | ||
| //Therefore we need this ugly hack: | ||
| var methodSymbolReferences = | ||
| Enumerable.Repeat(method, 1) |
There was a problem hiding this comment.
you could just do something like "ImmutableArray.Create(method, method.PartialDefinitionPart, method.PartialImplementationPart).WhereNotNull().Distinct().ToImmutable()".
This usage of .Union and Enumerable.repeat is very unlike any of our other code.
|
|
||
| //method.DeclaringSyntaxReferences does no longer return the two locations for partial methods so this is no longer true: | ||
| //https://stackoverflow.com/a/38294041 | ||
| //Therefore we need this ugly hack: |
There was a problem hiding this comment.
please remove this comment. This is not an 'ugly hack'. :)
| var methodDeclarationRoot = methodDeclaration.SyntaxTree.GetRoot(cancellationToken); | ||
| var editor = new SyntaxEditor(methodDeclarationRoot, methodDocument.Project.Solution.Workspace); | ||
| var newRoot = editor.GetChangedRoot(); | ||
| newSolution = newSolution.WithDocumentSyntaxRoot(methodDocument.Id, newRoot); |
There was a problem hiding this comment.
this loop feels like it will be wrong if both parts of the partial method are in the same document.
There was a problem hiding this comment.
Note: we will need to update all related overrides/implementations.
For examples of how we do this elsewhere, take a look at ReplaceMethodWithPropertyCodeRefactoringProvider. Specifically ReplaceGetMethodsAndRemoveSetMethodsAsync.
Here's the basic idea:
- You're going to do a SymbolFinder.FindReferencesAsync call to find all the cascaded methods from the method you want to update.
- You'll ignore the actual references, and just use all the definitions that were returned.
- You'll then group all the definitions by their defining-document. This way you can process one doc at a time, and update all the methods in that document at once.
- You then process each document one at a time, creating the new document.
- You create a new solution snapshot out of all these updated documents.
To update a document we create a single SyntaxEditor for that document, then go and process all the IMethodSymbols we grouped for that doucment. For each IMethodSymbol we go do it's declaration and update it according, making the change to the editor.
--
Note: if you do this, you don't need any special partial-method handling. It will just fall out naturally from doing the FindReferences.
| case GenericNameSyntax genericName: | ||
| return genericName.TypeArgumentList.Arguments; | ||
| case MemberAccessExpressionSyntax memberAccessExpression when memberAccessExpression.Name is GenericNameSyntax genericName: | ||
| return genericName.TypeArgumentList.Arguments; |
There was a problem hiding this comment.
- what happens if you have a
foo?.Bar<X>foo.bar?.Baz<X>orfoo?.bar.baz<X>? - Syntax facts are for unifying concepts over VB and C#, not for generalized deconstruction APIs. So, in this case, you should remove this helper and move it to the AbstractAddParameter code. It would grab the ExpressionOfInvocationExpression then do the tests about GenericName/MemberAccess, and decompose things there.
| @@ -1273,6 +1273,16 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
| Return If(arguments.HasValue, arguments.Value, Nothing) | |||
| End Function | |||
|
|
|||
| Public Function GetTypeArgumentsOfInvocationExpression(invocationExpression As SyntaxNode) As SeparatedSyntaxList(Of SyntaxNode) Implements ISyntaxFactsService.GetTypeArgumentsOfInvocationExpression | |||
| Dim arguments = TryCast(TryCast(invocationExpression, InvocationExpressionSyntax)?.Expression, GenericNameSyntax)?.TypeArgumentList?.Arguments | |||
| Return If(arguments.HasValue, arguments.Value, Nothing) | |||
There was a problem hiding this comment.
this is an example of why this is wrong. say you have "foo.Bar(of X)(y, z, w)". This code will not work because the Expression is not a GenericNameSyntax, it's a MemberAccessExpressionSyntax. But you only encoded the full decomposition in the C# side of things.
Instead, the decomposition should move into AbstractAddParameterXXX
| protected override void M1(int v) { } | ||
| void M2() | ||
| { | ||
| M1(1); |
There was a problem hiding this comment.
virtual was not updated as well.
There was a problem hiding this comment.
Yes. I have a section in one of the comments that discusses this kind of problems (Interface inheritance and method overriding). I proposed to not fix along the inheritance chain but that might not be what you want.
There was a problem hiding this comment.
is there a reason this is provided as one of option? it doesn't seems reducing much work from users.
There was a problem hiding this comment.
tagging @kuhlenh @jinujoseph since it basically makes override to have 2 entries in code fix list.
|
@MaStr11 looks very nice! thank you for this PR! I am okay taking this PR. but there are some design question we probably need to get +1 from PM (@kuhlenh) and lead (@jinujoseph) First question is Second question is Third question is ... now more like dev questions. first question is second question is anyway, no reason to block this PR. looks good! who knows people might like this approach better than signature change and we might change signature change a bit more like this! |
ya, I keep type signature help. I guess muscle memory. I meant signature change obviously.. |
|
We discussed fixing the call site but decided to do it later to keep the PR small enough. See e.g. my #22314 (comment) from Feb 20. The PR was started in September 2017. Back then |
don't worry about call site. if we decide to fix call site, we should use signature change service in this fixer rather than re-inventing wheel again. I am not suggesting you do that in this PR. but we might open tracking PR (@jinujoseph @kuhlenh) in case we want to support references.
don't worry about IOperation as well. again, not suggesting you should use IOperation here. it is something we probably need to discuss in our design meeting @jinujoseph if we keep syntax/semantic style in analyzers/fixers, people will keep using them like
it will be never considered how long time passes. I think we should decide whether we will embrace it or not, and if we are going to embrace it, we should go and change all of analyzers/fixers to IOperation. and fix issues if we encounter those. we did same when we moved to new light bulb system (we had different system, but when we had new lightbulb system, we went ahead and changed all existing ones to LB system and fixed issues along the way including functional or performance). we did same when we introduced syntax editor/generator. we did same for IOperation for roslyn analyzer repo. I am +1 on doing same for roslyn repo as well. I am quite against on the opinion that we block converting to IOperation until we somehow magically know perf of IOperation is good enough. I believe only way we can improve IOperation's perf or know that its perf is good enough is actually converting all existing analyzers to IOperation and actually dogfooding them. for example, I dont believe the SQlite perf issue we had we would have known unless we let it checked in and dogfooding, if we blocked it for another 10 months speculating about its perf impact, I believe all we got will be finding out exact same issue just 10 months later. same goes for editorconfig work, all the thing picked up was never the issue. the actual issue was dead lock due to JTF. if it got checked in 1 week earlier, it would have found the issue 1 week earlier and had enough time to put editor fix in the 15.7. all the blocking based on speculation did not actually fix or help anything but made thing just worse. |
…tinct the list of MethodSymbols returned from symbol finder to avoid duplicated work.
jinujoseph
added
the
Need Design Review
Changing the order of parameters or removing a parameter is quite different from adding one. You can't fix the call site if you add a parameter. I doubt that AbstractChangeSignatureService is of any help here. There is only a limited set of call sites that can be fixed:
Maybe there are more but most often you just can't figure out how the caller needs to be fixed. Adding a parameter is a breaking change for all caller's (except the one that triggered the code fixer). |
|
@jinujoseph this PR was in the works since September 2017. Moving it into the DesignPending bucket comes late. Nobody answered my design questions in the beginning and I went down a road that needed to be reversed later (I wanted to include support for type parameters). |
|
@MaStr11 This is an unfortunate fallout of the timing of the work the team is doing to be better at handling community contributions. I'm hoping the new policies make this sort of thing go much more smoothly. Note: "need design review" is nto a bad thing. It effectively means: this will go to the IDE design group to just ensure that they like what's going on here. Ideally they will be ok with everything. If not, that's def annoying, but is more a fallout of the non-existent processes in place around community contributions in the past. Ideally, in the future this sort of thing would be much more front loaded. You can see this happening already with other community contributions. People are much quicker to move it along the process so that design choices can be hammered out up front before a lot of investment has been made into the implementation. |
|
@MaStr11 , |
|
@MaStr11 This went to a design meeting yesterday. The general theme focused on two elements:
Of the alternatives we may consider in the future, this pull request does a great job of defining an alternative and implementing it in a usable manner across a variety of inputs. We see no reason to block this pull request, and intend to learn what we can from usage patterns. We may or may not revisit the decision in a future release. ❓ Aside from the obvious merge conflict and in light of the design discussion outcome, did you see any further work this needs before getting it merged? |
sharwell
removed
the
Need Design Review
I also think that this PR is quite different from change signature:
This PR has more in common with the "Add method overload" fix than with the change signature dialog.
@heejaechang requested some changes and I think I addressed all of them. A final review by the buddy (Who is this btw) of the PR would be appreciated. It would also be useful to create some follow up issues if this PR get's merged. Out of my mind, this would be:
public class BaseClass
{
public virtual void M() { }
}
public class Derived: BaseClass
{
public override void M()
{
base.M();
}
public void Test()
{
M(1); // error CS1501: No overload for method 'M' takes 1 arguments
}
}Fix: public class BaseClass
{
public virtual void M(int i) { }
}
public class Derived: BaseClass
{
public override void M(int i)
{
base.M(i); // This call can be fixed as shown, but isn't for now.
}
public void Test()
{
M(1);
}
} |
That's me, and thanks for the update 😄 |
…terCodeFixForMethod
Customer scenario
Fixes #21446.
Fixes #25143.
Follow up to #17082.
This PR expands the
AddPrarameterCodefixProviderto also support method invocations.Bugs this fixes:
Fixes #21446.
Follow up to #17082.
Workarounds, if any
None.
Risk
Code refactoring might break user code.
Performance impact
Low. The analyzer is already shipped and this only increases the number of supported diagnostics.
Is this a regression from a previous update?
#17082 intentionally left method invocation out, because of the lots of complicated cases.
Root cause analysis:
Was intended for future improvement.
How was the bug found?
Customer reported. #21446
Test documentation updated?
No.