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

Compiler assumes that null-oblivious assembly would have been able to define a property as T? #56518

Open
jnm2 opened this issue Sep 18, 2021 · 4 comments

Comments

@jnm2
Copy link
Contributor

jnm2 commented Sep 18, 2021

Version Used: 17.0.0-p4

There should be no warning here. Using SomeCollection<C?> would be very incorrect and cause problems in most of the uses of the collection. It's important that the collection is modeled to show that there are never nulls in the collection.

The compiler should assume an indeterminate state for whether the null-oblivious assembly would have used T? instead of T. This would be consistent with what the compiler assumes about whether a null-oblivious assembly would have used object? instead of object, and so on.

#nullable enable

class C
{
    void M(C? focusedItem)
    {
        var c = new SomeCollection<C>();

        // ⚠ CS8601 Possible null reference assignment.
        //              ↓↓↓↓↓↓↓↓↓↓↓
        c.FocusedItem = focusedItem;
    }
}

#nullable disable

class SomeCollection<T>
{
    // This would have been T? but it's defined in an assembly that was compiled without NRTs.
    public T FocusedItem { get; set; }
}
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 18, 2021
@CyrusNajmabadi
Copy link
Member

@jcouv ?

@RikkiGibson
Copy link
Contributor

This is just a matter of type substitution. We could make it so that when a non-annotated 'C' is used as an argument for an oblivious 'T', it results in an oblivious 'C', but the effects seem wide-reaching, affecting behavior of inputs to many different kinds of members.

I would be tempted to try making the change and see what the impact is on the test baselines.

@jcouv
Copy link
Member

jcouv commented Sep 20, 2021

As Rikki pointed out, this is by-design. We're plugging in a type argument that is ! (not-annotated) into a type parameter that is ~ (oblivious), and the current design yields a !.
The current recommendation for users would be to change the type argument to be oblivious (using localized #nullable disable) but that is painful.

I've explored a few related options at length last year (in the context of #49441 and with a few proposed design options https://gist.github.com/jcouv/4dd5291e65a4cbffa83c03440427d557) to sort out some type substitution irregularities.
One option (labelled "proposal 0" in linked doc) was that ~ plugged into ! and ! plugged into ~ would yield ~. But that had nasty side effects because the compiler uses ~ for type arguments on definitions (ie. was hard to change without messing more things up).
But I think that the narrower change of just doing the second half (having ! plugged into ~ yield ~) may be feasible and worth a try, despite being less symmetrical. It seems like it would only produce fewer warnings because it lets oblivious flow through more.

@RikkiGibson
Copy link
Contributor

I think this only has the effect of removing warnings, so probably a low risk change. I think it's worth experimenting with for 17.1.

@RikkiGibson RikkiGibson added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 21, 2021
@RikkiGibson RikkiGibson added this to the 17.1 milestone Sep 21, 2021
@jinujoseph jinujoseph modified the milestones: 17.1, 17.3 Apr 27, 2022
@jaredpar jaredpar modified the milestones: 17.3, Backlog Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants