Suppress 'IDE0041 Null check can be simplified' for unconstraint generic parameters. #24173

MaStr11 · 2018-01-11T12:12:29Z

Customer scenario

IDE0041 Null check can be simplified is shown for the ReferenceEquals method and a fix is offered. The fix generates code which might have the compiler error 'Can not convert null to type parameter T because it could be a non-nullable value type.'. This fix suppresses IDE0041 if the parameter passed to ReferenceEquals is ~~an unconstraint generic type parameter~~ Update 01/17/2018: a value type constraint generic type parameter.

Bugs this fixes

#23581

Workarounds, if any

Suppress IDE0041 with a pragma or change the generated code from (value is null)to (value == null).

Risk

Low. Small additional check.

Performance impact

Low. Syntax and semantic analysis has already been done by other checks on the code path before. This adds some more checks.

Is this a regression from a previous update?

/

Root cause analysis

/

How was the bug found?

Customer reported #23581

Test documentation updated?

/

MaStr11 · 2018-01-11T12:17:14Z

Another option might be to generate (value == null) instead of (value is null). VBs value Is Nothing seems also be fine with unconstraint parameters.

MaStr11 · 2018-01-12T09:12:14Z

@CyrusNajmabadi

Another option might be to generate (value == null) instead of (value is null). VBs value Is Nothing seems also be fine with unconstraint parameters.

This seems to be the better option (I just discovered this option after I finished the PR). Are there any downsides?

CyrusNajmabadi · 2018-01-12T19:44:44Z

This seems to be the better option (I just discovered this option after I finished the PR). Are there any downsides?

Not that i can tell. Make it so!

MaStr11 · 2018-01-15T14:05:47Z

Oh my gosh! I just read dotnet/csharplang#505 and discovered that (v == null) fails for value type constraints:

public void TheFunction<T>(T a) where T : struct {
  var c = (a != null); // CS0019: Operator '!=' cannot be applied to operand of type 'T' and '<null>'
}

Public Sub TheFunction(Of T As Structure)(a As T)
    If a Is Nothing Then 'BC30020: 'Is' operator does not accept operands of type 'T'. Operands must be reference or nullable types.
    End If
End Sub

I will update the PR to suppress on value types now.

CyrusNajmabadi · 2018-01-15T18:47:15Z

src/EditorFeatures/CSharpTest/UseIsNullCheck/UseIsNullCheckTests.cs

@@ -36,7 +36,7 @@ class C
 {
    void M(string s)
    {
-        if (s is null)
+        if (s == null)


this seems like a regression.

CyrusNajmabadi · 2018-01-15T18:47:54Z

src/Features/CSharp/Portable/UseIsNullCheck/CSharpUseIsNullCheckCodeFixProvider.cs

@@ -17,12 +17,16 @@ protected override string GetIsNullTitle()
        protected override string GetIsNotNullTitle()
            => GetIsNullTitle();


the feature is literall the "Use 'is null'" feature :) You're now using "== null".

Yes. But I thought we agreed on that. Did I got you wrong?

CyrusNajmabadi · 2018-01-15T19:13:14Z

Did I got you wrong?

Yes (my fault). I meant specifically using '==' only for the cases where 'is' wouldn't work.

CyrusNajmabadi · 2018-01-15T19:14:21Z

basically:

use "is null" whenever possible.
fall back to "== null" when '1' isn't possible. (or possibly just suppress).
if neither are possible, offer nothing.

MaStr11 · 2018-01-15T19:16:29Z

That's fine to me (I'm on the road tomorrow so this will not be done until Wednesday. Sorry.)

CyrusNajmabadi · 2018-01-15T19:26:20Z

No worries :)

MaStr11 · 2018-01-17T12:29:31Z

I changed the logic as requested by @CyrusNajmabadi. I was wondering if it is worth to document somewhere that this fixer can be simplified once this is done:

@gafter

We should change the language to permit value is null for an unconstrained generic.

There seems to be no issue in the language repo yet to link to.

CyrusNajmabadi · 2018-01-17T19:08:15Z

There seems to be no issue in the language repo yet to link to.

you could make one :) Then link to that in the code with a note.

CyrusNajmabadi · 2018-01-17T19:09:16Z

src/Features/CSharp/Portable/UseIsNullCheck/CSharpUseIsNullCheckCodeFixProvider.cs

            => ((PrefixUnaryExpressionSyntax)notExpression).WithOperand((ExpressionSyntax)CreateIsNullCheck(argument));
