dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

Null-Propagation Quick-Fix Suggests Invalid Fix #16114

Closed 333fred closed 7 years ago

333fred commented 7 years ago

Version Used: d15prerel/26022.07

Simple example:

namespace ConsoleApp1
{
    class Base
    {
        public Base(int i) { }
    }

    class Program : Base
    {
        public Program(string a) :
            base(a != null ? a.Length : throw new ArgumentNullException(nameof(a)))
        {
        }
    }
}

The IDE suggests moving base(a != null ? a.Length : throw new ArgumentNullException(nameof(a))) to use null propagation as follows: base(a?.Length). This changes behavior, removing the ArgumentNullException, and it also no longer compiles, as a?.Length is an int?, not an int.

/cc @Pilchie @dpoeschl

alrz commented 7 years ago

It probably should be a?.Length ?? throw new ArgumentNullException(nameof(a)).

333fred commented 7 years ago

No, that also changes behavior. Length isn't going to return null, but you can easily imagine a different property that returns null, even if the original object isn't null.

alrz commented 7 years ago

@333fred When you use ?? the nullable type will be unwrapped, and in this case, it also compiles away.

333fred commented 7 years ago

@alrz that doesn't stop the change of behavior. If the original object isn't null, but the property is, a?.Length ?? throw new ArgumentNullException(nameof(a)) will throw, but a != null ? a.Length : throw new ArgumentNullException(nameof(a)) won't. You could use (a ?? throw new ArgumentNullException(nameof(a))).Length, but that looks terrible to me and is hard to parse visually.

alrz commented 7 years ago

@333fred Right, if the property is of a reference type, that's true. I'm just saying that in your example it doesn't change the behavior so it will be safe to do it in this specific case.

davkean commented 7 years ago

I believe @CyrusNajmabadi recently fixed this, I can't find the PR though. While it still repro's in 26028.1, it doesn't repro in my RoslynDev hive with recent bits from master.

Pilchie commented 7 years ago

@CyrusNajmabadi Can you confirm a fix here?

CyrusNajmabadi commented 7 years ago

Yes. This has been fixed. We were missing the check that the code down the when-true/false path was 'null' as appropriate.

CyrusNajmabadi commented 7 years ago

Checked. We only offer the fix if you have a != null ? a.Length : null

alrz commented 7 years ago

@CyrusNajmabadi In case a.Length is a value type it could use ?? operator. Should that be another fix then or we don't usually consider types to suggest fixes?

CyrusNajmabadi commented 7 years ago

Seems like a reasonable thing a new feature could provide.