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.

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

Champion "permit t is null for unconstrained type parameter" (16.3, Core 3) #1284

Open
3 of 5 tasks
gafter opened this issue Jan 26, 2018 · 19 comments
Open
3 of 5 tasks
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Smallish Feature
Milestone

Comments

@gafter
Copy link
Member

gafter commented Jan 26, 2018

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

Currently it is an error to write if (t is null) ... when the type of t is a type parameter that is not constrained to be a reference type. We should permit this to allow people to write that instead of if (t == null) ... uniformly when testing for null values. A code fixer to do just that has to have a special case for this and it feels quite irregular. See dotnet/roslyn#24173. See also #1261.

The current spec requires that the constant expression null be converted to the type of the value being tested. That would have to be changed (for type parameters) to make this work.

Related to that, we should also permit t is 3 when the type of t is a type parameter.

More generally, when the object being tested is an open type, we treat it the same as if it is of type object for the purposes of legality of the pattern.

This was implemented in dev16: dotnet/roslyn#25995

@ghost
Copy link

ghost commented Jan 31, 2018

Good change, seems that v is null should help to avoid boxing allocations for some logic too.
It may be the key difference between v == null.

image

Compare
image
vs
image

Thank to JetBrains for the new feature "heap allocator". Actually analyzer suggest a suspicious code optimization for a generic parameter which cause boxing allocations with value types.

Reported to https://youtrack.jetbrains.com/issue/RIDER-13249

@CyrusNajmabadi
Copy link
Member

IsNull<T>(this T obj) => obj == null;

If you intend this to only be used on reference types (which would make sense given they're the only things that can be null), you can do IsNull<T>(this T obj) where T : class => obj == null; This will never cause boxing.

If you do want this to work on value types as well (not sure why..., but maybe you have some generic stuff that wants to work on both, but deal with null reference types specially), then you can write it as above and know that the CLR should optimize this such that your code will not box for value types.

@ghost
Copy link

ghost commented Jan 31, 2018

@CyrusNajmabadi
Yes, thank, I understand that it is possible to use where T : class and suggested logic is very rare. :)
I just think about the possible symmetry.

T v = ...

v is T /* no boxing */
v != null /* possible boxing */

v is null /* ? use non-boxing optimization [more expected for me] */
v == null /* possible boxing */

@ghost
Copy link

ghost commented Jan 31, 2018

@CyrusNajmabadi
In another words there are possible two implementations of IsNull method with the key difference in boxing.

public static bool IsNull<T>(this T o) => o == null; /* may cause boxing */
public static bool IsNull<T>(this T o) => o is T ? false : true; /* non-boxing */

I suspect that the new implementation will be equals to the second.

public static bool IsNull<T>(this T o) => o is null; /* non-boxing */

What do you think about it?

@CyrusNajmabadi
Copy link
Member

My point is that public static bool IsNull<T>(this T o) => o == null should never cause boxing. I believe the CLR is smart enough to look for that pattern and just convert it to 'false' immediately when instantiating with value types.

@ghost
Copy link

ghost commented Jan 31, 2018

@CyrusNajmabadi
It means that JetBrains heap allocator shows invalid hint.

@gafter
Copy link
Member Author

gafter commented Feb 1, 2018

@reinspi (previously known as Makeloft) Please stop posting in this repo. We are long past any interest in your equating extension methods with pattern-matching.

@gafter
Copy link
Member Author

gafter commented Feb 1, 2018

Added:

Related to that, we should also permit t is 3 when the type of t is a type parameter that is not constrained to be a reference type.

@DavidArno
Copy link

@gafter,

Related to that, we should also permit t is 3 when the type of t is a type parameter that is not constrained to be a reference type.

What would be the scope of this addition? Would it be limited to numbers, or could I do t is "hello" too, if T isn't constrained to a value type?

@yaakov-h
Copy link
Member

yaakov-h commented Feb 1, 2018

So this is interesting... for anyone following along at home:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Attributes.Jobs;
using BenchmarkDotNet.Running;

