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.

Sign up for GitHub

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

Jump to bottom

Creating new TypeName without reparsing + usage in NRBF #103713

Merged
merged 21 commits into from
Aug 14, 2024
Merged

Creating new TypeName without reparsing + usage in NRBF #103713

merged 21 commits into from
Aug 14, 2024

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jun 19, 2024 •
edited
Loading

@jkotas I've tried to implement the APIs we have discussed in #102263 and so far it looks good.

I need to spend more time testing MakeGenericTypeName and thinking about all edge cases.

contributes to #102263
fixes #106022

@adamsitnik adamsitnik added area-System.Reflection.Metadata binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it labels Jun 19, 2024
@adamsitnik adamsitnik self-assigned this Jun 19, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

This was referenced Jun 19, 2024
Open
Closed
Closed
@jkotas jkotas changed the title [DRAFT] More TypName APIs [DRAFT] More TypeName APIs Jun 20, 2024
@adamsitnik
Copy link
Member Author

I need to spend more time testing MakeGenericTypeName and thinking about all edge cases.

The MakeGenericType name from reflection works only for generic type definitions:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs

Line 431 in 9e57de2

if (!IsGenericTypeDefinition)

Should MakeGenericTypeName have similar limitation? Otherwise, we are going to allow for creating generic type names where typeName.IsConstructedGenericType is pointer, array etc. Such name would be illegal for runtimes we have today.

@jkotas
Copy link
Member

jkotas commented Jun 27, 2024

Should MakeGenericTypeName have similar limitation?

Sounds good to me. I assume that you are thinking about requiring IsSimpleName to be true. Correct?

@adamsitnik
Copy link
Member Author

Sounds good to me. I assume that you are thinking about requiring IsSimpleName to be true. Correct?

IsSimple or IsConstructedGenericType (so users could change generic args of given generic type)

@jkotas
Copy link
Member

jkotas commented Jun 27, 2024

IsConstructedGenericType (so users could change generic args of given generic type)

I do not think that regular reflection APIs allow this. Why would we want to allow it? If users want to change generic args of constructed generic type, they should get the generic type definition first on their side.

@adamsitnik adamsitnik mentioned this pull request Jun 28, 2024
Open
@adamsitnik
Merge remote-tracking branch 'upstream/main' into extendTypeName
7d2d0d6 
# Conflicts:
#	src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs
@adamsitnik
address API review feedback
9a0cd58
@adamsitnik adamsitnik reopened this Aug 2, 2024
@adamsitnik
remove CreateSimpleTypeName, introduce MakeSimpleTypeNameThrowsForNon…
aabe51b 
…SimpleNames:

- all TypeName instances remain always valid and escaped
- it's impossible to detect all sequences that are escaped
@build-analysis build-analysis bot mentioned this pull request Aug 2, 2024
Open
3 tasks
@build-analysis build-analysis bot mentioned this pull request Aug 12, 2024
Open
jkotas
jkotas requested changes Aug 13, 2024
View reviewed changes
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs Outdated
/// <param name="assemblyName">Assembly name.</param>
/// <returns>Created simple name.</returns>
/// <exception cref="InvalidOperationException">The current type name is not simple.</exception>
public TypeName MakeSimpleTypeName(AssemblyNameInfo? assemblyName)
Copy link
Member

Choose a reason for hiding this comment

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

If you believe that this name is wrong

Yes, I think that the method name is not right. The conceptual behavior of this method does not match any other Make... instance method in the BCL. The established BCL pattern for methods that change properties of immutable types is With... instance or extension method.

We consider BCL prior art when naming new APIs to maintain consistency. (MakeSimpleTypeName name would be ok if there was no BCL prior art.)

it does not suggest anything that it's going to work only for simple names.

I do not think that it is a problem. It is common for APIs to have additional limitations for how they can be used that are not captured in the API name. For example, WithDegreeOfParallelism can be used at most once per query, but this limitation is not captured in the API name.

The BF scenario is important to me and I am not convinced that I don't need it.

Do you agree that it is just a minor optimization, and that this exact API is not required to make the Nrbf-reader secure?

For Nrbf-reader security, you should only need to avoid non-linear amount of work that can be achieved via more general-purpose set of APIs.

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Aug 13, 2024
Open
3 tasks
@adamsitnik adamsitnik mentioned this pull request Aug 13, 2024
Merged
jkotas
jkotas approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

System.Reflection.Metadata part LGTM modulo comments. Thank you!

bartonjs
bartonjs previously requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Style violations must be corrected.

adamsitnik and others added 3 commits August 14, 2024 09:39
@adamsitnik @bartonjs @jkotas
Apply suggestions from code review
69cd1fa 
Co-authored-by: Jeremy Barton <[email protected]>
Co-authored-by: Jan Kotas <[email protected]>
@adamsitnik
don't use sbyte to store array rank, the limit of 32 can be changed i…
aa0870f 
…n the future and should not be enforced by the parser
@adamsitnik
fix the build
30be71e
@adamsitnik adamsitnik dismissed bartonjs’s stale review August 14, 2024 07:47

The suggestion has been applied and the feedback was addressed.

@adamsitnik adamsitnik merged commit cb42ed8 into dotnet:main Aug 14, 2024
81 of 84 checks passed
@build-analysis build-analysis bot mentioned this pull request Aug 14, 2024
Closed
@adamsitnik adamsitnik mentioned this pull request Aug 19, 2024
Merged
@jkotas jkotas mentioned this pull request Aug 26, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@bartonjs bartonjs bartonjs left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

Assignees

@adamsitnik adamsitnik

Labels
area-System.Reflection.Metadata binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: TypeName.MakeSimpleTypeName(AssemblyNameInfo) to avoid type name reparsing
4 participants
@adamsitnik @jkotas @JeremyKuhne @bartonjs