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 - Nullable: Propagate constraints via nullable constructor #6856

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

Part of #6812

Set argument constraints for new Nullable<int>(value) or new Nullable<int>(42)

This is a draft, the 1st Squash 03-NewArg commit doesn't need a review as it is in #6853

Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to sync our work for this PR.

}
else if (context.State[operation.Arguments.First().ToArgument().Value] is { } value)
{
return context.State.SetOperationValue(context.Operation, value.WithConstraint(ObjectConstraint.NotNull));

Choose a reason for hiding this comment

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

I don't think we need this with my work. The argument must be a non-nullable value type. Before it gets passed to the argument, there is a reference operation. We will learn on that operation the operation value "NotNull". This is more precise as it also supports e.g. new int?(true ? a : 42) as a learns "NotNull" on the reference operation and 42 learns NotNull in "ConstantCheck".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SetOperationValue throws away whatever constraint was there from PreProcess.
We need it this way to propage other random constraints (like cryptography, or the future CBDE replacement constraints)

So those 2 do not collide (too much). In theory, the value that will arrive should already have your constraint. And in that case, WithConstraint will do NOOP as I added the check there.

@pavel-mikula-sonarsource pavel-mikula-sonarsource marked this pull request as ready for review March 6, 2023 15:53
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests can be re-written without the "ToString" workaround.

""";
var setter = new PreProcessTestCheck(OperationKind.Literal, x => x.Operation.Instance.ConstantValue.Value is false ? x.SetOperationConstraint(TestConstraint.First) : x.State);
var validator = SETestContext.CreateCS(code, setter).Validator;
validator.ValidateTag("IsTrue", x => x.AllConstraints.Select(x => x.ToString()).OrderBy(x => x).JoinStr(", ").Should().Be("NotNull, True"));

Choose a reason for hiding this comment

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

I moved away from the "ToString" based testing. Instead I do something like this:

Suggested change
validator.ValidateTag("IsTrue", x => x.AllConstraints.Select(x => x.ToString()).OrderBy(x => x).JoinStr(", ").Should().Be("NotNull, True"));
validator.ValidateTag("IsTrue", x => x.AllConstraints.Select(x => x.Kind).Should().BeEquivalentTo(new [] { ConstraintKind.ObjectNotNull, ConstraintKind.BoolTrue }));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to have x.Should().HaveConstraints(...params...) :)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. (I can not merge because of the unrelated IT failure).

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