Skip to content

Handle generic types as parameters of attributes in MetadataDecoder#70151

Merged
jjonescz merged 25 commits intodotnet:mainfrom
jjonescz:66370-MetadataDecoderGeneric
Oct 23, 2023
Merged

Handle generic types as parameters of attributes in MetadataDecoder#70151
jjonescz merged 25 commits intodotnet:mainfrom
jjonescz:66370-MetadataDecoderGeneric

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 27, 2023 •
edited
Loading

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).

@ghost ghost 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
AlekseyTs reviewed Oct 2, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Oct 2, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Oct 2, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Oct 3, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Oct 3, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Oct 3, 2023
View reviewed changes
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 3, 2023

Done with review pass (commit 1)

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

AlekseyTs commented Oct 6, 2023 •
edited
Loading 

                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 •
edited
Loading 

                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

AlekseyTs commented Oct 6, 2023

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

This was referenced Oct 13, 2023
Closed
Closed
Open
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 16, 2023

Done with review pass (commit 23)

AlekseyTs
AlekseyTs approved these changes Oct 17, 2023
View reviewed changes
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

AlekseyTs commented Oct 18, 2023

@dotnet/roslyn-compiler For the second review

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

AlekseyTs commented Oct 20, 2023

@dotnet/roslyn-compiler For the second review

@jaredpar
Copy link
Member

jaredpar commented Oct 20, 2023

@333fred PTAL

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 23, 2023

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

333fred
333fred reviewed Oct 23, 2023
View reviewed changes
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
@allisonchou allisonchou mentioned this pull request Oct 24, 2023
Closed
@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

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred approved these changes

Assignees

No one assigned

Labels

Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

17.9 P1

4 participants

@jjonescz @AlekseyTs @jaredpar @333fred