Primary Constructors - Don't report unused parameters#67167
Primary Constructors - Don't report unused parameters#67167akhera99 merged 8 commits intodotnet:mainfrom
Conversation
| var methodSyntax = method.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); | ||
| if (method.IsConstructor() && | ||
| _compilationAnalyzer.IsRecordDeclaration(methodSyntax)) | ||
| (_compilationAnalyzer.IsRecordDeclaration(methodSyntax) || _compilationAnalyzer.IsClassDeclaration(methodSyntax))) |
There was a problem hiding this comment.
Where do we make sure this is a primary constructor and not a regular class constructor? Can you add tests to ensure that unused parameters are still flagged for a regular user written constructor in a regular class declaration?
There was a problem hiding this comment.
Ah, I see how this will work. Constructor symbol with its syntax node as a class declaration syntax guarantees it being a primary constructor. Would still be good to add a test for unused parameters of a regular constructor (if one doesn’t already exist)
There was a problem hiding this comment.
i don't love that we're testing this way. I'd prefer it be done similar to the extension we have for GetAssociatedSynthesizedRecordProperty. Can you make a similar extension for this question (i.e. 'IsPrimaryConstructor').
| End Function | ||
|
|
||
| Protected Overrides Function IsClassDeclaration(node As SyntaxNode) As Boolean | ||
| Return False |
There was a problem hiding this comment.
This feels weird, I believe we should check for node being a ClassDecarationSyntax here. Obviously no constructor in VB will have ClassDeclarationSyntax associated with it as VB doesn’t support primary constructors, so the modified if condition in base type will always be false for VB.
mavasani
left a comment
There was a problem hiding this comment.
LGTM.
Minor suggestion for additional tests, and update to VB override.
| var methodSyntax = method.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); | ||
| if (method.IsConstructor() && | ||
| _compilationAnalyzer.IsRecordDeclaration(methodSyntax)) | ||
| (_compilationAnalyzer.IsRecordDeclaration(methodSyntax) || _compilationAnalyzer.IsClassDeclaration(methodSyntax))) |
There was a problem hiding this comment.
Should this just be _compilationAnalyzer.IsTypeDeclaration(methodSyntax) and replace both methods with a single IsTypeDeclaration method?
| @@ -231,7 +231,7 @@ parameter.ContainingSymbol is not IMethodSymbol method || | |||
| // TODO: Remove this when implicit operations are synthesised: https://github.com/dotnet/roslyn/issues/47829 | |||
There was a problem hiding this comment.
Consider removing this comment. The issue was closed as by-design.
| { | ||
| MethodKind: MethodKind.Constructor, | ||
| DeclaringSyntaxReferences.Length: > 0, | ||
| ContainingType: { IsType: true } containingType, |
There was a problem hiding this comment.
no need for the IsType: true check.
| ContainingSymbol: IMethodSymbol | ||
| { | ||
| MethodKind: MethodKind.Constructor, | ||
| DeclaringSyntaxReferences.Length: > 0, |
There was a problem hiding this comment.
| DeclaringSyntaxReferences.Length: > 0, | |
| DeclaringSyntaxReferences.Length: [var constructorReference, ..], |
can then use that below.
| { | ||
| if (parameter is | ||
| { | ||
| DeclaringSyntaxReferences.Length: > 0, |
There was a problem hiding this comment.
is this check needed?
...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/IParameterSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
…nsions/IParameterSymbolExtensions.cs Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
Fixes: #67013
Falls under the same issue as #47830