Added OriginalAccessModifierAttribute.

[CptMoore]

I recently had to work in Mono.Cecil with CustomAttributes, so I thought I'd try the same here.

Should solve #73.

If it works, it should look something like this:

[OriginalAccessModifier("public")]

I did not want to just output the original attributes, because nobody knows what NestedFamANDAssem means... so its all nicely mapped as per ms documentation to what we would have in the sources, see https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers .

Feel free to adjust directly the PR/sources as required or give feedback.

Note/TODO (in addition to krafs list):

• I did not test this at all, I don't even know how to.
• assemblyContext.AddOriginalAccessModifierAttribute is not being written to by anything, it is just true by default for now.
• need to add the fields to the attribute type too, otherwise the attribute is invalid. without those fields it still works for disassemblers (rider, dnspy) but probably will not be nice if actually loading the assembly (nvrmd, i think here its just an argument to the constructor without any persistence of the value. my other solution is fields based)

[krafs]

Thanks for contributing! Especially with the attribute and access modifier mapping.

I could help fill in some of the gaps in the PR, sure. You've already mentioned some things, but I'll list it here too:

• Tests. I'd like a test or two asserting that attributes are emitted as expected with the correct access modifier. This can probably be done similarly to the other tests, where an assembly is compiled at runtime, loaded with dnLib, and asserting the existence and values of the custom attributes on the members.
• The AddOriginalAccessModifierAttribute should be exposed as an msbuild property. I also think this should be opt-in, so the attribute is not emitted by default.
• AddOriginalAccessModifierAttribute needs to be considered in the assembly hash, so that changing the property value is recognized as a configuration change and triggers the task to re-process the assemblies.
• I'd prefer if we keep AssemblyEditor static and stateless. I understand the idea of wrapping it around the ModuleDef, but I think it's easier to reason about the code if we limit the number of places we store state. For example, we can probably store the attribute constructor in the PublicizeAssemblyContext instead.
• Documentation. A section that explains the feature should be added to the README.

Looks like I'll have some free time this weekend, so I can have a look.

[CptMoore]

• I think it's easier to reason about the code if we limit the number of places we store state

that is actually what i aimed for in a sense, before state was stored/manipulated at many different sites (site as in i have to scroll to find the location), even code being duplicated. Now it is more centralized, less duplication therefore fewer sites are needed.

but please feel free to optimize it in your way, for me this is just a PoC and i needed the code to be easier for me to understand and manipulate.

• probably store the attribute constructor in the PublicizeAssemblyContext instead

as long as it is per assembly, it should work. i copied this from my other solution which actually processing the same assemblies at different steps, and mono.cecil provides fast dict-based type lookup, so just re-selecting a type is a very natural and efficient solution there.

• so the attribute is not emitted by default

I would like users to have that attribute enabled by default, if not by default config, maybe by the main/first examples in the readme? The main point of the attribute it to make people aware of what they access, and beginners need that attribute more than others i believe.

BepInEx.AssemblyPublicizer, where the idea is form, also has it on by default.

[krafs]

It can definitely be helpful to beginners to see what has changed. I'm just concerned about existing users being upset their decompiled code suddenly being littered with attributes they weren't expecting.

I'll give it some thought. Good input 👍

[krafs]

I spent time investigating this last weekend. All tests failed. One of the reasons is because members are decorated with the attribute, but the attribute type is never added to the assembly. And adding the type to the assembly fails because new types can't be added while iterating over them. I did not get any further than that, but making sure the type is added outside the iteration is probably one thing that needs to be done. Also, I think we should have at least one test that asserts publicized members are decorated with the attribute. I am aware the test suite is complicated, mainly because there isn't a good framework for E2E testing for nuget packages.

I may come back and have another look, but time is scarce nowadays, so I can't promise anything.
Feel free to have another look.

Tests are executed as any normal test, e.g. dotnet test.
Debugging is weird. Your best bet is to add a Debugger.Launch(); in the code to force a breakpoint, but I think that only works if you're on Windows.

I'll put this PR in draft mode for now.

[CptMoore]

making sure the type is added outside the iteration is probably one thing that needs to be done

done, also fixed some issues making it more a valid attribute (better method and type attributes..)

I think we should have at least one test that asserts publicized members are decorated with the attribute

makes sense, if we get that far...

I am now getting this following issue (only 1/10 tests fail) after adding some proper method/type attributes:

.dotnet/sdk/9.0.101/Roslyn/Microsoft.CSharp.Core.targets(89,5): error : Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. [/tmp/66820e7e-ef2e-4f2f-83d7-22e5e9960221/App.csproj]
.dotnet/sdk/9.0.101/Roslyn/Microsoft.CSharp.Core.targets(89,5): error :    at Microsoft.CodeAnalysis.PEModule.HasAttributeUsageAttribute(EntityHandle token, IAttributeNamedArgumentDecoder attributeNamedArgumentDecoder, AttributeUsageInfo& usageInfo) [/tmp/66820e7e-ef2e-4f2f-83d7-22e5e9960221/App.csproj]

Could be anything:

• test loading or dnlib writing the wrong libs (corelib vs mscorlib)
• the custom attribute type being too incorrect for the test
  • wrong attributes on method or type
  • wrong structure, like dnlib not properly set up
