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

[Tornhoof]

IDE0041 should only be shown if the type is nullable or the generic parameter is constrained to a nullable type.
Executing the codefix will create invalid code.

Version Used:
Visual Studio Enterprise 2017 15.5 RTM
Steps to Reproduce:

1. Use repro snippet from below
2. Build
3. Look at messages

Expected Behavior:
Not shown
Actual Behavior:
Message: IDE0041 Null check can be simplified is shown for the Reference Equals

Repro:

public static void NotNull<T>(T value, string parameterName)
{
    if (ReferenceEquals(value, null)) // IDE0041 not valid here
    {
        throw new ArgumentNullException(parameterName);
    }
}

[Zshazz]

My apologies for asking a question on an old issue, but I have recently run into this on VS Professional 15.8.5. I see that it should have been addressed in 15.7. Is that accurate? If so, there may have been a regression.

The following code still has suggestions for IDE0041 in all places (including the original repro above):

    public sealed class A
    {
        public bool Test<T>(T a) => ReferenceEquals(a, null); // IDE0041 not valid here

        public static void NotNull<T>(T value, string parameterName)
        {
            if (ReferenceEquals(value, null)) // IDE0041 not valid here
            {
                throw new ArgumentNullException(parameterName);
            }
        }
    }

    public class B<T>
    {
        public bool Test(T b) => ReferenceEquals(b, null); // IDE0041 not valid here
    }

[Logopher]

I also see this in 15.8.3, with a non-generic reference type.

[Neme12]

@sharwell I see it too. Can you reopen this?

[Neme12]

cc @MaStr11

[MaStr11]

For unconstrained generic parameters, we should offer to simplify to == null (instead of the invalid is null). See the test here:

roslyn/src/EditorFeatures/CSharpTest/UseIsNullCheck/UseIsNullCheckForReferenceEqualsTests.cs

