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

Expose nullability info #54985

Merged
merged 12 commits into from Jul 13, 2021
Merged

Expose nullability info #54985

merged 12 commits into from Jul 13, 2021

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Jun 30, 2021

Fixes #29723

Approved API shape:

namespace System.Reflection
{
    public sealed class NullabilityInfoContext
    {
        public NullabilityInfo Create(ParameterInfo parameterInfo);
        public NullabilityInfo Create(PropertyInfo propertyInfo);
        public NullabilityInfo Create(EventInfo eventInfo);
        public NullabilityInfo Create(FieldInfo parameterInfo);
    }

    public sealed class NullabilityInfo
    {
        public Type Type { get; }
        public NullabilityState ReadState { get; }
        public NullabilityState WriteState { get; }
        public NullabilityInfo? ElementType { get; }
        public NullabilityInfo[] GenericTypeArguments { get; }
    }

    public enum NullabilityState
    {
        Unknown,
        NotNull,
        Nullable
    }
}
  • [MaybeNullWhen] should work same as [MaybeNull].

  • if the NullablePublicOnlyAttribute set in the module we return NullabilityState.Unknown for private and/or internal members

  • The nullability for generic type T should be tracked by the user, a field declaration List<string?> list or List<string> list will have type parameter nullability info but nullability for other List<T> API calls should be tracked by the user. For example for list.Add(T item) the nullability of item will be evaluated as follows:

    1. T is Nullable if the declaration has ? i.e. GenericType<T?> (even if T is value type it is nullable value type)
    2. T is Nullable for GenericType<T> when nullability context enabled and the concrete type of T is ref type or nullable value type. (In this case the nullability of parameter T of List.Add(T item) will be Nullable for both List<string> and List<string?> instance, we might want 4th nullability state for this scenario) CC @terrajobst
    3. T is NotNull for GenericType<T> when concrete T is non-nullable value type
    4. T is Unknown for GenericType<T> when nullability context disabled and the concrete type of T is ref type
    5. For open generics the behavior is the same as ref type of T
  • For value type nullability:

    • int? is nullable
    • int is not null no matter nullability context
    • AllowNull/DisallowNull/NotNull/MaybeNull atributes affect write/read state of int? type, but not int

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Jun 30, 2021
return nullability;
}

private bool IsPublicOnly(bool isPrivate, bool isFamilyAndAssembly, bool isAssembly, Module module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should isFamilyOrAssembly (aka protected internal) also be considered? Are there tests for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will test that, as far as i know, protected members was not affected

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some test, could not test exactly as could not find an assembly that removed nullability attribute for the internal member's too, it only applied for private members, for now i just guess that because of protected modifier protected internal members will not be affected

@buyaa-n buyaa-n moved this from Needs triage to Buyaa's triage backlog in Triage POD for Meta, Reflection, etc Jul 7, 2021
@n9
Copy link

n9 commented Jul 9, 2021

Does this PR provide information about generic arguments of implemented interfaces? For instance:

interface IBar<T> { }

class Bar : IBar<string> { }
class BarN : IBar<string?> { }

Context: #49900

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 9, 2021

Does this PR provide information about generic arguments of implemented interfaces?

No, and I don't think there is a way to distinguish IBar<string>, IBar<string?> from your example

If there is a member declared with a generic interface it would have the nullability of the generic type

interface IBar<T> { }
class C
{
    public  IBar<string> bar;  // {ReadState=NotNull, GenericTypeArguments = {ReadState=NotNull, ...}, ... } 
    public  IBar<string?> nBar;  // {ReadState=NotNull, GenericTypeArguments = {ReadState=Nullable, ...}, ... } 
}

@n9
Copy link

n9 commented Jul 9, 2021

@buyaa-n There are different flags in the IL:

.class private auto ansi beforefieldinit Bar
    extends [System.Private.CoreLib]System.Object
    implements .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
        01 00 02 00 00 00 00 01 00 00
    )
    class IBar`1<string>

.class private auto ansi beforefieldinit BarN
    extends [System.Private.CoreLib]System.Object
    implements .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
        01 00 02 00 00 00 00 02 00 00
    )
    class IBar`1<string>

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 9, 2021

@n9 I don't see that info in runtime, anyway, the API is not for Type nullability, it is only for FieldInfo, ParameterInfo, PropertyInfo, EventInfo nullability, and the NullabilityInfo returned by the API has no info of its Type's interface implementation cc @terrajobst

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion, looks good pending known open items (Mono test failures, readonly properties should return "unknown")

throw new ArgumentNullException(nameof(eventInfo));
}

return GetNullabilityInfo(eventInfo, eventInfo.EventHandlerType!, eventInfo.GetCustomAttributesData());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even NullablePublicOnlyAttribute is set the nullability attributes not being removed for private events (not sure if this is Roslyn bug or by design), therefore not checking that info for EventInfo

@buyaa-n buyaa-n merged commit 62966bc into dotnet:main Jul 13, 2021
Triage POD for Meta, Reflection, etc automation moved this from Buyaa's triage backlog to Done Jul 13, 2021
@buyaa-n buyaa-n deleted the expose_nullability_info branch July 13, 2021 16:34
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Expose top-level nullability information from reflection
10 participants