-
Notifications
You must be signed in to change notification settings - Fork 464
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
Workaround hash collision between literals 'null' and 'false' in DFA #6876
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,7 +154,32 @@ protected override void ComputeHashCodeParts(ref RoslynHashCode hashCode) | |
protected override bool ComputeEqualsByHashCodeParts(CacheBasedEquatable<ValueContentAbstractValue> obj) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Can you review the complexity of changing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sharwell This is not viable as I found it's easy to still get collisions even when null isn't involved. See #6876 (comment) |
||
{ | ||
var other = (ValueContentAbstractValue)obj; | ||
return HashUtilities.Combine(LiteralValues) == HashUtilities.Combine(other.LiteralValues) | ||
if (LiteralValues.Count != other.LiteralValues.Count) | ||
{ | ||
return false; | ||
} | ||
|
||
var hashCode1 = new RoslynHashCode(); | ||
int nullCount1 = 0; | ||
foreach (var value1 in LiteralValues) | ||
{ | ||
hashCode1.Add(value1); | ||
if (value1 is null) | ||
nullCount1++; | ||
} | ||
|
||
var hashCode2 = new RoslynHashCode(); | ||
int nullCount2 = 0; | ||
foreach (var value2 in other.LiteralValues) | ||
{ | ||
hashCode2.Add(value2); | ||
if (value2 is null) | ||
nullCount2++; | ||
} | ||
|
||
// null and false can easily cause hash collisions and cause incorrect behaviors. | ||
// We count the nulls to workaround this collision. | ||
return nullCount1 == nullCount2 && hashCode1.ToHashCode() == hashCode2.ToHashCode() | ||
Comment on lines
+157
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit hacky. Not sure if it's to an acceptable extent or not. @mavasani Can you advise please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mavasani Can this implementation change completely to really compute "Equals" instead of relying on hash code? ie, rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A second thought on my fix. There could still be collisions between values:
So I think we either need proper equality or include the type in hash code calculation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirmed that collisions are still possible. Here is a test: var code = """
public class Test
{
public static void Method(string parameter)
{
var flag = GetFlag();
if (flag is false || flag is 0)
{
if (flag is 0)
{
}
}
}
public static object GetFlag() => null;
}
""";
await VerifyCS.VerifyCodeFixAsync(code, code); We need to either compute equals properly without hash code, or include the Type in our hash code calculation (but not sure if including There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the collection containing the values could be modified to store both the common type of all values, and the set of the values themselves. If the common type is null, then the type of each element needs to be included in the hashing.
|
||
&& NonLiteralState.GetHashCode() == other.NonLiteralState.GetHashCode(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Can you review the complexity of changing the internal hash code of
null
to a non-zero constant? I see two potential ways to handle this:NullSentinel
type with a singleton instance that returns a known hash code, and use that to represent nullOption (2) would be simpler, but relies on all calls to
GetHashCode()
(and all uses ofEqualityComparer<T>
) to have the same understanding of the hash code for null. Option (1) is more complex, but forces the correct result.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the options are viable as I found it's easy to still get collisions even when null isn't involved. See #6876 (comment)