Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. #111646

teo-tsirpanis · 2025-01-21T00:50:57Z

Fixes #60380. Implementation follows the corresponding methods for method and field definitions, except that the binary search is performed on rows of the PropertyMap and EventMap tables for properties and fields respectively, and we don't account for rows with no properties/events.

ghost · 2025-01-21T00:51:01Z

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.

ghost · 2025-01-21T00:51:03Z

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.

Copilot

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeSystem/PropertyDefinition.cs:72

The new method GetDeclaringType should be covered by tests to ensure its correctness.

public TypeDefinitionHandle GetDeclaringType()

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.cs:1370

Ensure that the new method GetDeclaringType for PropertyDefinitionHandle is covered by tests.

return PropertyMapTable.FindTypeContainingProperty(propertyDef.RowId, PropertyTable.NumberOfRows);

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.cs:1360

Ensure that the new method GetDeclaringType for EventDefinitionHandle is covered by tests.

return EventMapTable.FindTypeContainingEvent(eventDef.RowId, EventTable.NumberOfRows);

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeSystem/EventDefinition.cs:60

The new method GetDeclaringType should be covered by tests to ensure it works as expected.

public TypeDefinitionHandle GetDeclaringType()

We have to return the value of the parent type column in the map table, not the row to the map table itself.

teo-tsirpanis · 2025-01-26T02:40:29Z

@dotnet/area-system-reflection-metadata this is ready for review.

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Internal/Tables.cs

steveharter · 2025-01-28T16:47:29Z

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeSystem/TypeDefinitionTests.cs

