Follow-up: Additional improvements needed for .NET Framework instrumentation type import (PR #1824)
Summary
PR #1824 addresses the core issue of ensuring type/method references use the target module's core library (mscorlib vs System.Runtime). However, the Copilot code review identified several additional improvements that should be implemented in a follow-up to ensure complete and robust handling of all type reference scenarios.
Background
This issue tracks the unresolved review comments from PR #1824 which fixes issue #1818. The current implementation correctly handles basic type imports but has some edge cases that need additional work.
Unresolved Items
1. Return Type Not Imported When Cloning ModuleTrackerTemplate Methods
Location: Instrumenter.cs lines 359-364
When cloning ModuleTrackerTemplate methods, the return type is copied from the template module (methodDef.ReturnType) without being imported into the target module's type system. This can leave return type references scoped to the coverlet assembly's core library (e.g., System.Runtime) when instrumenting .NET Framework targets.
Required Fix: Import/redirect the method return type using the same core-library helper used for parameters/locals.
2. Return Type Not Redirected for Updated Method References
Location: Instrumenter.cs lines 389-394
For method references moved onto the custom tracker type, updatedMethodReference uses methodReference.ReturnType directly (not imported/redirected). This can keep System.Runtime-scoped return types in the injected IL for .NET Framework targets.
Required Fix: The return type should be imported/redirected consistently with parameters (use the core-library import helper for the return type as well).
3. Generic Type Arguments Not Handled in ImportMethodToCoreLibrary
Location: Instrumenter.cs lines 462-485
ImportMethodToCoreLibrary only rewrites DeclaringType, parameter types, and return type element scopes. This misses nested type references for generic instance methods/types (e.g., Interlocked.Exchange<T> used in ModuleTrackerTemplate), where the generic arguments can still remain scoped to System.Runtime.
Required Fix: Consider recursively redirecting scopes for all nested TypeReferences:
- Generic arguments
- Modifiers
- Arrays/byref/pointers
- Avoid trying to set Scope on
GenericParameter types
4. Code Duplication with CoreLibMetadataImporterProvider
Location: Instrumenter.cs lines 439-456
The new ImportToCoreLibrary/ImportMethodToCoreLibrary logic largely duplicates the existing core-library redirection logic in CoreLibMetadataImporterProvider later in the same file.
Required Fix: Refactor to share a single implementation (e.g., a common helper that both paths call) to reduce the risk of future divergence.
Acceptance Criteria
Related
Labels
enhancementcoverlet-corenetfx
Follow-up: Additional improvements needed for .NET Framework instrumentation type import (PR #1824)
Summary
PR #1824 addresses the core issue of ensuring type/method references use the target module's core library (mscorlib vs System.Runtime). However, the Copilot code review identified several additional improvements that should be implemented in a follow-up to ensure complete and robust handling of all type reference scenarios.
Background
This issue tracks the unresolved review comments from PR #1824 which fixes issue #1818. The current implementation correctly handles basic type imports but has some edge cases that need additional work.
Unresolved Items
1. Return Type Not Imported When Cloning ModuleTrackerTemplate Methods
Location:
Instrumenter.cslines 359-364When cloning
ModuleTrackerTemplatemethods, the return type is copied from the template module (methodDef.ReturnType) without being imported into the target module's type system. This can leave return type references scoped to the coverlet assembly's core library (e.g.,System.Runtime) when instrumenting .NET Framework targets.Required Fix: Import/redirect the method return type using the same core-library helper used for parameters/locals.
2. Return Type Not Redirected for Updated Method References
Location:
Instrumenter.cslines 389-394For method references moved onto the custom tracker type,
updatedMethodReferenceusesmethodReference.ReturnTypedirectly (not imported/redirected). This can keepSystem.Runtime-scoped return types in the injected IL for .NET Framework targets.Required Fix: The return type should be imported/redirected consistently with parameters (use the core-library import helper for the return type as well).
3. Generic Type Arguments Not Handled in ImportMethodToCoreLibrary
Location:
Instrumenter.cslines 462-485ImportMethodToCoreLibraryonly rewritesDeclaringType, parameter types, and return type element scopes. This misses nested type references for generic instance methods/types (e.g.,Interlocked.Exchange<T>used inModuleTrackerTemplate), where the generic arguments can still remain scoped toSystem.Runtime.Required Fix: Consider recursively redirecting scopes for all nested TypeReferences:
GenericParametertypes4. Code Duplication with CoreLibMetadataImporterProvider
Location:
Instrumenter.cslines 439-456The new
ImportToCoreLibrary/ImportMethodToCoreLibrarylogic largely duplicates the existing core-library redirection logic inCoreLibMetadataImporterProviderlater in the same file.Required Fix: Refactor to share a single implementation (e.g., a common helper that both paths call) to reduce the risk of future divergence.
Acceptance Criteria
Interlocked.Exchange<T>Related
Labels
enhancementcoverlet-corenetfx