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

Add handling of generic attributes to CustomAttributeDecoder #57466

Closed

Conversation

MichalStrehovsky
Copy link
Member

If the attribute constructor refers to the generic T in its signature, we would throw BadImageFormatException. This adds handling by capturing the generic context and interpreting the signature variables when needed.

I've hit this in the src\tests\reflection\GenericAttribute tests with NativeAOT since the NativeAOT compiler uses S.R.Metadata to read custom attributes.

No tests yet because I would like to discuss testing strategy.

  • There's notes about pre-existing test debt in CustomAttributeDecoderTests.
  • I don't want to write an IL test for this because they're a maintenance nightmare.
  • We crosstarget (and test?) on .NET Framework. I don't know the support plans for generic attributes when targeting net461 with Roslyn. I don't know the support status of generic attributes in this repo.

I tested it with the src\tests\reflection\GenericAttribute test and NativeAOT, but that test is not much more than a smoke test, really.

Cc @tmat @RikkiGibson

If the attribute constructor refers to the generic T in its signature, we would throw `BadImageFormatException`. This adds handling by capturing the generic context and interpreting the signature variables when needed.
@tmat
Copy link
Member

tmat commented Aug 16, 2021

    private ArgumentTypeInfo DecodeNamedArgumentType(ref BlobReader valueReader, bool isElementType = false)

Named argument can't by of generic type argument type?

class A<S, T>
{
    S[] P { get; }
    A(T arg) {} 
}

[A<int, bool>(1, P = new[] { true })]
class C {}

Refers to: src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/CustomAttributeDecoder.cs:230 in 5204378. [](commit_id = 5204378, deletion_comment = False)

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely add unit tests for this.

You can use MetadataBuilder to construct the metadata image in the test that has generic attributes and then read them back.

@MichalStrehovsky
Copy link
Member Author

Named argument can't by of generic type argument type?

Named arguments encode their type inline - we don't need to resolve the members to find their signature.

@tmat
Copy link
Member

tmat commented Aug 16, 2021

Named arguments encode their type inline - we don't need to resolve the members to find their signature.

So DecodeNamedArgumentType does not need to handle SignatureTypeCode.GenericTypeParameter? Seems odd

Never mind, the types used there will be specific instantiations, not generic type parameters.


int parameterIndex = signatureReader.ReadCompressedInteger();
int numGenericParameters = genericContextReader.ReadCompressedInteger();
if (parameterIndex >= numGenericParameters)
Copy link
Member

@krwq krwq Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider also checking for negative parameterIndex and numGenericParameters


switch (typeCode)
{
case (int)SignatureTypeCode.Boolean:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there any existing subroutine for checking simple types?

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering it's not valid in C# to create generic attributes I'm ok with no extra tests and just unblocking scenarios for problematic assemblies. If they're ever supported then we should add more tests and fix any rough edges if there are any

@tmat
Copy link
Member

tmat commented Aug 24, 2021

@krwq Generic attributes are going to be supported in C# 10 or 11. We need tests.

@RikkiGibson
Copy link
Contributor

Generic attributes are shipping in preview in .NET 6/C# 10, and will hopefully ship stable in C# 11.

@MichalStrehovsky
Copy link
Member Author

I thought I would be able to get to the tests, but it probably won't happen anytime soon. I filed #58073. It's better if it's handled by someone else. The fix I have works and I can just fork the file where I need this. I have test coverage through other CoreCLR tests from that.

@krwq
Copy link
Member

krwq commented Aug 25, 2021

@tmat @RikkiGibson if that's the case then agreed we need tests

@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2021
@MichalStrehovsky MichalStrehovsky deleted the readGenericAttributes branch September 30, 2022 04:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants