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

Handle generic types as parameters of attributes in MetadataDecoder #70151

Merged
merged 25 commits into from
Oct 23, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 27, 2023

Fixes #66370.
Fixes #55190.

PEAttributeData has the attribute symbol, so this PR changes MetadataDecoder to use attribute argument types from that symbol (instead of reading the metadata directly), hence all cases are handled - including the previously missing generic type parameters.

PIA attribute validation has also been refactored. Previously, it decoded attributes in all cases, now it does that only for SourceAssemblySymbols; whereas it consults metadata directly for PEAssemblySymbols. See #70151 (comment).

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 27, 2023
@jjonescz jjonescz force-pushed the 66370-MetadataDecoderGeneric branch from 8a29dff to d280b1e Compare October 2, 2023 10:49
@jjonescz jjonescz marked this pull request as ready for review October 2, 2023 12:31
@jjonescz jjonescz requested a review from a team as a code owner October 2, 2023 12:31
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jjonescz jjonescz marked this pull request as draft October 6, 2023 10:31
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 6, 2023

                if (signatureHeader.IsGeneric && sigReader.ReadCompressedInteger() != 0)

We might want to have an equivalent check in the method above, just in case. #Closed


Refers to: src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs:1795 in 6c27299. [](commit_id = 6c27299, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 6, 2023

                if (returnTypeCode != SignatureTypeCode.Void)

And this check as well #Closed


Refers to: src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs:1805 in 6c27299. [](commit_id = 6c27299, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4), tests are not looked at.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 23)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 25)

@jjonescz jjonescz marked this pull request as ready for review October 17, 2023 14:01
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For the second review

@AlekseyTs AlekseyTs requested a review from a team October 18, 2023 16:14
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For the second review

@jaredpar
Copy link
Member

@333fred PTAL

@AlekseyTs
Copy link
Contributor

@333fred, @dotnet/roslyn-compiler For the second review

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I have a question on the structure of the new methods.

@jjonescz jjonescz merged commit fc6e0c2 into dotnet:main Oct 23, 2023
@jjonescz jjonescz deleted the 66370-MetadataDecoderGeneric branch October 23, 2023 18:23
@ghost ghost added this to the Next milestone Oct 23, 2023
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
4 participants