+            foreach (var typeDefHandle in reader.TypeDefinitions)
+            {
+                var typeDef = reader.GetTypeDefinition(typeDefHandle);
+                foreach (var nestedTypeHandle in typeDef.GetNestedTypes())


Can we add a sanity check here to make sure that these foreach statement actually loop through at least one? Like:

Assert.True(typeDef.GetNestedTypes().Length > 0);

The test assembly actually dosen't have any nested types. I tried to add some but after recompiling it this assert failed.

Your suggested assert wouldn't currently work, because not all types have all kinds of members each.

Added asserts that GetTypesWithEvents() and GetTypesWithProperties() are not empty, and commented out the asserts on the nested classes.

We assert that the test assembly has types with events and properties, and acknowledge that it does not have nested types.

jkotas · 2025-02-09T01:20:56Z

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeSystem/TypeDefinitionTests.cs

+            foreach (var typeDefHandle in reader.TypeDefinitions)
+            {
+                var typeDef = reader.GetTypeDefinition(typeDefHandle);
+                // The assembly does not declare any nested types yet.


Would it be better to add some nested types to this assembly instead?

Added a test.

jkotas

LGTM

jkotas · 2025-02-09T01:24:47Z

@MichalStrehovsky Do you know on top your head where there any places in the AOT compilers that can use these APIs? If yes, we should create an issue to adopt these APIs once they make it into the tools runtime refence assemblies.

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeSystem/TypeDefinitionTests.cs

This reverts commit 2013a59.

jkotas · 2025-02-10T00:15:18Z

/ba-g unrelated timeout

jkotas · 2025-02-10T00:17:02Z

Thank you!

MichalStrehovsky · 2025-02-10T06:43:36Z

@MichalStrehovsky Do you know on top your head where there any places in the AOT compilers that can use these APIs? If yes, we should create an issue to adopt these APIs once they make it into the tools runtime refence assemblies.

Nothing comes to mind. We ran into lack of this API in the ILTrim hackathon.

* main: (41 commits) Automated bump of chrome version (dotnet#112309) Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. (dotnet#111646) Update the System.ComponentModel.Annotations solution to build in VS (dotnet#112313) JIT: initial support for stack allocating arrays of GC type (dotnet#112250) [main] Update dependencies from dotnet/roslyn (dotnet#112260) Update Xcode casing (dotnet#112307) update the location of assert for REG_ZR check (dotnet#112294) Enable `SA1206`: Keyword ordering (dotnet#112303) Address feedback on dense FrozenDictionary optimization (dotnet#112298) Start regular pri-1 tests runs with native AOT (dotnet#111391) Observe exceptions from _connectionCloseTcs (dotnet#112190) Test failure - SendAsync_RequestVersion20_ResponseVersion20 (dotnet#112232) Fix init race in mono_class_try_get_[shortname]_class. (dotnet#112282) Remove repeated call to DllMain (dotnet#112285) Replace bitvector.h/cpp with ptrArgTP type in gc_unwind_x86.h/inl (dotnet#112268) JIT: Limit 3-opt to 1000 swaps per run (dotnet#112259) [main] Update dependencies from dotnet/icu, dotnet/runtime-assets (dotnet#112120) Update dependencies from https://github.com/dotnet/emsdk build 20250205.3 (dotnet#112223) Fix EventPipe on Android CoreClr. (dotnet#112270) Fix exception handling in the prestub worker (dotnet#111937) ...

ghost added area-System.Reflection.Metadata new-api-needs-documentation labels Jan 21, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 21, 2025

teo-tsirpanis requested a review from Copilot January 21, 2025 00:53

Copilot AI reviewed Jan 21, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jan 21, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

teo-tsirpanis marked this pull request as ready for review January 25, 2025 21:06

teo-tsirpanis added 4 commits January 25, 2025 23:35

Add GetDeclaringType to PropertyDefinition and EventDefinition.

fbc42b2

Add test.

e1cfa01

Add documentation to all GetDeclaringType APIs.

1a5769c

Fix tests.

4779643

We have to return the value of the parent type column in the map table, not the row to the map table itself.

teo-tsirpanis force-pushed the srm-declaring-type branch from 74a48f0 to 4779643 Compare January 25, 2025 23:04

ericstj assigned steveharter Jan 27, 2025

jkotas reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Internal/Tables.cs Outdated Show resolved Hide resolved

steveharter reviewed Jan 28, 2025

View reviewed changes

steveharter added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 5, 2025

teo-tsirpanis added 2 commits February 6, 2025 03:35

Remove unnecessary code.

b908468

Update tests.

f421e9c

We assert that the test assembly has types with events and properties, and acknowledge that it does not have nested types.

dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 6, 2025

teo-tsirpanis requested a review from steveharter February 6, 2025 02:03

This was referenced Feb 6, 2025

WasmTestOnChrome-MT-System.Runtime.InteropServices.JavaScript.Tests CI timeouts #104203

Closed

[browser][MT] System.Runtime.InteropServices.JavaScript.Tests CI timeouts #104492

Open

WasmTestOnChrome-MT-System.Net.Http.Functional.Tests timeout #106094

Open

jkotas reviewed Feb 9, 2025

View reviewed changes

jkotas approved these changes Feb 9, 2025

View reviewed changes

Test TypeDefinition.GetDeclaringType().

2013a59

jkotas reviewed Feb 9, 2025

View reviewed changes

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeSystem/TypeDefinitionTests.cs Outdated Show resolved Hide resolved

jkotas added 2 commits February 9, 2025 11:50

Revert "Test TypeDefinition.GetDeclaringType()."

d362dad

This reverts commit 2013a59.

Add test for nested types

8fd3b53

jkotas merged commit 1c3b1b5 into dotnet:main Feb 10, 2025
82 of 85 checks passed

teo-tsirpanis deleted the srm-declaring-type branch February 10, 2025 02:17

teo-tsirpanis added this to the 10.0.0 milestone Feb 11, 2025

github-actions bot locked and limited conversation to collaborators Mar 13, 2025

Add GetDeclaringType to PropertyDefinition and EventDefinition. #111646

Add GetDeclaringType to PropertyDefinition and EventDefinition. #111646

Uh oh!

Conversation

teo-tsirpanis commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 21, 2025

Uh oh!

ghost commented Jan 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis commented Jan 26, 2025

Uh oh!

Uh oh!

steveharter Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Feb 9, 2025

Uh oh!

Uh oh!

jkotas commented Feb 10, 2025

Uh oh!

Uh oh!

jkotas commented Feb 10, 2025

Uh oh!

MichalStrehovsky commented Feb 10, 2025

Uh oh!

Uh oh!

Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. #111646

Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. #111646

teo-tsirpanis commented Jan 21, 2025 •

edited

Loading

jkotas Feb 9, 2025 •

edited

Loading