Skip to content

No IDE0060 for unused parameter in partial method implementation #63530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mavasani merged 2 commits into from
Sep 22, 2022
+35 −2
Merged

No IDE0060 for unused parameter in partial method implementation #63530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
967247a
No IDE0060 diagnostic for partial method implementation
louis-z Aug 22, 2022
466493e
Add a test as per request + fix failing tests
louis-z Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedParametersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1430,6 +1430,38 @@ private void M(int [|x|])
");
}

[WorkItem(57814, "https://github.com/dotnet/roslyn/issues/57814")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedParameters)]
public async Task UnusedParameterInPartialMethodImplementation_NoDiagnostic()
{
await TestDiagnosticMissingAsync(
@"
public partial class C
{
public partial void M(int x);
}

public partial class C
{
public partial void M(int [|x|])
{
}
}
");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedParameters)]
public async Task ParameterInPartialMethodDefinition_NoDiagnostic()
{
await TestDiagnosticMissingAsync(
@"
public partial class C
{
public partial void M(int [|x|]);
}
");
}

[Fact, WorkItem(36817, "https://github.com/dotnet/roslyn/issues/36817")]
public async Task ParameterWithoutName_NoDiagnostic()
{
Expand Down
4 changes: 2 additions & 2 deletions ...nalyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ void M()
}

[Fact]
public async Task Initialization_ConstantValue_RemoveUnsuedParametersSuppressed()
public async Task Initialization_ConstantValue_RemoveUnusedParametersSuppressed()
{
var source =
@"class C
Expand Down Expand Up @@ -214,7 +214,7 @@ int M()
}

[Fact]
public async Task Initialization_ConstantValue_RemoveUnsuedParametersNotApplicable()
public async Task Initialization_ConstantValue_RemoveUnusedParametersNotApplicable()
{
var source =
@"class C
Expand Down
1 change: 1 addition & 0 deletions ...ndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.SymbolStartAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ parameter.ContainingSymbol is not IMethodSymbol method ||
method.IsVirtual ||
method.IsOverride ||
method.PartialImplementationPart != null ||
method.PartialDefinitionPart != null ||
Copy link
Member

@Youssef1313 Youssef1313 Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but I feel the check is too strong.

Copy link
Contributor Author

@louis-z louis-z Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that

  • a partial method definition might be generated by some tool, with a parameter list as part of its signature
  • the corresponding implementation must have the same signature

I believe it's pointless to run the IDE0060 rule on the implementation (i.e. when method.PartialDefinitionPart != null ).

After all, if any of the parameters are unused by the implementation, what can the developer do? They probably have no control over the autogenerated definition and the implementation's signature must match the definition's...

Copy link
Member

@Youssef1313 Youssef1313 Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should specifically check for generated code. This check can probably be easier when #63447 is merged

Copy link
Contributor Author

@louis-z louis-z Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about this upcoming IsGeneratedCode flag. You're right, waiting for it sounds like a good idea.

Copy link
Contributor

@mavasani mavasani Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if IsGeneratedCode check helps here. The callback would only be made for non-generated code for IDE0060 analyzer, and you can't check whether or not the other partial symbol is generated or not from this callback.

!method.ExplicitOrImplicitInterfaceImplementations().IsEmpty ||
method.IsAccessor() ||
method.IsAnonymousFunction() ||
Expand Down