namespace NullBenchmark
{
    class Program
    {
        static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run<IsNullVersusIsT>();
        }
    }

    static class NullCheckExtensions
    {
        public static bool IsNull<T>(this T t) => t == null;
        public static bool IsT<T>(this T t) => t is T;
    }

    [RyuJitX64Job]
    [LegacyJitX86Job]
    [MemoryDiagnoser]
    public class IsNullVersusIsT
    {
        [Benchmark]
        public bool IsNull() => 42.IsNull();

        [Benchmark]
        public bool IsT() => 42.IsT();
    }
}

Results on .NET Framework:

 Method |          Job |       Jit | Platform |      Mean |     Error |    StdDev |  Gen 0 | Allocated |
------- |------------- |---------- |--------- |----------:|----------:|----------:|-------:|----------:|
 IsNull | LegacyJitX86 | LegacyJit |      X86 | 0.0000 ns | 0.0000 ns | 0.0000 ns |      - |       0 B |
    IsT | LegacyJitX86 | LegacyJit |      X86 | 1.4537 ns | 0.0082 ns | 0.0073 ns | 0.0038 |      12 B |
 IsNull |    RyuJitX64 |    RyuJit |      X64 | 0.0087 ns | 0.0021 ns | 0.0018 ns |      - |       0 B |
    IsT |    RyuJitX64 |    RyuJit |      X64 | 0.0053 ns | 0.0039 ns | 0.0036 ns |      - |       0 B |

Results on CoreCLR (and slower CPU):

 Method |      Mean |     Error |    StdDev | Allocated |
------- |----------:|----------:|----------:|----------:|
 IsNull | 0.0416 ns | 0.0316 ns | 0.0280 ns |       0 B |
    IsT | 0.0578 ns | 0.0342 ns | 0.0303 ns |       0 B |

It would appear that t == null never boxes, but t is T does on the old Framework JIT only.

I have no idea why t == null on the old framework JIT is completely flatlined, though.

@gafter
Copy link
Member Author

gafter commented Feb 1, 2018

@DavidArno

What would be the scope of this addition? Would it be limited to numbers, or could I do t is "hello" too, if T isn't constrained to a value type?

This is specifically for the number '3' 😉

Seriously, the proposal is to permit a constant pattern of any type when the value to be matched is an "open type".

@gafter
Copy link
Member Author

gafter commented Apr 12, 2018

Fixed in dotnet/roslyn#25995 for C# 8.0.

@yaakov-h
Copy link
Member

Is 8.0 the next expected release after 7.3?

@SIkebe
Copy link

SIkebe commented Apr 12, 2018

@yaakov-h #200 (comment)

@dasMulli
Copy link

Should this only be allowed in C# 8? The current compiler happily compiles bool IsNull<T>(T v) => v is null with LangVersion 7.2 as reported on StackOverflow: https://stackoverflow.com/questions/56153403/using-the-is-operator-with-unconstrained-generics/56155441#56155441 (tried myself with 2.2.203 SDK and a LangVersion 7.2 netcoreapp2.2).

The problem this created for the OP is that their C# 7.2 program developed in VS 2019 does not build on a 2017 build server.

@333fred
Copy link
Member

333fred commented May 15, 2019

@dasMulli see dotnet/roslyn#34678

@gafter
Copy link
Member Author

gafter commented May 16, 2019

@dasMulli It was only intended to be permitted in C# 8. I will investigate and file a bug and fix VS2019 (to not allow this) if needed.

@gafter
Copy link
Member Author

gafter commented May 16, 2019

@dasMulli That was a bug that was fixed in dotnet/roslyn#34695.

@gafter gafter closed this as completed May 16, 2019
@gafter gafter reopened this May 16, 2019
@dasMulli
Copy link

Thanks! I was only searching for CS0403 and couldn't find the issue or PR.

@jcouv jcouv changed the title Champion "permit t is null for unconstrained type parameter" Champion "permit t is null for unconstrained type parameter" (16.3, Core 3) Jul 18, 2019
@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Smallish Feature
Projects
None yet
Development

No branches or pull requests

8 participants