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.

Sign up for GitHub

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: Caching: Cache "GetHashCode" for SymbolicValue #6968

Merged
merged 12 commits into from
Mar 27, 2023
Merged

SE: Caching: Cache "GetHashCode" for SymbolicValue #6968

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
65d1021
Add cache for SymbolicValue
martin-strecker-sonarsource Mar 21, 2023
49b5c30
Add copy constructor to reset "Lazy"
martin-strecker-sonarsource Mar 21, 2023
59cf2fb
Remove duplicate code
martin-strecker-sonarsource Mar 21, 2023
80c001e
Use nullable int instead on lazy
martin-strecker-sonarsource Mar 23, 2023
cbd6fae
Fix implementation
martin-strecker-sonarsource Mar 23, 2023
83d3aae
GetHashCode tests for SymbolicValue
martin-strecker-sonarsource Mar 23, 2023
389b423
Add more cases.
martin-strecker-sonarsource Mar 23, 2023
82134d2
Supress warning
martin-strecker-sonarsource Mar 23, 2023
ee16d48
Remove supression
martin-strecker-sonarsource Mar 23, 2023
0a5e74f
Formatting
martin-strecker-sonarsource Mar 23, 2023
bd2aa1a
Rename field
martin-strecker-sonarsource Mar 23, 2023
f3ef93d
Remove if
martin-strecker-sonarsource Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/SymbolicValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,22 @@ public sealed record SymbolicValue
public static readonly SymbolicValue True = NotNull.WithConstraint(BoolConstraint.True);
public static readonly SymbolicValue False = NotNull.WithConstraint(BoolConstraint.False);

private readonly ImmutableDictionary<Type, SymbolicConstraint> constraints = ImmutableDictionary<Type, SymbolicConstraint>.Empty;
private int? hashCode;

// SymbolicValue can have only one constraint instance of specific type at a time
private ImmutableDictionary<Type, SymbolicConstraint> Constraints { get; init; } = ImmutableDictionary<Type, SymbolicConstraint>.Empty;
private ImmutableDictionary<Type, SymbolicConstraint> Constraints
{
get => constraints;
init
{
if (constraints != value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is not covered when values are the same. We don't use it that way. And we actually shouldn't, as we should attempt no-operation when possible.

Do we really need the condition then?

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 doesn't hurt to keep it, IMO. It allows doing something like, e.g. with Constraints = someCond ? Empty : Constraints. I could add a test case to increase test coverage if that is a concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the condition. It's one more additional Equals check on a hot path that doesn't need to be there.

And so far, we didn't need it. And it's against the intended usage of the property. We can always add it back if we need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine with me.

{
constraints = value;
hashCode = null;
}
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}
}

public IEnumerable<SymbolicConstraint> AllConstraints =>
Constraints.Values;
Expand Down Expand Up @@ -83,7 +97,7 @@ public T Constraint<T>() where T : SymbolicConstraint =>
Constraints.TryGetValue(typeof(T), out var value) ? (T)value : null;

public override int GetHashCode() =>
HashCode.DictionaryContentHash(Constraints);
hashCode ??= HashCode.DictionaryContentHash(constraints);

public bool Equals(SymbolicValue other) =>
other is not null && other.Constraints.DictionaryEquals(Constraints);
Expand Down
41 changes: 41 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/SymbolicValueTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,5 +335,46 @@ public void RemoveEntry_Miss_Returns_Instance_Type()
var sut = SymbolicValue.Null.WithConstraint(TestConstraint.First);
sut.WithoutConstraint<DummyConstraint>().Should().BeSameAs(sut).And.HaveOnlyConstraints(ObjectConstraint.Null, TestConstraint.First);
}

[TestMethod]
public void GetHashCode_ReturnsDifferentValuesForPredefinedValues()
{
SymbolicValue.Empty.GetHashCode().Should().Be(SymbolicValue.Empty.GetHashCode());
SymbolicValue.Empty.GetHashCode().Should().NotBe(SymbolicValue.Null.GetHashCode())
.And.NotBe(SymbolicValue.NotNull.GetHashCode())
.And.NotBe(SymbolicValue.True.GetHashCode())
.And.NotBe(SymbolicValue.False.GetHashCode());
}

[TestMethod]
public void GetHashCode_DifferentValuesAreUnique()
{
new[]
{
SymbolicValue.Empty.GetHashCode(),
SymbolicValue.NotNull.GetHashCode(),
SymbolicValue.Null.GetHashCode(),
SymbolicValue.True.GetHashCode(),
SymbolicValue.False.GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).WithConstraint(TestConstraint.First).WithConstraint(ObjectConstraint.Null).GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).WithConstraint(TestConstraint.First).WithConstraint(ObjectConstraint.NotNull).GetHashCode(),
}.Should().OnlyHaveUniqueItems();
}

[TestMethod]
public void GetHashCode_UncachedValues()
{
var baseConstraint = SymbolicValue.Empty
.WithConstraint(DummyConstraint.Dummy)
.WithConstraint(TestConstraint.First)
.WithConstraint(ObjectConstraint.Null)
.WithConstraint(BoolConstraint.True);
baseConstraint.GetHashCode().Should().Be(baseConstraint.GetHashCode());
var similar = baseConstraint.WithoutConstraint(DummyConstraint.Dummy).WithConstraint(DummyConstraint.Dummy);
similar.Should().NotBeSameAs(baseConstraint);
similar.GetHashCode().Should().Be(baseConstraint.GetHashCode());
similar.Equals(baseConstraint).Should().BeTrue();
}
}
}