+
+        protected override SyntaxNode CreateNullCheck(SyntaxNode argument, bool isUnconstraintGeneric)


Unconstrained (though Unconstraint is cute :))

CyrusNajmabadi · 2018-01-17T19:12:18Z

src/Features/Core/Portable/UseIsNullCheck/AbstractUseIsNullDiagnosticAnalyzer.cs

@@ -108,9 +119,18 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol refe
                return;
            }

+            var properties = ImmutableDictionary<string, string>.Empty;
+
+            switch (GetGenericParameterConstraintType(syntaxFacts, semanticModel, arguments[0], arguments[1], cancellationToken))


a simpler approach seems like it would be if you just had this method return the type parameter if there is one.

then you could do:

if (typeParameter?.HasValueConstraint == true) { return; } else if (...?.HasReferenceConstraint == true) { ... }

Then there's no need for the enum to shuttle data across that's already in the ITypeParameter.

CyrusNajmabadi

Overall logic LGTM. I think there are some things that can be simplified though.

sharwell · 2018-02-14T16:27:32Z

@Pilchie Requesting approval for 15.7 before we do the work to rebase this 😄

Pilchie · 2018-02-14T17:21:35Z

I'm in favor of fixing false positive diagnostic results as early as possible, so I would be supportive of this for 15.7.

sharwell · 2018-02-14T18:05:00Z

@MaStr11 This pull request needs to be rebased onto the dev15.7.x branch so we can get it to users earlier. Let me know if you'd like to do this yourself, or if you'd like me to go ahead and do it for you. 👍

MaStr11 · 2018-02-14T20:40:16Z

@sharwell I changed the base branch but it seems there are some unrelated commits are also part of the PR. I'm not able to fix (because I don't know how to do it and because I'm currently on vacation and don't have my laptop with me). If there is anything that needs to be done it would be great if you could do it.

sharwell · 2018-02-14T20:50:34Z

@MaStr11 Thanks I'm on it!

…agnostic.

…ll) to (v == null).

case 'type constraint': no fix offered case 'reference constraint': C# use '==null' instead of 'is null' default: C# 'is null', VB 'IsNothing'

sharwell · 2018-02-14T23:28:30Z

@Pilchie for approval

Zenexer · 2019-08-02T15:05:38Z

Another option might be to generate (value == null) instead of (value is null). VBs value Is Nothing seems also be fine with unconstraint parameters.

I know I'm late to the game here, but this needs to be documented: do not do this! It's is not the same thing. A ReferenceEquals comparison to null often occurs in the Equals override of a type, and that is in turn often referenced from operator == and operator !=. value == null will create an infinite loop.

MaStr11 requested a review from a team as a code owner January 11, 2018 12:12

CyrusNajmabadi approved these changes Jan 11, 2018

View reviewed changes

CyrusNajmabadi reviewed Jan 15, 2018

View reviewed changes

jinujoseph assigned sharwell Jan 16, 2018

CyrusNajmabadi reviewed Jan 17, 2018

View reviewed changes

CyrusNajmabadi approved these changes Jan 17, 2018

View reviewed changes

MaStr11 mentioned this pull request Jan 18, 2018

Proposal: Permit 'value is null' if 'value' is unconstrained generic dotnet/csharplang#1261

Closed

gafter mentioned this pull request Jan 26, 2018

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

Open

5 tasks

sharwell approved these changes Feb 14, 2018

View reviewed changes

sharwell changed the base branch from master to dev15.7.x February 14, 2018 16:26

sharwell changed the base branch from dev15.7.x to master February 14, 2018 16:27

MaStr11 changed the base branch from master to dev15.7.x February 14, 2018 20:34

sharwell changed the base branch from dev15.7.x to master February 14, 2018 20:50

MaStr11 and others added 8 commits February 14, 2018 14:52

Add HasUnconstraintGenericParameter check to suppress reporting of di…

2e2cbb6

…agnostic.

Removed generic constraint check. Changed C# null check from (v is nu…

f67fb25

…ll) to (v == null).

Added check for value type constraint parameter.

24fddff

Changed logic for generic parameter constraints:

f94eaa9

case 'type constraint': no fix offered case 'reference constraint': C# use '==null' instead of 'is null' default: C# 'is null', VB 'IsNothing'

Simplify handling of type constraints.

5145252

Added code comment with link to dotnet/csharplang#1261.

1979ff8

Spelling correction

9bb2dd8

