roslyn: Visual Studio Code-Fix for generating GetHashCode results in OverflowException in checked assembly
Version Used: Microsoft Visual Studio Enterprise 2017 Version 15.5.2
Steps to Reproduce:
- Create a new project with “Check for arithmetic overflow/underflow” enabled in the project Build settings
- Add the following class:
class Foo
{
object bar;
string baz;
}
- Place the cursor immediately after
Foo
- Press Ctrl+. (period)
- Select “Generate Equals and GetHashCode()…”
- Create a new
Foo
object and callGetHashCode()
Full code:
using System.Collections.Generic;
namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
new Foo().GetHashCode();
}
}
class Foo
{
object bar;
string baz;
public override bool Equals(object obj)
{
var foo = obj as Foo;
return foo != null &&
EqualityComparer<object>.Default.Equals(bar, foo.bar) &&
baz == foo.baz;
}
public override int GetHashCode()
{
var hashCode = -1438245972;
hashCode = hashCode * -1521134295 + EqualityComparer<object>.Default.GetHashCode(bar);
hashCode = hashCode * -1521134295 + EqualityComparer<string>.Default.GetHashCode(baz);
return hashCode;
}
}
}
Expected Behavior:
The compiled program exits with code 0, printing no output.
Actual Behavior:
The compiled program crashes with a System.OverflowException
:
System.OverflowException
HResult=0x80131516
Message=Arithmetic operation resulted in an overflow.
Source=ConsoleApp1
StackTrace:
at ConsoleApp1.Foo.GetHashCode() in C:\temp\ConsoleApp1\ConsoleApp1\Program.cs:line 29
at ConsoleApp1.Program.Main(String[] args) in C:\temp\ConsoleApp1\ConsoleApp1\Program.cs:line 9
I expect that the IDE code-fix should wrap the method body in an unchecked
block when the assembly is checked.
About this issue
- Original URL
- State: closed
- Created 6 years ago
- Comments: 36 (29 by maintainers)
Does many years of flip-flopping really count as experience 😁 (In my defence, there’s no perfect balance between simplicity, speed of execution and quality of hash, especially in general cases that code generation has to deal with, so I don’t feel too bad for moving between those).
As far as I can reason, I think you’re right about xor and shift.
Using
System.HashCode.Combine
where possible would IMO be ideal, but that of course isn’t always available.I’d consider:
This make rotating the bits an intermediary step, between each addition of an affecting property, so it avoids both the issues around XOR alone and around losing bits due to shifting in just one direction.
There might be better ideas to be gleaned from the discussions that happened at https://github.com/dotnet/corefx/issues/14354 and https://github.com/dotnet/corefx/issues/8034 (though they didn’t have the no-overflow requirement). And better input from those who contributed to that work.
I think it’s enough that someone could be able to look a the code and say “that should certainly mix up the bits, I hope it mixes them well” rather than being able to understand every nuance. (Even the very well known multiply-by-31 isn’t fully understood AFAIK having seen a study [alas I can’t find it] that found it worked better in practice than multiplying by other Mersenne primes, but the authors couldn’t explain why).