Fix S1172: code fix does not delete unused parameter in function call #9534
Conversation
sebastien-marichal
force-pushed
the
sma/s1172-code-fix
branch
from
d611740
to
394fb4c
Compare
|
Low coverage is due to defensive coding. |
|
Can we make sure we find a way to deal with cases where the argument to remove has a side effect? |
| return Task.CompletedTask; | ||
| arguments = callers | ||
| .SelectMany(x => x.Locations) | ||
| .Select(x => GetArgumentFromLocation(x, parameterIndex)); |
There was a problem hiding this comment.
This will not work with out of order named arguments.
| _ => | ||
| { | ||
| var newRoot = root.RemoveNodes( | ||
| [parameter, ..arguments.Where(x => x is not null)], |
There was a problem hiding this comment.
Move the Where to the assignment of arguments.
| if (parameter?.Parent.Parent is BaseMethodDeclarationSyntax methodDeclaration) | ||
| { | ||
| var diagnostic = context.Diagnostics.First(); | ||
| var diagnosticSpan = diagnostic.Location.SourceSpan; | ||
| var parameter = root.FindNode(diagnosticSpan, getInnermostNodeForTie: true) as ParameterSyntax; | ||
| var model = await context.Document.GetSemanticModelAsync(context.CancellationToken); | ||
| var parameterIndex = methodDeclaration.ParameterList.Parameters.IndexOf(parameter); | ||
|
|
||
| if (!bool.Parse(diagnostic.Properties[MethodParameterUnused.IsRemovableKey])) | ||
| if (parameterIndex >= 0 && model.GetDeclaredSymbol(methodDeclaration, context.CancellationToken) is { } methodSymbol) | ||
| { | ||
| return Task.CompletedTask; | ||
| } | ||
| var callers = await SymbolFinder.FindCallersAsync( | ||
| methodSymbol, | ||
| context.Document.Project.Solution, | ||
| ImmutableHashSet.Create(context.Document), | ||
| context.CancellationToken); | ||
|
|
||
| context.RegisterCodeFix( | ||
| Title, | ||
| c => | ||
| { | ||
| var newRoot = root.RemoveNode( | ||
| parameter, | ||
| SyntaxRemoveOptions.KeepLeadingTrivia | SyntaxRemoveOptions.AddElasticMarker); | ||
| return Task.FromResult(context.Document.WithSyntaxRoot(newRoot)); | ||
| }, | ||
| context.Diagnostics); | ||
|
|
||
| return Task.CompletedTask; | ||
| arguments = callers | ||
| .SelectMany(x => x.Locations) | ||
| .Select(x => GetArgumentFromLocation(x, parameterIndex)); | ||
| } |
There was a problem hiding this comment.
If you move it to a method you can call it directly from the RemoveNodes invocation.
| // https://github.com/SonarSource/sonar-dotnet/issues/8187 | ||
| public class Repro_8187_CodeFix | ||
| { | ||
| void Method() | ||
| { | ||
| DoSomething(21); | ||
| } | ||
|
|
||
| void DoSomething(int a) // Fixed | ||
| { | ||
| Console.WriteLine(a); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a test with named arguments, which are out of order. Also some tests with optional arguments.
|
As discussed, as arguments can include very complex expressions, I am only removing argument expressions that do no updates whatsoever. |
sebastien-marichal
force-pushed
the
sma/s1172-code-fix
branch
from
c804c9f
to
a3d4ad7
Compare
|
|
sebastien-marichal
requested review from
Tim-Pohlmann and
mary-georgiou-sonarsource
and removed request for
Tim-Pohlmann
Fixes #8187
It does not support changing usage outside of the current SyntaxTree (e.g.: in case of usage in partial class).