Lines 223 to 248 in 9892220

	[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)]
	public async Task TestValueParameterTypeIsUnconstrainedGeneric()
	{
	await TestInRegularAndScriptAsync(
	@"
	class C
	{
	public static void NotNull<T>(T value)
	{
	if ([||]ReferenceEquals(value, null))
	{
	return;
	}
	}
	}
	", @"
	class C
	{
	public static void NotNull<T>(T value)
	{
	if (value == null)
	{
	return;
	}
	}
	}

This behavior was discussed on the PR that fixed this issue:
#24173 (comment)

It seems that IDE0041 is properly reported, but the CodeAction to do the fix does nothing:

This might already be fixed by @CyrusNajmabadi in commit Fix conflict between fixers that was preventing one from showing up. 856932a#diff-f2c45bae9d7dfdbb5253223b6ecfdb48

because if I add the code from above to a test in master MaStr11@77c29cd the test is green (it does the right thing, without any other changes). @CyrusNajmabadi Can you confirm that the current behavior in 15.8.5 is already fixed in master?

PS: It seems the special caseing (== null instead of is null) can be removed soon because #25995 is merged and fixes dotnet/csharplang#1284.

[Neme12]

This might already be fixed by @CyrusNajmabadi in commit Fix conflict between fixers that was preventing one from showing up. 856932a#diff-f2c45bae9d7dfdbb5253223b6ecfdb48

You're right, I just checked and this is fixed in master. Although it's a little weird that the code fix title says "Use 'is null' check". In my opinion it should say "Use '== null' check" here.

PS: It seems the special caseing (== null instead of is null) can be removed soon because #25995 is merged and fixes dotnet/csharplang#1284.

I'm assuming that the language feature would only be enabled for C# 8. If that's the case, this logic cannot be removed. It still has to work this way if somebody has LangVersion set to 7.3 or below.

[MaStr11]

Thanks for checking, that it is fixed in master. I couldn't check it myself because I have trouble getting my Roslyn experimental hive working (once more).

In my opinion it should say "Use '== null' check" here.

That's right. I think we did so because this is temporary until C# 8 is released and it's an edge case that doesn't come up that often. The title already distinguishes between is null and not is null and could be extended to also support == and !=.

roslyn/src/Features/Core/Portable/UseIsNullCheck/AbstractUseIsNullForReferenceEqualsCodeFixProvider.cs

Line 40 in 9892220

var title = negated ? GetIsNotNullTitle() : GetIsNullTitle();

I'm assuming that the language feature would only be enabled for C# 8. If that's the case, this logic cannot be removed. It still has to work this way if somebody has LangVersion set to 7.3 or below.

Right.

[Neme12]

I think we did so because this is temporary until C# 8 is released

It is "sort of" temporary. C# 7 will not change and many users will (as always) continue using projects that have to support building in VS 2017, or even 2015 and avoid using C# 8 for a few years. It would be nice if the title was fixed, because this special case is not going to get removed any time soon.

[sharwell]

Closing this as the original bug was still fixed as described. Regressions can be opened as a new issue. Otherwise it makes a mess of the history and links between PRs and issues.

[sharwell]

PS: It seems the special caseing (== null instead of is null) can be removed soon ...

I believe this would need a check for the LangVersion property of the project, but that's pretty normal by now.

You're right, I just checked and this is fixed in master.

Based on this, I don't think we need to create a new issue, but we can if people want.

[CyrusNajmabadi]

I'm fine with this case having a slightly incorrect title. :)

[CyrusNajmabadi]

@CyrusNajmabadi Can you confirm that the current behavior in 15.8.5 is already fixed in master?

It works for me in master. So things seem fine :)

[NN---]

Still not fixed in 15.8.7:

Several samples:

		public T Value
		{
			get
			{
				var some = this as Some;
				Code.AssertState(!ReferenceEquals(some, null), "Option has no value."); // IDE0041
				return some.Value;
			}
		}

		public bool Equals(Option<T> other)
		{
			if (other == null)
				return false;

			var thisSome  = this as Some;
			var otherSome = other as Some;

			if (ReferenceEquals(thisSome, null)) // IDE0041
				return ReferenceEquals(otherSome, null); // IDE0041

			if (ReferenceEquals(otherSome, null)) // IDE0041
				return false;

			return EqualityComparer<T>.Default.Equals(Value, other.Value);
		}

[TheBeardedLlama]

Yep I'm getting the same

Looks like we need a new issue and a proper explanation of the situation

[Neme12]

What is Some? Can you please make a full code example?

[NN---]

https://github.com/rsdn/CodeJam/blob/af4d33847cf71e6b8f0d8e11b1d5a9edd0d76ad9/Main/src/Structures/Option/Option%601.cs

You can open CodeJam.sln in this project and see all these warnings .

[jmarolf]

I believe this is fixed in 15.9

[abelbraaksma]

I believe this is fixed in 15.9

Nope, just tested 15.9.0 and it is still the same. Also, the auto-correction of the suggested fix does nothing, it leaves the code untouched, even for the simplest cases. It currently says "is null" and not "== null".

Edit: it seems that negation of the message doesn't work either, it doesn't say "is not null", nor "!= null":

And considering the discussion above, I am not sure whether replacing it with null == other is actually the preferred edit here, but I have to admit that I am not aware of the full set of changes of C# 8, let alone whether changing this would hamper if the same code gets compiled with C# 7 as well.

[jmarolf]

@abelbraaksma the correct thing is to do nothing here. At least we do not apply the codefix. We should update the bug to say that the problem is the codefix being offered at all. Looks like someone added some logic so it wasn't applied anymore

[jmarolf]

@CyrusNajmabadi looks like this was regressed in 15.9

[CyrusNajmabadi]

Works for me on the latest bits:

I can see the fix isn't in 15.9: https://github.com/dotnet/roslyn/blob/dev15.9.x/src/Features/Core/Portable/UseIsNullCheck/AbstractUseIsNullForReferenceEqualsCodeFixProvider.cs

Specifically, it's missing these lines:

roslyn/src/Features/Core/Portable/UseIsNullCheck/AbstractUseIsNullForReferenceEqualsCodeFixProvider.cs

Lines 31 to 32 in 76c02fe

	private static bool IsSupportedDiagnostic(Diagnostic diagnostic)
	=> diagnostic.Properties[UseIsNullConstants.Kind] == UseIsNullConstants.ReferenceEqualsKey;

So this is simply a case of #29458 not being included in 15.9 for some reason.

@jmarolf I don't know how i can help here. The bug has been fixed. Roslyn has just chosen to not release it yet. Let me know if you need any more info. Thanks!

[CyrusNajmabadi]

Closing out. Have validated this is working in the 16.x line.