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

Wrapping a call with [MaybeNullWhen] results in incorrect compiler warnings #39681

Closed
johncrim opened this issue Nov 4, 2019 · 3 comments
Closed
Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types Resolution-Duplicate The described behavior is tracked in another issue

Comments

@johncrim
Copy link

johncrim commented Nov 4, 2019

Version Used:
dotnet sdk 3.0.100

Steps to Reproduce:

  1. dotnet new console
  2. Add <Nullable>enable to the .csproj file:
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>
  1. Add a method with a [MaybeNullWhen] attribute that wraps a call to another method using a [MaybeNullWhen] attribute. Eg:
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace MaybeNullWhen_Propagation
{
    class Program
    {
        private Dictionary<int, Process> _processes = new Dictionary<int, Process>();

        public bool TryGetProcessById(int processId, [MaybeNullWhen(false)] out Process process)
        {
            return _processes.TryGetValue(processId, out process);
        }

        public static void Main(string[] args)
        {
            Console.WriteLine("Hello World!");
        }
    }
}
  1. dotnet build

Expected Behavior:
No warnings

Actual Behavior:
Received warning CS8601

Program.cs(14,58): warning CS8601: Possible null reference assignment. [C:\src\cc\test-cases\MaybeNullWhen-Propagation\MaybeNullWhen-Propagation.csproj]
    1 Warning(s)
    0 Error(s)

I'm not able to find a good workaround without disabling a warning. Using an additional local variable, and then assigning null to the out param when returning false, results in a different warning:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace MaybeNullWhen_Propagation
{
    class Program
    {
        private Dictionary<int, Process> _processes = new Dictionary<int, Process>();

        public bool TryGetProcessById(int processId, [MaybeNullWhen(false)] out Process process)
        {
            if (_processes.TryGetValue(processId, out Process? p))
            {
                process = p;
                return true;
            }
            else
            {
                process = null;
                return false;
            }
        }

        public static void Main(string[] args)
        {
            Console.WriteLine("Hello World!");
        }
    }
}

Results in:

Program.cs(21,27): warning CS8625: Cannot convert null literal to non-nullable reference type. [C:\src\cc\test-cases\MaybeNullWhen-Propagation\MaybeNullWhen-Propagation.csproj]
    1 Warning(s)
    0 Error(s)
@RikkiGibson
Copy link
Contributor

RikkiGibson commented Nov 8, 2019

I think this is a symptom of #36039. Inside the TryGetProcessById implementation, the compiler currently isn't taking the attribute on out Process process into account. It's only at the call sites that the attribute is actually considered in the analysis.

I hope this gets addressed at some point, but my recommendation for the short term is to just change the implementation to _processes.TryGetValue(processId, out process!);

@joshwyant
Copy link

My workaround currently is using

#nullable disable
...
#nullable enable

@jcouv
Copy link
Member

jcouv commented Dec 11, 2019

Duplicate of #39656 (also about wrapping a TryGetValue call in a TryGetValue method with [MaybeNullWhen(...)]).
I'll go ahead and close this one. Thanks

Attributes are known to only affect callers at the moment. They are not respected in method bodies. The scenario you describe is particularly tricky though, as it is conditional...

@jcouv jcouv closed this as completed Dec 11, 2019
@jcouv jcouv added the Resolution-Duplicate The described behavior is tracked in another issue label Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types Resolution-Duplicate The described behavior is tracked in another issue
Projects
None yet
Development

No branches or pull requests

5 participants