OriginalVisibilityAttribute

[CptMoore]

I really find it useful to see in the de-compiled code the original access modifier of a method/field/type, as BepInEx.AssemblyPublicizer provides it.

It looks like this:

    [OriginalAttributes(FieldAttributes.Private)]
    public DataManager dataManager;

That is what is embedded into the publicized dll:

namespace BepInEx.AssemblyPublicizer
{
  internal sealed class OriginalAttributesAttribute : Attribute
  {
    public OriginalAttributesAttribute([In] TypeAttributes obj0)
    {
    }

    public OriginalAttributesAttribute([In] MethodAttributes obj0)
    {
    }

    public OriginalAttributesAttribute([In] FieldAttributes obj0)
    {
    }
  }
}

The naming could be better, I'd suggest OrignalAccessModifierAttribute...

[krafs]

I've been meaning to do something like this, but haven't gotten around to it. It's a good idea!
I'd probably want to keep it simple, but I see a few different approaches:

Single attribute with enum

[OriginalVisibility(Visibility.Private)]
public void Foobar() { }

[PublicizedFrom(AccessModifier.Private)]
public void Foobar() { }

or, like you suggested

[OriginalAccessModifier(AccessModifier.Private)]
public void Foobar() { }

Single attribute with string

[OriginalVisibility("private")]
public void Foobar() { }

Separate attribute per modifier (5 in total)

[OriginallyPrivate]
public void Foobar() { }

[OriginallyInternal]
public void Foobar() { }

What do you think? I think your suggestion is quite good actually, but let me know if you have any more thoughts.

[CptMoore]

I would keep the naming after how Microsoft officially names it, so something with "access modifier" it should be.. ( like so people can find the link themselves in the future just by keyword search)

Using publicized in the name also makes it clear its from the publicizer, so PublicizedFrom(AccessModifier. ... ) is actually nice but "From" is abit confusing (email from, range from.. too many meanings in that word already) and redundant. why not just Publicized(AccessModifier.ProtectedInternal) ? It tells me the publicizer changed something here, and it was that access modifier before.

I would have only one attribute and not separate attributes, as I could just do a find usages on the attribute and find all locations in the IDE or assembly viewer. At least i find it more useful than having it separated.

String is the worst due to loss of static analysis. Enums still work great here, the BepInEx example was from the disassembler im using and it clearly showed the enums nicely, and you can even find usage on them.

[krafs]

Looking at this issue a year later I'm trying to see the value of it.
For people using BepInEx with this feature - what value does it add?

[CptMoore]

When BindingFlags are involved, this feature would be very useful.

I have no clue what bindingsFlags I need to set to get the correct methods, as they are all public for the decompiler, while in reality some might be private or internal.

    [HarmonyTargetMethods]
    internal static IEnumerable<MethodBase> TargetMethods()
    {
        return typeof(JSONSerializationUtility).GetMethods(BindingFlags.Public); // wrong
    }

Since the original visibility flags were lost, I now need to either find the original DLL and run it through an external decompiler, or temporarily disable the publicizer and enable it again later.

[krafs]

That's an interesting use case. But BindingFlags are not really a problem with Harmony. Harmony has AccessTools as the convenient reflection abstraction, which disregards BindingFlags. And if wanting to go for pure reflection, one can still use AccessTools.all as the BindingFlags in GetMethod and not have to care whether a member is public or private.

I'm not completely against adding something like this, but I would like to limit the amount of assembly editing we do, and keep it simple if possible.

Unless the value makes it worthwhile, of course.

[krafs]

I guess being able to see that you're overriding a publicized, non-public method could be useful to see, so you can go exclude that from being publicized. But I haven't heard anyone really complaining much about that 🤔

[CptMoore]

I was looking at method overloads yesterday, and it was important to know which were the important methods.
In the end there was just one private method, found that out by disabling and re-enabling the publicizer.
Then had to patch all generic version of said private method.

It is mainly a convenience feature:

• More quickly parse decompiled code (know whats internal code, skip the rest)
• Easier detect issues with subclassing (can then even be done by some to-be-written analysis tool)
• Easier target methods (as visibility modifiers can be used for targeting the exact methods, e.g. in case of overloads)

[krafs]

How do you mean it helps targeting a specific overload? You can't use the access modifier as a condition on the Publicized items in the csproj.

Regardless, it does sound like a feature like this could be valuable. I'll keep it in the backlog. Busy times coming up, so not sure when or if I can get to it though.

[CptMoore]

How do you mean it helps targeting a specific overload? You can't use the access modifier as a condition on the Publicized items in the csproj.

I meant it is easier for the human having to target a method. If I know it was originally private, I can ask the code to just target the private version of the method. The code base I am currently looking at, has many such examples, where there are multiple static public convenience methods and a single static private method that goes along.

Anyway, thanks for keeping it in the backlog.