Visual Studio Code-Fix for generating GetHashCode results in OverflowException in checked assembly

[yaakov-h]

Version Used:
Microsoft Visual Studio Enterprise 2017
Version 15.5.2

Steps to Reproduce:

1. Create a new project with "Check for arithmetic overflow/underflow" enabled in the project Build settings
2. Add the following class:

class Foo
{
    object bar;
    string baz;
}

3. Place the cursor immediately after Foo
4. Press Ctrl+. (period)
5. Select "Generate Equals and GetHashCode()..."
6. Create a new Foo object and call GetHashCode()

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.

[MaStr11]

Duplicate of #24093

[sharwell]

@MaStr11 I went the other way around with the duplicates, but thanks for pointing it out 👍

[MaStr11]

Comment copied from #24093

This is the method than needs to be changed:
ICodeDefinitionFactoryExtensions_CreateGetHashCodeMethod.cs
This method currently doesn't mentions overflows in checked environments at all. @CyrusNajmabadi did most of the changes in commit f6eaea6. The history of this goes back until before Roslyn was open sourced. He may can answer why that problem wasn't taken into account.

Albeit there is a problem with VB to consider: It doesn't support checked/unchecked (see the stackoverflow thread Overriding GetHashCode in VB without checked/unchecked keyword support? for details). I think that's also why there aren't any Checked or Unchecked methods on SyntaxGenerator.

IMHO this is best solved by providing language specific implementations. And I fear that the VB version would be quite different from the C# version and needs to be well designed before it can be implemented here. There are some implementations given in the stackoverflow thread above but some of them can't be used (need an extra struct), are using a long as intermediate value or might be not well balanced.

[CyrusNajmabadi]

He may can answer why that problem wasn't taken into account.

I didn't think of it when i wrote it originally :)

[MaStr11]

Related dotnet/corefx#25013. The fixer could generate calls to System.HashCode.Combine on supported (future) platforms.

[CyrusNajmabadi]

Yes. The corefx bug was originally pushed on (by us) for this precise reason :)

[CyrusNajmabadi]

See: #17646

[yaakov-h]

As far as I can tell, that only addresses the issue when targeting >= .NET Core 2.1 and/or some hypothetical future version of .NET Framework / Mono / Xamarin.

Is there a (reasonable) way to fix this for the runtimes that are in use today?

[MaStr11]

Is there a (reasonable) way to fix this for the runtimes that are in use today?

I see two possible approaches:

Keep the existing algorithm for VB and C#

This would require language specific additions to the currently generated MethodStatements:
C#

unchecked
{
  // current MethodStatements
}

VB

' The following implementation relies on integer overflows. Make sure to 
' compile your assembly with /removeintchecks+ compiler option
' https://docs.microsoft.com/en-us/dotnet/visual-basic/reference/command-line-compiler/removeintchecks
' current MethodStatements

Use an algorithm that doesn't relies on overflows

This will likely be some combination of shift and xor. Given the discussions in the corefx repo (dotnet/corefx#8034 Adding a HashCode type to help with combining hash codes, and the follow up dotnet/corefx#14354) with more than 350 comments it seems difficult to find a consensuses what a good algorithm might look like.

Just to get started. This would be my list of requirements (derived from the currently generated code)

• It is compact (one line per member, plus one line for init of a seed and the return statement)
• It is easy to exclude members from the hash by deleting the line
• It is language independent (does not rely on C# only or VB only language features)
• It has reasonable hashing quality (whatever that means)
• Is readily understandable and self-explanatory without being too mystical
• Avoids NREs on member access

I'm by no means an hash algorithm expert but I looked around at stackoverflow to find an algorithm that fits to the requirements above and it seems xor and shift might be a fit. That is based on a comment by @JonHanna that is somewhat buried in a longer thread (emphases by me):

A simple shift on one of the values before xoring is only marginal extra cost for a good reduction in collision. Prime-number multiplication increasing both time and quality again. Which is better between shift or mult is hence debatable. Plain xor though very often has a lot of collisions on real data and is best avoided

But there is a catch (Source MSDN Object.GetHashCode):

A second alternative solution involves weighting the individual hash codes by left-shifting the hash codes of successive fields by two or more bits. Optimally, instead of being discarded, bits shifted beyond bit 31 should wrap around rather than be discarded. Since bits are discarded by the left-shift operators in both C# and Visual Basic, this requires creating a left shift-and-wrap method like the following: