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

SE: Learn from bool collection methods #9497

Merged
merged 8 commits into from
Jul 4, 2024
Merged

SE: Learn from bool collection methods #9497

merged 8 commits into from
Jul 4, 2024

Conversation

mary-georgiou-sonarsource
Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource commented Jul 3, 2024

Related to #8266 and #7457

@mary-georgiou-sonarsource mary-georgiou-sonarsource added Area: CFG/SE CFG and SE related issues. Sprint: Hardening Fix FPs/FNs/improvements labels Jul 3, 2024
@@ -236,7 +234,6 @@ public static int NonLinqExtensionNullChecking(this IEnumerable<int> source)

[DataTestMethod]
[DataRow("Count")]
// [DataRow("Any")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These cases at this point need their own test - not sure if it worths it.

@@ -49,11 +49,9 @@ public void InstanceReference_SetsNotNull_VB()

[DataTestMethod]
[DataRow("Aggregate", "(x, y) => x")]
//[DataRow("Any")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These cases at this point need their own test - not sure if it worths it.

Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

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

Review Round 1

Comment on lines +92 to +96
return states.SelectMany(x => new List<ProgramState>
{
x.SetOperationConstraint(invocation, ObjectConstraint.Null),
x.SetOperationConstraint(invocation, ObjectConstraint.NotNull)
}).ToArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't review this - it will change in the next PR.

Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looks pretty good already - a few comments, but nothing major.

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.

I replied to the outstanding suggestions from the first review.

Copy link

sonarcloud bot commented Jul 4, 2024

Copy link

sonarcloud bot commented Jul 4, 2024

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, you can add the UTs for the extension method in a follow-up.

@@ -42,4 +42,10 @@ internal static class IInvocationOperationExtensions
|| (invocation.TargetMethod.IsExtensionMethod
&& !invocation.Arguments.IsEmpty
&& state.ResolveCaptureAndUnwrapConversion(invocation.Arguments[0].ToArgument().Value).Kind == OperationKindEx.InstanceReference);

public static IOperation GetInstance(this IInvocationOperationWrapper invocation, ProgramState state) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

You could improve coverage by adding UTs for this extension method. This would be good practice in general. I'm sorry I missed mentioning this earlier.

@Tim-Pohlmann Tim-Pohlmann merged commit db48fa2 into master Jul 4, 2024
31 checks passed
@Tim-Pohlmann Tim-Pohlmann deleted the mary/fp-s2259 branch July 4, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CFG/SE CFG and SE related issues. Sprint: Hardening Fix FPs/FNs/improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants