Skip to content
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 FP: Jagged arrays #9102

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Conversation

CristianAmbrosini
Copy link
Contributor

Fixes #8163

Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

LGTM assuming SC analysis turns out ok after the pipeline is fixed.

@@ -95,7 +95,7 @@ public void CallMethod()
MethodArray(new String[] { "1", "2" }, new String[] { "1", "2" }); // Noncompliant, FP. Elements in args: [System.String[], System.String[]]
MethodArray(new int[] { 1, 2 }, new int[] { 1, 2 }); // Noncompliant, FP. Elements in args: [System.Int32[], System.Int32[]]

MethodJuggedArray(new int[] { 1, 2 }); // Noncompliant, FP. Elements in args: [System.Object[]]
MethodJuggedArray(new int[] { 1, 2 }); // Compliant: jagged array [System.Object[]]
Copy link
Contributor

Choose a reason for hiding this comment

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

jugged :D

Suggested change
MethodJuggedArray(new int[] { 1, 2 }); // Compliant: jagged array [System.Object[]]
MethodJaggedArray(new int[] { 1, 2 }); // Compliant: jagged array [System.Object[]]

: null;

private static bool IsJaggedArrayParam(IParameterSymbol param) =>
param.Type is IArrayTypeSymbol { ElementType: IArrayTypeSymbol };
Copy link
Contributor

Choose a reason for hiding this comment

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

param.Type is always IArrayTypeSymbol, correct? I wonder if a direct cast would be more explicit here. Then again... I like the pattern-matching syntax :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear I replied but I don't see the message now for some reason. Anyway not always, but in this particular implementation, we are checking previously that the parameter is an array. So yes, that part will always be true. But I would leave the pattern-matching because it's quite neat IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it affects coverage:
image
I'm ok with accepting it as defensive programming.

Copy link

sonarcloud bot commented Apr 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 17, 2024

@CristianAmbrosini CristianAmbrosini enabled auto-merge (squash) April 17, 2024 08:27
@CristianAmbrosini CristianAmbrosini merged commit 1409c5a into master Apr 17, 2024
27 checks passed
@CristianAmbrosini CristianAmbrosini deleted the cristian/S3878_jaggered branch April 17, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix S3878 FP: Jagged arrays
2 participants