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

IntPtr is (silently without compile error) not comparable to null... what to do? #31456

Closed
jjxtra opened this Issue Jul 29, 2018 · 14 comments

Comments

Projects
None yet
8 participants
@jjxtra

jjxtra commented Jul 29, 2018

IntPtr is the only struct that I know of that can compare to null. The comparison is always false, even for IntPtr.Zero. The following code should be a compile error, just as with any other struct:

bool result = (new IntPtr(0) == null);

This code compiles and is always false.

Let me know if this belongs in a different repo, wasn't sure if this was a compiler or framework issue...

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

I'm guessing this would be too much of a breaking change to make now. @jkotas thoughts?

        public unsafe override bool Equals(object obj)
        {
            if (obj is IntPtr)
            {
                return (_value == ((IntPtr)obj)._value);
            }
            return false;
        }
@jjxtra

This comment has been minimized.

jjxtra commented Jul 29, 2018

Any code doing such comparison is effectively broken, and will fail as in this example where an IntPtr.Zero sneaks through. So I don't see a problem with this:

public unsafe override bool Equals(object obj)
{
    if (obj is IntPtr)
    {
        return (_value == ((IntPtr)obj)._value);
    }
    return (_value == 0 && obj == null);
}

Failing that, I think making this a compile error would be idea. Not sure how that would work with the Equals override though...

@jjxtra jjxtra changed the title from IntPtr should not be comparable to null, any attempt should be compile error to IntPtr is not comparable to null... what to do? Jul 29, 2018

@jjxtra jjxtra changed the title from IntPtr is not comparable to null... what to do? to IntPtr is (silently without compile error) not comparable to null... what to do? Jul 29, 2018

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

Note that the compiler is just eliminating the comparison eg

            IntPtr ptr = IntPtr.Zero;
            Console.WriteLine(ptr == null);
            Console.WriteLine(ptr != null);

becomes in IL

	IntPtr ptr = IntPtr.Zero;
	Console.WriteLine(false);
	Console.WriteLine(true);
@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

Any code doing such comparison is effectively broken,

It could certainly still be a breaking change to change such behavior. Plenty of times apps are running happily with some "incorrect" code and changing that can cause them to suddenly mysteriously stop working which can block upgrade. I'm not offering an opinion on whether that risk is acceptable (it's a little easier for .NET Core as it can be installed side by side).

@sharwell

This comment has been minimized.

Member

sharwell commented Jul 29, 2018

If you enable the "strict" feature of the compiler, the original code produces warning CS8073:

The result of the expression is always 'false' since a value of type 'IntPtr' is never equal to 'null' of type 'IntPtr?'

You can enable the "strict" feature like this:
https://github.com/Microsoft/perfview/blob/29788fe1dc799dc317d0a90f46bfae4ac6ccd333/src/Directory.Build.props#L10

The comparison was technically prohibited by the language specification, but a bug in the old C# compiler resulted in the comparison silently evaluating to false. Due to the age of this bug and the near certainty of applications depending on it in one way or another, the original behavior was preserved by default and the correction (reporting the warning) is only enabled if you explicitly modify your project to request that it be treated as a warning.

@jjxtra

This comment has been minimized.

jjxtra commented Jul 29, 2018

Been doing C# for 15 years and never turned on that option. I might have known about it at one point, but if I did I forgot. But I could be the minority. Either way I'm turning it on for all my projects :)

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

It would be interesting to see what happens if we enable it globally in Corefx. Do you want to try @jjxtra?

@jjxtra

This comment has been minimized.

jjxtra commented Jul 29, 2018

@danmosemsft Sure, I've wanted to do a string pull request anyway, so this would be a great chance to do so. Can you point me to .NET core build / testing instructions?

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

Check the documentation folder, start with the developer guide document.

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

I think properties in here apply to all projects. Do a clean build after editing
https://github.com/dotnet/corefx/blob/master/src/Directory.Build.props

@tannergooding

This comment has been minimized.

Member

tannergooding commented Jul 29, 2018

CC. @jaredpar

@jjxtra

This comment has been minimized.

jjxtra commented Jul 29, 2018

Thanks for the info. Is there any way to load this up in Visual Studio and edit / compile from there? Or is it all command line?

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented Jul 29, 2018

Ypu can open individual projects in VS (15.7+) just fine. However I am not sure whether there is an up to date .sln file including all of them.

4creators added a commit to dotnetrt/coreclr that referenced this issue Jul 29, 2018

Fix invalid IntPtr == null comparisons, set strict mode for Roslyn
Issue reported in dotnet/corefx#31456
Solution is to compare always against IntPtr.Zero and use Roslyn
stric mode for reporting warnings for IntPtr == null comparisons

jkotas added a commit to dotnet/coreclr that referenced this issue Jul 30, 2018

Fix invalid IntPtr == null comparisons, set strict mode for Roslyn (#…
…19191)

Issue reported in dotnet/corefx#31456
Solution is to compare always against IntPtr.Zero and use Roslyn
stric mode for reporting warnings for IntPtr == null comparisons
@stephentoub

This comment has been minimized.

Member

stephentoub commented Sep 25, 2018

@danmosemsft, this can be closed now, right?

@karelz karelz added this to the 3.0 milestone Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment