I think integration test failures are due to need to ingest newer changes from main into the feature branch.

Done review pass (commit 5). I did not look at test changes.

Done review pass (commit 6). Have not looked at test changes.

@333fred I made the changes to restore the old nullable value type behavior, support pointers and function pointers, and fix adding the synthesized methods to PrivateImplementationDetails.

Test failures look legitimate. Done review pass (commit 9)

Bleh, yes, basically when a type parameter is constrained to some flavor of System.Nullable<T> it's a bit of a pain to actually access the HasValue member on it. This comes up in TestNullCheckedSubstitution2. I added 2 commits to try and resolve.

The first commit e40037f actually attempts to access the HasValue member on the type parameter constrained to nullable value type. This introduced a verification failure that I wasn't expecting: PeVerify failed for assembly '': [ : B3`1[T]::M[U]][mdToken=0x6000003][offset 0x00000002][found address of ref ][expected address of value 'System.Nullable`1[T]'] Unexpected type on the stack.. I wonder if a separate bug in lowering/emit is causing us to emit a ldarga when we should be emitting a ldarg.

It seems like the code actually executes just fine at least on core. Still it made me a bit wary and since the scenario is so out in left field I pushed 00a2898 which reverts most of its parent and uses the "boxing" code gen in this specific case.

edit: debugging the HasValue version a bit more, it seems like we hit this branch in the emitter. It seems like it's not really designed to contend with type parameters constrained to specific value types. We could probably fix the emitter to give ldarg in this case which would probably satisfy the verifier but it doesn't really feel worth it right now.

        return factory.HiddenSequencePoint(factory.If(paramIsNullCondition, throwArgNullStatement));

I'm curious why we chose to inline the if (arg is null) throw new ArgumentNullException(); in these cases rather than using a helper.

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullChecking.cs:102 in c345bd5. [](commit_id = c345bd5, deletion_comment = False)

I'm curious why we chose to inline the if (arg is null) throw new ArgumentNullException(); in these cases rather than using a helper.

For pointers it didn't feel worthwhile to come up with another pair of helpers (ThrowPointerIfNull, etc.) due to their uncommonness. For nullable value types it seemed like it would require a generic ThrowIfNull<T>(T? argument, string paramName) which also doesn't seem worth it.

FWIW, I imagine we may evolve this implementation in the future. If the runtime changes to inline methods across assemblies in more scenarios, we might be able to drop the internal helpers and use directly the ArgumentNullException.ThrowIfNull helpers defined for both objects and pointers.

roslyn-integration-ci won't pass due to being out of date with main. Merging now.