-
Notifications
You must be signed in to change notification settings - Fork 228
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.
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
Fix S3878 AD0001: Cover the case of ImplicitArrayCreationExpressionSyntax #9456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch can be simplified, looks good otherwise.
argument switch | ||
{ | ||
_ when argument.Expression as ArrayCreationExpressionSyntax is { } arrayCreation => model.GetTypeInfo(arrayCreation.Type.ElementType).Type, | ||
_ when argument.Expression as ImplicitArrayCreationExpressionSyntax is { } implicitArrayCreation => model.GetTypeInfo(implicitArrayCreation).Type, | ||
_ => null | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument switch | |
{ | |
_ when argument.Expression as ArrayCreationExpressionSyntax is { } arrayCreation => model.GetTypeInfo(arrayCreation.Type.ElementType).Type, | |
_ when argument.Expression as ImplicitArrayCreationExpressionSyntax is { } implicitArrayCreation => model.GetTypeInfo(implicitArrayCreation).Type, | |
_ => null | |
}; | |
argument.Expression switch | |
{ | |
ArrayCreationExpressionSyntax arrayCreation => model.GetTypeInfo(arrayCreation.Type.ElementType).Type, | |
ImplicitArrayCreationExpressionSyntax implicitArrayCreation => model.GetTypeInfo(implicitArrayCreation).Type, | |
_ => null | |
}; |
argument switch | ||
{ | ||
_ when argument.Expression as ArrayCreationExpressionSyntax is { } arrayCreation => model.GetTypeInfo(arrayCreation.Type.ElementType).Type, | ||
_ when argument.Expression as ImplicitArrayCreationExpressionSyntax is { } implicitArrayCreation => model.GetTypeInfo(implicitArrayCreation).Type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong. It does return the array type (e.g., int[]
), but the method is called ArrayElementType (which would be int
). It should be (model.GetTypeInfo(implicitArrayCreation).Type as IArrayTypeSymbol)?.ElementType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe more UTs are required then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the UTs cover differences in the array rank? I don't think so, and maybe the implementation as well:
void Method(params object[] args) { }
Method(new object[] { null }); // Noncompliant
// These cases might be FPs
Method(new object[,] { { null } }); // Compliant
Method(new[,] { { new object() } }); // Compliant
// This should work
Method(new object[][] { new[] { new object() } }); // Compliant
params array must be single-dimensional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cover only jagged arrays.
string.Format(CultureInfo.InvariantCulture, "{0}.{1}", new object[] { "", new object() }); | ||
MethodImplicitArray(new[] { "Hello", "Hi" }); // Noncompliant , Implicit array creation | ||
Method(new [] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method(new [] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] | |
Method(new[] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add one more test with a multidimensional array
Method(new [] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] | |
Method(new[] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] | |
Method(new[,] { { 1, 2 } }); // Compliant, Elements in args: [System.Int32[,]] |
5aa8663
to
25e6f5f
Compare
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, because the cases left are edge cases, but there seems to be something wrong with the implementation. I think there is some special handling with object[]
params and I'm not sure, we handle multi-dimensional arrays properly.
You may want to have another look later.
MethodImplicitArray(new[] { "Hello", "Hi" }); // Noncompliant , Implicit array creation | ||
Method(new[] { 1, 2, 3, }); // Compliant, Elements in args: [System.Int32[]] | ||
Method(new[,] { { 1, 2 } }); // Compliant, Elements in args: [System.Int32[,]] | ||
Method(new object[] { null }); // Compliant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Method(new String[] { "1", "2" });
Noncompliant, while Method(new object[] { null });
is compliant? This looks like an FN to me.
No description provided.