-
Notifications
You must be signed in to change notification settings - Fork 5k
Expose nullability info #54985
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
Conversation
Note regarding the 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. |
...libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfoContext.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfoContext.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfoContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/NullabilityInfoContextTests.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfo.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfoContext.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfoContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Nullability/NullabilityInfo.cs
Outdated
Show resolved
Hide resolved
return nullability; | ||
} | ||
|
||
private bool IsPublicOnly(bool isPrivate, bool isFamilyAndAssembly, bool isAssembly, Module module) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
9bdfed9
to
930026a
Compare
…thod(MetadataToken)
930026a
to
e190c91
Compare
src/libraries/System.Private.CoreLib/src/System/Reflection/NullabilityInfoContext.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/NullabilityInfoContextTests.cs
Outdated
Show resolved
Hide resolved
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 |
No, and I don't think there is a way to distinguish 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, ...}, ... }
} |
@buyaa-n There are different flags in the IL:
|
@n9 I don't see that info in runtime, anyway, the API is not for |
There was a problem hiding this 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()); |
There was a problem hiding this comment.
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
3704065
to
86a287d
Compare
Fixes #29723
Approved API shape:
[MaybeNullWhen]
should work same as[MaybeNull]
.if the
NullablePublicOnlyAttribute
set in the module we returnNullabilityState.Unknown
for private and/or internal membersThe nullability for generic type
T
should be tracked by the user, a field declarationList<string?> list
orList<string> list
will have type parameter nullability info but nullability for otherList<T>
API calls should be tracked by the user. For example forlist.Add(T item)
the nullability ofitem
will be evaluated as follows:Nullable
if the declaration has?
i.e.GenericType<T?>
(even ifT
is value type it is nullable value type)Nullable
forGenericType<T>
when nullability context enabled and the concrete type ofT
is ref type or nullable value type. (In this case the nullability of parameterT
ofList.Add(T item)
will beNullable
for bothList<string>
andList<string?>
instance, we might want 4th nullability state for this scenario) CC @terrajobstNotNull
forGenericType<T>
when concreteT
is non-nullable value typeUnknown
forGenericType<T>
when nullability context disabled and the concrete type ofT
is ref typeT
For value type nullability:
int?
is nullableint
is not null no matter nullability contextAllowNull/DisallowNull/NotNull/MaybeNull
atributes affect write/read state ofint?
type, but notint