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

Enforce non-null factory return in Lazy<T> #111987

Open
hhmmjjnn opened this issue Jan 30, 2025 · 8 comments
Open

Enforce non-null factory return in Lazy<T> #111987

hhmmjjnn opened this issue Jan 30, 2025 · 8 comments
Labels
area-System.Runtime needs-author-action An issue or pull request that requires more info or actions from the author.

Comments

@hhmmjjnn
Copy link

In C#, the contract of non-nullability for Func<T> can be easily violated without any compiler error or warning, even in legitimate code, specially when crossing assembly boundaries:

#nullable disable
namespace AcmeInc.LandmineFactory;
/* (...) */
    object createLandmine()
    {
        return null; // 100% valid and legitimate code
    }

#nullable enable
namespace BlackMesa.AnomalousMaterials;
/* (...) */
        var landmine = new Lazy<object>(createLandmine); // we don't know better
        Console.WriteLine(landmine.Value.ToString()); // kaputt

Even though the factory implementation can be provided via public interface, the class Lazy<T> trusts it blindly:

https://github.com/dotnet/runtime/blob/release/8.0/src/libraries/System.Private.CoreLib/src/System/Lazy.cs#L323C1-L324C1

        _value = factory();

https://github.com/dotnet/runtime/blob/release/8.0/src/libraries/System.Private.CoreLib/src/System/Lazy.cs#L378C1-L379C1

        PublicationOnly(initializer, factory());

I would argue that, at the boundary of a System project and a non-System project, the nullability contract should be enforced at runtime, by the System project, for incoming data: (compare this statement to rule CA1062)

https://github.com/dotnet/runtime/blob/7693b6d0202281dc38dcfb9562e9064fcd13bc61/src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs#L114C1-L115C1

        T value = valueFactory() ?? throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation);

On the other hand, a non-System project should be able to trust the type signature of a concrete object implemented in the System project:

        Console.WriteLine(landmine.Value.ToString()); // I trusted you 😥

Whether this would be a breaking change is debatable.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 30, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 30, 2025
@stephentoub
Copy link
Member

Whether this would be a breaking change is debatable.

It would definitely be a breaking change. It's valid to have a Lazy<string?>, for example, where null is produced by the factory method.

@huoyaoyuan
Copy link
Member

In other words, the Lazy itself can't distinguish Lazy<T> and Lazy<T?> in its implementation.

@colejohnson66
Copy link

colejohnson66 commented Jan 30, 2025

As @stephentoub and @huoyaoyuan said, it's not really actionable. It's unfortunate, but it's a fact of life when dealing with nullable reference types. As you showed, there's already nothing stopping someone from returning null from a supposedly non-null function. In addition, even if nullable reference types are enabled, this is legal code and won't give a warning, but will still blow up at runtime:

public static void BlowUp()
{
    string result = GetString();
    Console.WriteLine(result.Length);
    return;

    // assume this method is in an external assembly, so you
    //   don't actually know it returns null without inspection
    static string GetString() =>
        null!;
}

The "simplest" way to solve this would be the runtime providing some InvalidOperationException.ThrowIfNull([NotNull] object? value) method, but I highly doubt they'd want to do that.

@stephentoub
Copy link
Member

The "simplest" way to solve this would be the runtime providing some InvalidOperationException.ThrowIfNull([NotNull] object? value) method, but I highly doubt they'd want to do that.

That would do what?

@colejohnson66
Copy link

People could use it to throw if a null is returned from a "non-null" returning method. From the OP's example:

var landmine = new Lazy<object>(createLandmine);
InvalidOperationException.ThrowIfNull(landmine); // <--
Console.WriteLine(landmine.Value.ToString());

Essentially, the same as ArgumentNullException.ThrowIfNull, but for return values instead of parameters. I don't think it's that good of an idea.

@jkotas jkotas added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 30, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@tannergooding
Copy link
Member

Note that this behavior is covered under https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-context

The code compiled under the segment that is #nullable disable is functionally "oblivious" and consuming code, even if its in a nullable enabled context, won't get a warning from dereferencing it because the default state is presumed non-null.

The fix for this is to ensure that all code is nullable enabled so that user-expectations can be met.

@tannergooding tannergooding added needs-author-action An issue or pull request that requires more info or actions from the author. and removed untriaged New issue has not been triaged by the area owner labels Jan 30, 2025
Copy link
Contributor

This issue has been marked needs-author-action and may be missing some important information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

No branches or pull requests

6 participants