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: Move CollectionConstraint from S4158 to the engine (part 1) #8702

Merged

Conversation

gregory-paidis-sonarsource
Copy link
Contributor

Part of #7866

Moves the following logic from the rule to the ObjectCreation processor:

void Foo(List<int> one)
{
  var two= new List<int>(one); // apply one's "CollectionConstraint"  to two
}

@gregory-paidis-sonarsource gregory-paidis-sonarsource changed the title CollectionConstraint: Move it to the engine (part 1) SE: Move CollectionConstraint from S4158 to the engine (part 1) Feb 6, 2024
@gregory-paidis-sonarsource gregory-paidis-sonarsource marked this pull request as ready for review February 6, 2024 16:05
&& CollectionCreationConstraint(context.State, operation) is { } constraint)
{
return context.SetOperationConstraint(constraint)
.SetOperationConstraint(operation, ObjectConstraint.NotNull);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is setting the NotNull constraint as well useful?
It didn't happen before, but I do not think it can be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be logic somewhere that handles this case already. Make sure it still works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the UTs, it seems to be working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because you are setting it here. What I'm saying is there should be code somewhere that is setting NotNull already. Remove your code and see if it still works.

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 misunderstood what you meant.
You are both correct and wrong:

  • It does not work if I remove it
  • There is indeed code somewhere that is setting it to NotNull. And it's 20 lines down :)

I basically over-wrote the previous behavior by adding this if-arm.
I re-structured the conditionals a bit, I think it makes more sense no

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/move-collection-constraint-1 branch from 9278c4a to 52eb017 Compare February 6, 2024 16:49
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.

Extract everything into a new class and we should be good.

KnownType.System_Collections_Immutable_IImmutableArray_T,
KnownType.System_Collections_Generic_Dictionary_TKey_TValue,
KnownType.System_Collections_Generic_IDictionary_TKey_TValue,
KnownType.System_Collections_Immutable_IImmutableDictionary_TKey_TValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this will be private and consumers will just query this class via static methods.
I want to do this gradually, so I will leave it public for now and pluck the references one by one in smaller PRs.

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/move-collection-constraint-1 branch from da8d5ba to 879d2d8 Compare February 7, 2024 10:22
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/move-collection-constraint-1 branch from 879d2d8 to 64dfe46 Compare February 7, 2024 10:26
var declared = new Exception();
assigned = new EventArgs();
var collection1 = new List<int>();
collection1.Add(42);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails to set the constraint from empty to non-empty, because it's part of the rule (for now)

Copy link

sonarqubecloud bot commented Feb 7, 2024

Copy link

sonarqubecloud bot commented Feb 7, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

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.

Nicely done! Only some naming suggestions.

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit ab589ec into master Feb 7, 2024
39 checks passed
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the greg/move-collection-constraint-1 branch February 7, 2024 15:57
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.

2 participants