Nullable for Type class

[buyaa-n]

No description provided.

[stephentoub]

Thanks for the initial work on this. I think there are a fair number of mistakes in the APIs, though, as derived types weren't factored in. Please add to this PR the other derivations of Type that are in corelib, and then the whole thing can be re-reviewed. Otherwise the annotation of Type isn't accurate.

[buyaa-n]

@stephentoub should it cover immediate descendants or all derived type? I guess preferably all descendants but isn't it will become too big PR in that case?

[stephentoub]

I guess preferably all descendants but isn't it will become too big PR in that case?

Ideally "all". Even if it ends up being bigger, it'll help us to be more efficient in the long run, because we won't need to keep revisiting the types. It'll also be easier to review when all related types are done together. And will help to validate the correctness of the changes being reviewed.

[buyaa-n]

The instance of this class only created using native call

coreclr/src/vm/runtimehandles.cpp

Line 2420 in 4a26541

FCIMPL2(RuntimeMethodBody *, RuntimeMethodHandle::GetMethodBody, ReflectMethodObject *pMethodUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE)

where these values looks like set non null and here it passed to non nullable method parameters. So kept them non nullable

[stephentoub]

? [](start = 21, length = 1)

Can this be null? I'm curious because of the method name; it makes it sound like only a valid license would be returned, and I'm assuming (maybe incorrectly) that null would be an invalid license.

It doesn't matter much for this method itself, as it's internal; I just wonder whether this nullable-ness is flowing out and impacting the annotations we're adding to callers that are exposed publicly.

[buyaa-n]

Can this be null? I'm curious because of the method name; it makes it sound like only a valid license would be returned, and I'm assuming (maybe incorrectly) that null would be an invalid license.

marked nullable as MethodBase.Invoke(...) method called on return has nullable return. You mean if validation failed it would throw instead of returning null? From try catch most likely that is true

It doesn't matter much for this method itself, as it's internal; I just wonder whether this nullable-ness is flowing out and impacting the annotations we're adding to callers that are exposed publicly.

Checked publicly exposed method of IClassFactory and IClassFactory2 interface

void CreateInstance(
            [MarshalAs(UnmanagedType.Interface)] object? pUnkOuter,
            ref Guid riid,
            [MarshalAs(UnmanagedType.Interface)] out object? ppvObject);

Seems paramete out object? ppvObject should be non nullable

[stephentoub]

marked nullable as MethodBase.Invoke(...) method called on return has nullable return

That's because any method with any return value might be returned. You'd need to examine the particular method being invoked.

You mean if validation failed it would throw instead of returning null?

Yes, that's my question. Maybe @AaronRobinsonMSFT knows.

Seems paramete out object? ppvObject should be non nullable

The docs state:

If the object does not support the interface specified in riid, the implementation must set *ppvObject to NULL.

so I believe nullable here is correct. But that doesn't mean that it might be null for every use of this method, just that some uses could result in null.

[AaronRobinsonMSFT]

You mean if validation failed it would throw instead of returning null?

Yes, that's my question. Maybe @AaronRobinsonMSFT knows.

None of these COM functions can return null during allocation. In this scenario an Exception will be marshalled as an error HRESULT since the function method is marked as returning void. This can be observed in the exception below that throws CLASS_E_NOTLICENSED which will result in an error HRESULT to the caller - which means by COM semantics that all out parameters are in an undefined state.

[buyaa-n]

thank you, updated only this method to return non null

[stephentoub]

            internal void Insert(ref T[] list, string name, MemberListType listType)

null is getting passed in here explicitly, with null!. Seems like instead this should be string?, since obviously this method needs to be able to handle null getting passed in, as we are.

---

Refers to: src/System.Private.CoreLib/src/System/RtType.cs:367 in 6f10090. [](commit_id = 6f10090, deletion_comment = False)

[stephentoub]

ref string? name [](start = 41, length = 16)

Please add // TODO-NULLABLE: [https://github.com/dotnet/roslyn/issues/26761](https://github.com/dotnet/roslyn/issues/26761`) here

[buyaa-n]

added

[stephentoub]

I did another spot-check pass through and left some comments. Let's get the remaining comments addressed, as well as the build passing, and then get this merged. Thanks!