Changed code comment link to dotnet/csharplang#1284.

bffe221

sharwell force-pushed the SuppressIDE0041_UseIsNullcheck_ForUnconstraintGenericParameters branch from 1f1a266 to bffe221 Compare February 14, 2018 20:53

MaStr11 requested review from a team as code owners February 14, 2018 20:53

sharwell changed the base branch from master to dev15.7.x February 14, 2018 20:53

sharwell approved these changes Feb 14, 2018

View reviewed changes

sharwell removed request for a team February 14, 2018 23:28

sharwell added this to the 15.7 milestone Feb 14, 2018

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 14, 2018

Pilchie added the Approved to merge label Feb 14, 2018

sharwell merged commit 367b725 into dotnet:dev15.7.x Feb 14, 2018

MaStr11 deleted the SuppressIDE0041_UseIsNullcheck_ForUnconstraintGenericParameters branch March 2, 2018 15:17

Neme12 mentioned this pull request Mar 19, 2018

Inccorect 'Null check can be simplified' #25588

Closed

MaStr11 mentioned this pull request Oct 3, 2018

IDE0041 Null check can be simplified is shown for Reference Equals of all generic parameters #23581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress 'IDE0041 Null check can be simplified' for unconstraint generic parameters. #24173

Suppress 'IDE0041 Null check can be simplified' for unconstraint generic parameters. #24173

MaStr11 commented Jan 11, 2018 •

edited

Loading

MaStr11 commented Jan 11, 2018

MaStr11 commented Jan 12, 2018

CyrusNajmabadi commented Jan 12, 2018

MaStr11 commented Jan 15, 2018 •

edited

Loading

CyrusNajmabadi Jan 15, 2018

CyrusNajmabadi Jan 15, 2018

MaStr11 Jan 15, 2018

CyrusNajmabadi commented Jan 15, 2018

CyrusNajmabadi commented Jan 15, 2018

MaStr11 commented Jan 15, 2018

CyrusNajmabadi commented Jan 15, 2018

MaStr11 commented Jan 17, 2018

CyrusNajmabadi commented Jan 17, 2018

CyrusNajmabadi Jan 17, 2018

CyrusNajmabadi Jan 17, 2018

CyrusNajmabadi left a comment

sharwell commented Feb 14, 2018

Pilchie commented Feb 14, 2018

sharwell commented Feb 14, 2018

MaStr11 commented Feb 14, 2018

sharwell commented Feb 14, 2018

sharwell commented Feb 14, 2018

Zenexer commented Aug 2, 2019

		@@ -17,12 +17,16 @@ protected override string GetIsNullTitle()
		protected override string GetIsNotNullTitle()
		=> GetIsNullTitle();

		=> ((PrefixUnaryExpressionSyntax)notExpression).WithOperand((ExpressionSyntax)CreateIsNullCheck(argument));

		protected override SyntaxNode CreateNullCheck(SyntaxNode argument, bool isUnconstraintGeneric)

Suppress 'IDE0041 Null check can be simplified' for unconstraint generic parameters. #24173

Suppress 'IDE0041 Null check can be simplified' for unconstraint generic parameters. #24173

Conversation

MaStr11 commented Jan 11, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

MaStr11 commented Jan 11, 2018

MaStr11 commented Jan 12, 2018

CyrusNajmabadi commented Jan 12, 2018

MaStr11 commented Jan 15, 2018 • edited Loading

CyrusNajmabadi Jan 15, 2018

Choose a reason for hiding this comment

CyrusNajmabadi Jan 15, 2018

Choose a reason for hiding this comment

MaStr11 Jan 15, 2018

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 15, 2018

CyrusNajmabadi commented Jan 15, 2018

MaStr11 commented Jan 15, 2018

CyrusNajmabadi commented Jan 15, 2018

MaStr11 commented Jan 17, 2018

CyrusNajmabadi commented Jan 17, 2018

CyrusNajmabadi Jan 17, 2018

Choose a reason for hiding this comment

CyrusNajmabadi Jan 17, 2018

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

sharwell commented Feb 14, 2018

Pilchie commented Feb 14, 2018

sharwell commented Feb 14, 2018

MaStr11 commented Feb 14, 2018

sharwell commented Feb 14, 2018

sharwell commented Feb 14, 2018

Zenexer commented Aug 2, 2019

MaStr11 commented Jan 11, 2018 •

edited

Loading

MaStr11 commented Jan 15, 2018 •

edited

Loading