Arguments to Custom Attributes Do Not Update Upon Type Rename

[djhohnstein]

AsmResolver Version

4.11.2

.NET Version

.NET 6.0

Operating System

Windows

Describe the Bug

Class types that use a custom attribute type decorator do not update the class type's custom attribute's fixed arguments to reflect the attribute type's new name upon renaming.

This can be explained better with code. Let's suppose we have two type definitions (TypeDefinition) for classes ClassA and ClassB. ClassB has an attribute decorator of MyCustomAttribute which takes one argument, a type. ClassB is instantiated as follows:

[MyCustomAttribute(typeof(ClassA))]
public class ClassB {
    ....
}

Let's say the TypeDefinition variable for ClassA is stored as a variable as follows:

TypeDefinition tDefForClassA = Module.GetAllTypes().Where(x => string.Equals(x.Name?.Value, "ClassA"));

If I were to rename ClassA via:

tDefForAttr.tDefForClassA = "ClassC"

You would see that the arguments for the CustomAttributes field on ClassB's TypeDefinition object does not update to reflect the new argument of ClassC.

How To Reproduce

Create an example project with the following file:

using System;

namespace TestProject
{
    [AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
    class MyCustomAttribute : Attribute
    {
        public Type Type { get; set; }

        public MyCustomAttribute(Type outputType)
        {
            Type = outputType;
        }
    }

    internal class ClassA
    {
        
    }

    [MyCustomAttribute(typeof(ClassA))]
    internal class ClassB
    {
        internal void HelloWorld()
        {
            Console.WriteLine("Hello, world!");
        }
    }
    
    internal class Program
    {
        
        public static void Main(string[] args)
        {
            var b = new ClassB();
            b.HelloWorld();
        }
    }
}

Compile to TestProject.exe. Then, in a separate project leveraging AsmResolver, use the following code to update the ClassA type definition name:

var module = ModuleDefinition.FromFile("TestProject.exe");
TypeDefinition tDefClassA = module.GetAllTypes().First(x => x.Name == "ClassA");
tDefClassA.Name = "ClassC";
_editor.Module.Write("SecondTest.exe");

If you were to inspect ClassB's type definition via:

TypeDefinition tDefClassB = module.GetAllTypes().First(x => x.Name == "ClassB");
// Inspect tDefClassB.CustomAttributes.Signature

You would see that the signature here still references ClassA which no longer exists in the assembly.

Expected Behavior

The arguments to custom attribute decorator of classes getting updated as required. For example, ClassC should now populate the arguments of the signature in the previous code instead of ClassA.

Actual Behavior

ClassA remains referenced as an argument to the attribute constructor even though this type no longer exists.

Additional Context

No response

[djhohnstein]

I'm running out the door so apologies in advance if this isn't enough context. Happy to provide more if required.

[Washi1337]

Thanks for the detailed report. The reason this happens is because the binary format of custom attribute (CA) signatures is weird (read: incredibly stupid).

Unlike everything else in the .NET file format, CAs store type arguments as fully qualified name strings as opposed to metadata tokens. AsmResolver currently parses these strings and creates TypeSignatures that are completely separate entities not linked to their corresponding TypeDefinition. Updating the type definition will therefore not update the CA type argument and vice versa.

We could consider refactoring TypeNameParser to auto-resolve the parsed types to proper definitions. This consideration needs to be made carefully however, as this will probably mean (partially) reverting #351 or else we cannot guarantee all type signatures in CAs are linked. Reverting this is likely to reduce performance significantly as well as hinder UX (#336).

Open for suggestions.

[BadRyuner]

In the Name & Namespace properties of TypeDefinition, add: if (check is attribute type) { get all CA & rename all typesigs in CA }

[Washi1337]

In the Name & Namespace properties of TypeDefinition, add: if (check is attribute type) { get all CA & rename all typesigs in CA }

Note: We need to keep in mind that any type T can be put into a CA argument using typeof(T), not just types that inherit from Attribute.

[djhohnstein]

I unfortunately don't have any recommendations on how to approach this problem; however, I do really appreciate the thorough response. I could attempt to go about tracking/updating this in my own project if there's some function calls that are exposed for me to update those arguments. Otherwise, I'll just document it as "unsupported" in my project and move on.

Also, huge fan of the project. It's a tremendous accomplishment.

[nike4613]

We could consider refactoring TypeNameParser to auto-resolve the parsed types to proper definitions. This consideration needs to be made carefully however, as this will probably mean (partially) reverting #351 or else we cannot guarantee all type signatures in CAs are linked. Reverting this is likely to reduce performance significantly as well as hinder UX (#336).

It could be an opt-in specification during loading, or lazily inferred by setting .Name or .Namespace. That would avoid the perf hit for pure analysis, while remaining (or giving the option to remain) correct for modification.

[ds5678]

Or maybe ModuleDefinition.ResolveCustomAttributeArgumentTypeReferences()

[ds5678]

However, this ultimately doesn't seem much different from external references. If you rename a type in one module, the references in another module are not (currently) updated.

[nike4613]

However, this ultimately doesn't seem much different from external references. If you rename a type in one module, the references in another module are not (currently) updated.

I would expect that to change, with workspaces, right? Isn't that kind of tight coupling the point?

[Washi1337]

It could be an opt-in specification during loading, or lazily inferred by setting .Name or .Namespace. That would avoid the perf hit for pure analysis, while remaining (or giving the option to remain) correct for modification.

Or maybe ModuleDefinition.ResolveCustomAttributeArgumentTypeReferences()

Lazy inference, if possible/feasible, sounds like the best option from an UX standpoint. I would like to avoid adding yet another option/flag/method that can easily be forgotten by an end-user.

I would expect that to change, with workspaces, right? Isn't that kind of tight coupling the point?

Sort of. Workspaces (if it ever takes off) is supposed to be a separate analysis/refactoring layer on top of the current metadata model. AsmResolver.DotNet and friends should be relatively bare bones where you can change anything you want however you want, including changing things incorrectly.

In this perspective, there is an argument to be made to keep things as-is, though for better UX we probably shouldn't as this affects two type descriptors defined in the same module.

---

Side note: When linking serialized type signatures in CAs to serialized type definitions in metadata, we may want to consider the original namespace+name pair of the type definition, or else users may rename the type definition first before initializing the CA signature and thus break the link before AsmResolver gets the chance of linking the related types together. As this pertains to just serialized metadata only, we can probably get this information from the underlying type def row and backing #Strings stream, and thus likely do this without adding extra tracking logic/fields to TypeDefinition.