SE: Support ReferenceEquals #7005
Conversation
| @@ -55,6 +55,7 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp | |||
| _ when IsNullableGetValueOrDefault(invocation) => ProcessNullableGetValueOrDefault(context, invocation).ToArray(), | |||
| _ when invocation.TargetMethod.Is(KnownType.Microsoft_VisualBasic_Information, "IsNothing") => ProcessInformationIsNothing(context, invocation), | |||
| _ when invocation.TargetMethod.Is(KnownType.System_Diagnostics_Debug, nameof(Debug.Assert)) => ProcessDebugAssert(context, invocation), | |||
| _ when invocation.TargetMethod.Is(KnownType.System_Object, nameof(ReferenceEquals)) => ProcessReferenceEquals(context, invocation), | |||
There was a problem hiding this comment.
Here nameof(ReferenceEquals) is expressed without object., whereas few lines below nameof(object.Equals) is expressed with object..
I don't know which is the recommended way here, but I believe it should be consistent across the file/project.
There was a problem hiding this comment.
There's no specific convention for this. The alignment is a good point. I've removed the other object. because it doesn't need to be there.
| @@ -55,6 +55,7 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp | |||
| _ when IsNullableGetValueOrDefault(invocation) => ProcessNullableGetValueOrDefault(context, invocation).ToArray(), | |||
There was a problem hiding this comment.
The declaration of ProcessNullableGetValueOrDefault appears before ProcessInformationIsNothing by invocation order, but it is declared after ProcessEquals.
Code conventions don't seem to specify what should be the relative order of two methods M2 and M3 called by a method M1: they only say that M2 and M3 should be below M1. However, I believe it would be good to retain the same order, when you have a flat code structure like the one in this switch statement.
There was a problem hiding this comment.
As they don't call each other, it doesn't matter.
| @@ -746,7 +746,7 @@ public void Invocation_InformationIsNothing_NoTrackedSymbol() | |||
| Dim Result As Boolean = IsNothing("""" & Arg.ToString()) | |||
| Tag(""Result"", Result)"; | |||
| var validator = SETestContext.CreateVB(code, ", Arg As Object").Validator; | |||
| validator.ValidateTag("Result", x => x.AllConstraints.Should().ContainSingle().Which.Should().Be(ObjectConstraint.NotNull)); | |||
| validator.ValidateTag("Result", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull)); | |||
There was a problem hiding this comment.
Educational: this is basically equivalent to the previous one (except for null check on x and better messages) right?
Either way, much more readable!
There was a problem hiding this comment.
Yes, this is a mechanical refactoring. The HaveOnlyConstraint is a customextension of the assertion framework and it makes the assertions way easier to read.
So we're effectively migrating the old way to the new extensions.
| """; | ||
| var validator = SETestContext.CreateCS(code).Validator; | ||
| validator.ValidateTag("Result", x => x.HasConstraint(BoolConstraint.From(expectedResult)).Should().BeTrue()); | ||
| validator.ValidateTag("Left", x => x.AllConstraints.Select(x => x.Kind).Should().ContainSingle().Which.Should().Be(expectedConstraintLeft)); |
There was a problem hiding this comment.
Wouldn't be good to have a SymbolicValueAssertions.HaveOnlyConstraintOfKind?
Alternatively, I see x.AllConstraints.Select(x => x.Kind).Should().BeEquivalentTo(new[] { ... }) being used in other files. It seems more readable of a phrase that contains a subordinate relative clause.
There was a problem hiding this comment.
Yes, it would. More precisely just another overload on HaveOnlyConstraint.
We just didn't have time to do it yet, the current extensions were merged this sprint.
| public void Invocation_ReferenceEquals_LearnResult(string left, string right, bool expectedResult, ConstraintKind expectedConstraintLeft, ConstraintKind expectedConstraintRight) | ||
| { | ||
| var code = $""" | ||
| object left = {left}; |
There was a problem hiding this comment.
Non-actionable remark: by doing object left and object right, as opposed to passing left and right to the method directly, you are boxing every instance of a value type you pass to the test method.
It works here because ReferenceEquals is not generic and takes object for both parameters, but there would be differences otherwise.
There was a problem hiding this comment.
Yes, there's boxing in the way. And we don't care at this stage as you've mentioned. The direct value would get boxed on the argument anyway, and we need to assert the state after, so we need to capture it.
| } | ||
|
|
||
| [TestMethod] | ||
| public void Invocation_ReferenceEquals_CustomSignatures_NotSupported() |
There was a problem hiding this comment.
Educational: what do you mean here by NotSupported?
From the way it is implemented (using invocation.TargetMethod.Is(KnownType.System_Object, ... instead of the cheaper invocation.TargetMethod.Name == nameof(object.ReferenceEquals)) it seems to me that the code woulc correctly identify the local ReferenceEquals as being different than the object.ReferenceEquals.
As a result of that, it should not learn ObjectConstraint.True on argsX, yet it should learn ObjectConstraint.NotNull on them, because they are all bool and as such they can't be null.
So the assertion seems in line with the correct behavior.
What am I missing here?
If "unsupported" refers to the fact that the following assertion fails (behavior which is incorrect for me):
SETestContext.CreateCS($"""
object left = null;
object right = null;
var result = ReferenceEquals(left, right);
Tag("Result", result);
Tag("Left", left);
Tag("Right", right);
bool ReferenceEquals(object o1, object o2) => false;
""")
.Validator
.ValidateTag("Result", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
shouldn't the test rather ensure that the symbols of left and right passed to ReferenceEquals are not constrained, rather than asserting on the result of the custom ReferenceEquals implementations.
There was a problem hiding this comment.
Not supported means, that we will not simulate the correct behavior. Whatever "correct" means.
For object.RefernceEquals, we are sure how it behaves.
For user-defined ReferenceEquals, while we could assume the behavior based on the name, we just do not. We don't trust users.
So we don't learn anything at all from that invocation. The NotNull is learned only because it's bool type
| [DataTestMethod] | ||
| [DataRow("new object()", "new object()")] | ||
| [DataRow("new object()", "Unknown<object>()")] | ||
| [DataRow("Unknown<object>()", "new object()")] |
There was a problem hiding this comment.
Educational: this Unknown<object> is a generic static method returning default(T), which is used every time you want a symbol calculated on a method that is different from the one being analyzed, hence something on which you can't learn boolean constraints on (only the obvious fact that a bool is non-null).
Is that statement correct?
There was a problem hiding this comment.
Yes, it is.
It scaffolds a value that we cannot assume anything about. Once the analysis becomes cross-procedure, we will have to change the implementation from default to something more tricky. It now does the job only because we don't look inside.
| [DataRow("Unknown<object>()", "new object()")] | ||
| [DataRow("Unknown<object>()", "Unknown<object>()")] | ||
| [DataRow("new int?(42)", "Unknown<int?>()")] | ||
| [DataRow("(int?)42", "(int?)42")] |
There was a problem hiding this comment.
And here you can't learn between two boxed int because you can never know if they point to the same heap location or not.
More in general, you can't learn on value types passed to ReferenceEquals (the "anomalous" behavior reported here).
Is that correct?
If so, you should also have the same behavior for non-nullable structs, such as int, right? For example: [DataRow("42", "42")].
There was a problem hiding this comment.
We don't learn, because we don't even try. We only care about comparing to Null at this point.
We will eventually add support for some kind of NumberConstraint in the CBDE replacement, and on that one, we will be able to properly learn on Equals(42, 42). And we will have to change the implementation for RerefernceEquals to avoid learning, because of the reason you've mentioned.
pavel-mikula-sonarsource
force-pushed
the
Pavel/SE/ReferenceEquals
branch
from
3d63428
to
825c26f
Compare
pavel-mikula-sonarsource
force-pushed
the
feature/SE
branch
from
d1fa962
to
d6211d9
Compare
pavel-mikula-sonarsource
force-pushed
the
Pavel/SE/ReferenceEquals
branch
from
f09594e
to
cc10b80
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
SonarCloud Quality Gate failed. |
Part of #6997
object.ReferenceEquals(object, object)should behave the same asobject.Equals(object, object).While we would go further and learn more stuff, basic Null handling is enough to kill the noise.