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

[DllImportGenerator] Use ElementMarshallingGeneratorFactory to create the marshalling generator for collection elements #61219

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

jkoritzinsky
Copy link
Member

Fixes a typo. Currently doesn't cause test failures since ElementMarshallingGeneratorFactory == this in all current scenarios (this is not true in the struct marshalling generator hackathon branch).

… the marshalling generator for collection elements
@AaronRobinsonMSFT
Copy link
Member

Seems like we need a test for this logic. Should I assume this was because we started only with blittable support?

@jkoritzinsky
Copy link
Member Author

No, that is not why this was missed.

If we look in the source where this is used, we'll see that the only generators that wrap the AttributedMarshallingModelGeneratorFactory are the "HResult native return to exception" factory and the "[In]/[Out] validator" factory. Neither of these factories have an effect on any of the possible marshallers that elements of arrays would use, so the behavior change isn't easily testable with our current testing methodology of running the generator and testing the output (instead of adding unit tests that call into the various different types we define and use to implement the generator today).

generatorFactory = new DefaultMarshallingGeneratorFactory(options);
AttributedMarshallingModelGeneratorFactory attributedMarshallingFactory = new(generatorFactory, options);
generatorFactory = attributedMarshallingFactory;
if (!dllImportData.PreserveSig)
{
// Create type info for native out param
if (!method.ReturnsVoid)
{
// Transform the managed return type info into an out parameter and add it as the last param
TypePositionInfo nativeOutInfo = retTypeInfo with
{
InstanceIdentifier = PInvokeStubCodeGenerator.ReturnIdentifier,
RefKind = RefKind.Out,
RefKindSyntax = SyntaxKind.OutKeyword,
ManagedIndex = TypePositionInfo.ReturnIndex,
NativeIndex = typeInfos.Count
};
typeInfos.Add(nativeOutInfo);
}
// Use a marshalling generator that supports the HRESULT return->exception marshalling.
generatorFactory = new NoPreserveSigMarshallingGeneratorFactory(generatorFactory);
// Create type info for native HRESULT return
retTypeInfo = new TypePositionInfo(SpecialTypeInfo.Int32, NoMarshallingInfo.Instance);
retTypeInfo = retTypeInfo with
{
NativeIndex = TypePositionInfo.ReturnIndex
};
}
generatorFactory = new ByValueContentsMarshalKindValidator(generatorFactory);
attributedMarshallingFactory.ElementMarshallingGeneratorFactory = generatorFactory;

In the struct marshalling source generator, I found this bug because I had to use a different instance for the collection element factory than the collection factory, and using the collection factory for the elements was generating invalid code.

@AaronRobinsonMSFT
Copy link
Member

we'll see that the only generators that wrap the AttributedMarshallingModelGeneratorFactory

So this field is only needed when it is wrapped? I was debugging through that code path and it is incredibly complicated and we seem to be referring to each marshaller in a circular manner. At this point I am completely unsure of what is and isn't being used. Debugging this has also become very complicated due to all of the levels of indirection. I fear this is becoming more complicated than the original. The nesting itself makes the delegation difficult enough but add in the validator and it becomes - what is actually responsible in this case?

@jkoritzinsky
Copy link
Member Author

Yes, this field is only needed when it is wrapped since the code defaults it to this.

If we want to go with a different design than a decorator-pattern sort of thing I went with for the different marshalling generators, I'd be okay with that. It just felt like the best design at the time.

I'll go change how we set up the element marshalling factory right now though, after seeing how I had to use it in the struct marshalling generator, I think a slightly different design than what I went with will work best.

@jkoritzinsky
Copy link
Member Author

@AaronRobinsonMSFT I pushed out my slightly changed design. If you have any more requests or still don't like the new design, let me know.

@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky Thanks. This at least doesn't pollute the public API surface - that is a huge help/indication to me when learning. We can iterate as it evolves but this makes it far clearer with the present design. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants