Additional tests for refout. Additional argument check on IncludePrivateMembers #19478
Conversation
|
@dotnet/roslyn-compiler for review. Thanks |
|
@dotnet/roslyn-compiler for review (15.3) please. |
| } | ||
| }", | ||
| comp => comp.VerifyDiagnostics( | ||
| // (7,16): error CS0266: Cannot implicitly convert type 'I<Base>' to 'I<Derived>'. An explicit conversion exists (are you missing a cast?) |
There was a problem hiding this comment.
This looks like failure rather than success. Please either change out to in, or swap the argument and return types in M. #Resolved
| a.M(); | ||
| } | ||
| }", | ||
| comp => comp.VerifyDiagnostics()); |
There was a problem hiding this comment.
This demonstrates that there is an [Optional] attribute, but not that the argument value is preserved. #Resolved
| { | ||
| void M2() | ||
| { | ||
| System.Console.Write(A.number); |
There was a problem hiding this comment.
Can this be run to verify that the value was preserved? #Resolved
|
@gafter I addressed your feedback. |
| comp => | ||
| { | ||
| comp.VerifyDiagnostics(); | ||
| var verifier = CompileAndVerify(comp); |
There was a problem hiding this comment.
No, unfortunately, we cannot execute scenarios with ref assemblies because ref assemblies won't be loaded by the runtime.
I could test the scenario: (1) compile client against ref assembly, (2) execute against the real thing, but I think the IL check achieves the confidence we want (ie the ref assembly still has the constant, so the client ends up inlining that value).
| comp => comp.VerifyDiagnostics()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void RefAssemblyClient_StructWithPrivateGenericField() |
There was a problem hiding this comment.
Are we testing that the field type is preserved if the type is private?
There was a problem hiding this comment.
Yes. I find at least one test: RefAssembly_VerifyTypesAndMembersOnStruct.
There was a problem hiding this comment.
Also, RefAssemblyClient_StructWithPrivateReferenceTypeField and RefAssemblyClient_StructWithPrivateGenericField, indirectly.
There was a problem hiding this comment.
Do those tests test a type that is private to the reference assembly? For example:
struct S
{
private class C { }
private C F;
}
| Assert.Equal("S.field As System.Int32", field.ToTestDisplayString()) | ||
| End Sub | ||
|
|
||
| Private Sub RefAssemblyNoPiaReferenceFromMethodBody() |
There was a problem hiding this comment.
Missing <Fact> :-s
Thanks
| Dim [module] = DirectCast(referencedAssembly.Modules(0), PEModuleSymbol) | ||
|
|
||
| Dim itest1 = [module].GlobalNamespace.GetMember(Of NamedTypeSymbol)("ITest1") | ||
| Assert.NotNull(itest1.GetAttributes().Where(Function(a) a.IsTargetAttribute("System.Runtime.InteropServices", "TypeIdentifierAttribute")).First()) |
There was a problem hiding this comment.
Perhaps use .Single() rather than .First() for these attribute checks.
| ' The ref assembly produced by refout has more types than that produced by refonly, | ||
| ' because refout will bind the method bodies (and therefore populate more referenced types). | ||
| ' This will be refined in the future. Follow-up issue: https://github.com/dotnet/roslyn/issues/19403 | ||
| Private Sub RefAssemblyNoPiaReferenceFromMethodBody_verifyNoPia(image As ImmutableArray(Of Byte), expectMissing As Boolean) |
|
LGTM |
|
retest windows_debug_vs-integration_prtest please |
|
retest windows_release_vs-integration_prtest please |
|
@MeiChin-Tsai for ask-mode approval. |
|
retest windows_release_vs-integration_prtest please |
This is a follow-up on the ref assemblies feature, adding more tests that were not blockers (from the test plan, #18612).
Also, I realized there is one more combination of parameters that should be disallowed when emitting (IncludePrivateMembers=false, but neither EmitMetadataOnly or metadataPeStream are set).
Also tweaking an error message for deconstruction (to fix #16543). Notice that the designer file seemed out of date, so more changes got pulled in.
Risk
Performance impact
Low. This is mostly test changes and a very minor change to check arguments.
@dotnet/roslyn-compiler for review.