• bug in Microsoft.CodeAnalysis.PEModule.HasAttributeUsageAttribute

[krafs]

I saw this too, I think. It happens in the test that Publicizes all assemblies the project references. I think the test adds the attribute type to System.Runtime or mscorlib. These assemblies are then passed to Roslyn with detects something invalid about them and fails. Just a theory though. Out of curiosity, does bepinex support publicizing System.Runtime and mscorlib, and adding the attribute?

[CptMoore]

I think the test adds the attribute type to System.Runtime or mscorlib.

I have added a workaround to not add the special attribute to core libs.

Out of curiosity, does bepinex support publicizing System.Runtime and mscorlib, and adding the attribute?

I don't know, I would assume it has the same issues as yours in that regard.

I tried adding a new test, but i would need at least reflective access to the publicized dll to check for the attribute, and the current tests work by letting the dotnet compiler do the access checks.

Is there a stable way to find the location of the publicized dll?

[krafs]

Kind of, we'd need to look in the obj/PublicizedAssemblies for the assembly. It's suffixed with a hash, but should be possible. You can see it be generated when you reference it in a project.

I would have liked if we could figure out why the standard library assemblies fail, instead of just special casing them. For example, System.Runtime seems to mostly contain type-forwarders to other assemblies. Maybe that messed up something, not sure.

[CptMoore]

Ok not sure when I can do it, but accessing the dll directly via reflection assembly load should allow for a quick inspection. Just need 2 checks, no attribute added for unchanged stuff, and attribute added if visibility was changed.

I would have liked if we could figure out why the standard library assemblies fail

For me this is not as big of an issue, reasons:

1. never seen any System* publicizers, I only see like 3-4 HarmonyX patches on System patches themselves. I think its just not important. i did it once and had issues with overloads.
2. if you publicize and use mscorlib, you cant even be sure the methods you publicizied are available on other platforms. Windows vs Linux vs MacOS have different internal methods in mscorlib, meaning mods could break platform independence more easily. So I would even exclude corelibs from PublicizeAll!
3. and if we make it work for net9, we cant guarantee any other .net version, os and .net platform combination will work. e.g mscorlib unity 2018 might work, corlib unity 6 on macos might not.

but i understand that it would be nice to know exactly why it does not work, maybe its not corelib related, and something else needs to be checked to avoid problems.

[krafs]

I know of at least a couple projects that publicize mscorlib for high performance optimizations.

The problem I see is that we don't know why these assemblies fail. We can special-case them, but if we don't know why they are failing then when we push this out in a new version people could break their projects because they are referencing some similar assembly that behaves the same way. We can't special-case every assembly that people report.

I had a quick look at the stacktrace from the previous failing test run, and it seems to be failing when Roslyn inspects the attribute type we add to System.Runtime. Specifically, a NullReferenceException is thrown when Rolsyn calls HasAttributeUsageAttribute. That tells me Roslyn is checking for the AttributeUsageAttribute on our custom attribute. But there is no AttributeUsage. Maybe we should add it?

Also, isn't the attribute type incomplete? From what I have seen in other projects creating custom attributes, those are adding base calls to the Attribute base constructor. Maybe Roslyn needs these things to consider the attributes proper attributes? Just guessing.

[CptMoore]

That tells me Roslyn is checking for the AttributeUsageAttribute on our custom attribute. But there is no AttributeUsage. Maybe we should add it?

Our attribute inherits from the corlib base attribute, which has the AttributeUsageAttribute as inherited.

We can't special-case every assembly that people report.

If that really is a concern, maybe we should just make it configurable. I think the IsCorlib check anyway is hardcoded against a list of assemblies (or if it finds System.Object defined in the assembly).

((asmName = UTF8String.ToSystemStringOrEmpty(asm.Name)).Equals("mscorlib", StringComparison.OrdinalIgnoreCase) ||
 asmName.Equals("System.Runtime", StringComparison.OrdinalIgnoreCase) ||
 // This name could change but since CoreCLR is used a lot, it's worth supporting
 asmName.Equals("System.Private.CoreLib", StringComparison.OrdinalIgnoreCase) ||
 asmName.Equals("netstandard", StringComparison.OrdinalIgnoreCase) ||
 asmName.Equals("corefx", StringComparison.OrdinalIgnoreCase));

I am pretty sure its an issue with where System.Attribute is defined though, keeping it a dynamic check might still be preferable.

base calls to the Attribute base constructor.

Doesn't seem to make a difference. The base constructor is empty and I also don't see any fields being initialized. Maybe in the future that changes, so it is prudent to make sure the base constructor is called anyway.

[krafs]

Configurable? Has that not always been the idea? Being able to enable/disable emitting the attribute? Or am i misunderstanding something?

[CptMoore]

Configurable? Has that not always been the idea? Being able to enable/disable emitting the attribute? Or am i misunderstanding something?

Originally I would have said to just have an on/off switch globally, default on.

Now, in addition, I proposed to have it a list of excludes (mscorlib,System.Runtime,etc..) instead of hardcoding the skip based on what dnlib views to be a corelib (i personally still like deferring that to dnlib, but i also trust people to create issue tickets in case this causes problems).

[krafs]

We can start by omitting the core libraries with dnlib and see how that works out 👍