runtime: Analyzer rule CA2255 is conceptually broken and needs to be removed.

@CyrusNajmabadi is welcome in all discussions in this repo.

@masonwheeler

Github unfortunately does not have a link to flag inappropriate posts or report harassment

I understand that you disagree with the design, but I don’t appreciate switching to bad faith argumentation. @CyrusNajmabadi is hardly trying to harass you when he keeps pointing out that the behavior of the analyzer is by-design.

The anecdote from @ericstj provides evidence that our assumption holds: many people don’t fully understand how module initializers work, because the semantics of the runtime isn’t necessarily intuitive. As @jkotas pointed out static constructors have similar non-intuitive behaviors and over the years this has been a constant source of bug reports when subtle changes in user code cause them to no longer be called. We wanted to avoid people building complex architectures on top of module initializers because we know how fragile this can be. The point of the analyzer is to inform users that their mental model of how this feature works is very likely incorrect (or at least incomplete).

The fact that the semantics of module initializers work for you isn’t sufficient evidence that the analyzer is bogus; it just means that there are legitimate use cases for them, which we obviously don’t dispute since we recently added support for them.

I agree that asking people to suppress the analyzer when they are using them correctly is causing overhead but the cost of building a fragile architecture on top of them is much greater, so that trade off seems worth it to me.

The module initializer is how I avoid it. I picked that route specifically because I did not want to use reflection.

RunModuleConstructor is still reflection though. It’s not in the System.Reflection namespace, but that doesn’t make it any less reflection. GetUninitializedObject, RunClassConstructor, etc. are on the same boat. The implementation of RunModuleConstructor on CoreCLR is in ReflectionInvocation_RunModuleConstructor.

Module constructor is similar to a DllMain - it’s a special method executed under a lock. It’s better not to have one. People could do plugins in the native world by just having the plugin register itself on load from DllMain, but I’ve not seen anyone do that - there’s usually a well known entrypoint that the plugin host locates by name (one could say that’s reflection too) and calls that, mostly so that the plugin can do anything it needs without having to worry about being under a special lock.

This is also a topic of interest for me. And i find it very weird to have a position put forth that i should not engage with it. I was the original person discussing this topic with you @masonwheeler , on both roslyn-analzyers, and on gitter (where i shared useful information on how to disable these, as well as letting you know how you could stack editorconfig files to have both project and solution level suppressions take effect).

However, that we’ve been discussing this together from the start is not even relevant here. Our beliefs and CoC are strongly oriented around the idea that constructive and respectful discussion are part and parcel of how our repos work. I have shared my views and opinions on the topic and have respectfully been receptive to yours. All we ask is that you do the same with everyone who participates here as well. Thank you.

cc @jkotas @stephentoub

I personally couldn’t use ModuleInitializer for plugin registration, though I did really want to. The reason why it didn’t work for me was that it doesn’t actually run code until other code in the same module is run. You can do things like Assembly.Load and GetExportedTypes and the module initializer will not run. It’s not until you actually try to create a type or call a method that the initializer runs. At least for my plugin registration scenario, this didn’t make things any easier since I couldn’t use ModuleInitializer to achieve self-registration on load, and if I needed to invoke code in the module to trigger the module initializer, I just might as well have that code be responsible for initialization, or just add a static constructor to the type which the plugin discoverer was going to invoke to do my initialization since it would be less magic.

Here’s a sample: https://github.com/ericstj/sample-code/tree/ModuleInitializer/ModuleInitializer

That’s interesting! I didn’t know that. Now I’m curious: Why is this lock needed in order to run module initializers, and what restrictions does it impose that you need to know not to violate?

Module initializer must only run once. Nothing in the module is accessible from outside before module initializer runs. It’s similar restrictions to class constructors, except one needs to be able to reason about everything in the module, not just a single type.

1. it runs extremely slowly and 2) a developer can change things that make the code being reflected on invalid without giving you compile-time errors to notify you that you have to fix the reflection-based code.

RunModuleConstructor is a lot slower than just a method call too, but I don’t know your perf constraints. The assembly load is already plenty expensive that I don’t think it would make a big difference. As a counterargument to “without giving you compile-time errors” - RunModuleConstructor also doesn’t give that - it will run any module constructor on anything and the host won’t even know if it loaded a plugin. Using the attribute approach above would let the host detect that the thing that got loaded is not actually a plugin and maybe in shouldn’t be in the plugins directory and we shouldn’t run arbitrary code in it.

@masonwheeler

He was well aware that he is not welcome in this discussion before he ever made the first comment, and yet he did it anyway.

That’s not how this works. You don’t get to decide who is welcome to comment on issues in repos you don’t own, the owners of that repo do.

@CyrusNajmabadi is a key maintainer for Roslyn, a member of the C# design team, and thus has a lot of expertise with everything involving compilation and static code analysis. Not only are we dependent on getting his input, but we also actively seek it out.

Please change your attitude towards people holding contrarian viewpoints or we’re forced to block you. I disagree with your design proposal, but I also believe your technical feedback has merit and is worthwhile discussing. However, I’m not interested in talking to people who are acting hostile to members of our team.

tagging @stephentoub @jeffhandley . I’m not sure there’s actually a problem here. The analyzer seems to work towards its intended purpose. And users who understand what the analyzer is saying but still would like to proceed have several avenues available to them to suppress it. As we talked about offline dirs.props and editorconfig are trivial ways for you to inform the system that you are ok with the issues around module-initializers and do want to use them anyways.

In my opinion, module initializers are a poor answer to the plugin initialization problem, so I don’t think it’s worthwhile to attempt to force them to fit a problem they’re not really intended for. With the advent of generic attributes and abstract static members in interfaces, there is now a much nicer way to handle this problem:

public interface IPluginInitializer
{
    abstract static void Initialize();
}

public abstract class RegisterPluginAttribute : Attribute
{
    public Action InitializeMethod { get; }

    private protected RegisterPluginAttribute(Action initializeMethod)
    {
        InitializeMethod = initializeMethod;
    }
}

[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = true)]
public sealed class RegisterPluginAttribute<T> : RegisterPluginAttribute
    where T : IPluginInitializer
{
    public RegisterPluginAttribute()
        : base(T.Initialize)
    {
    }
}

Which is consumed as so:

[assembly: RegisterPlugin<SomePluginClass>]

sealed class SomePluginClass : IPluginInitializer
{
    static void IPluginInitializer.Initialize()
    {
        // ...
    }
}

Your application need only call GetCustomAttributes<RegisterPluginAttribute>() on the assemblies when you load them, and you’ll have access to each plugin’s static Initialize method. You’re also no longer bound by the public/internal accessibility requirements of [ModuleInitializer] this way.

In addition, you have full control over the initializer method’s signature when you take this approach. This has significant value because plugin initializers are typically going to require some kind of state (an interface exposing APIs, for example) to be passed in so that they can appropriately interact with the host program.

@terrajobst I can understand why it might appear to be “bad faith argumentation” in isolation. Suffice it to say, this is not an isolated incident, but a part of a much longer pattern of behavior stretching back years. He was well aware that he is not welcome in this discussion before he ever made the first comment, and yet he did it anyway. And continuing to insist that the behavior is by design, when I have already explained quite clearly that the design itself is flawed, is exactly the sort of passive-aggressive abuse that makes him unwelcome.

With regards to your technical points:

The anecdote from @ericstj provides evidence that our assumption holds: many people don’t fully understand how module initializers work, because the semantics of the runtime isn’t necessarily intuitive.

I don’t agree; it appears that he understands quite well how they work, and the problem is the unintuitive semantics actively getting in the way. As I noted above, this was something I pushed for fixing, by making module initializers run eagerly rather than lazily, and I still believe that would be an improvement that would cause significant gains with minimal, if any, tradeoff burden.

As @jkotas pointed out static constructors have similar non-intuitive behaviors and over the years this has been a constant source of bug reports when subtle changes in user code cause them to no longer be called. We wanted to avoid people building complex architectures on top of module initializers because we know how fragile this can be.

Again, I’d be willing to bet that 99% of those unintuitive, fragile behaviors can be laid directly at the feet of lazy initialization. (While I won’t advocate changing static constructors to eager initialization – there’s a lot of existing code out there that depends on the current semantics – in hindsight it was clearly a decision of questionable value!) But even without fixing module initialization semantics, I don’t believe those same problems apply to module initializers for a few reasons.

1. While they may not run eagerly, they run significantly more eagerly than static constructors do, being guaranteed to run before any other code in the module. There’s no chance, for example, of stumbling over a race condition on a code path that initializes one class before another.
2. Modules/assemblies (not technically the same thing but may as well be) are self-contained and provide a relatively strong isolation boundary that classes do not. This makes them significantly easier to reason about and significantly harder to accidentally fail to run.
3. Module initializers are obscure. While static constructors are in every 2-bit C# tutorial, you’re not going to find out about module initializers unless you actively go looking for them because you need functionality that’s kindasorta like static constructors but different. This isn’t the kind of feature that lends itself easily to being used by people with no idea what they’re doing.

It sounds like the analyzer is doing the right thing here then. It does seem like the docs should likely mention these cases though then so that users can know how to do things like “incorporating a call to RunModuleConstructor” as @masonwheeler mentioned.

Given the advanced nature as @jkotas mentioned, keeping things working as they are today (while improving docs around this rule) seems the most appropriate path forward.

That’s the entire point: its intended purpose is wrong. It is based on an incorrect premise.

I don’t see what’s incorrect. Reading the motivating thread, it seems sensible to me.

So how did it get overlooked entirely in the discussion that led to the creation of this analyzer?

I don’t think it was overlooked. I think this is exactly how most analyzers work. They detect potential problems, but are suppressible for the use cases where users can explicitly make a decision that for their domain it’s not a problem.

That seems more like wordplay than anything. It doesn’t have the problems that reflection has, namely, 1) it runs extremely slowly and 2) a developer can change things that make the code being reflected on invalid without giving you compile-time errors to notify you that you have to fix the reflection-based code.

Neither of these issues apply to the assembly-level attribute approach when used as shown above, so if that’s your reason for avoiding reflection you shouldn’t run into problems with that. Using abstract statics and generic attributes ensures that everything is validated at compile-time, so there’s no “magic” reliance on the structure of the code either (unlike typical approaches using reflection.)

There’s also this interesting nugget of information in RunModuleConstructor comments. We might want to lift that into the docs if the comment is still true (I can see the reasons why it could be true):

https://github.com/dotnet/runtime/blob/b83fee9f6e3442b9808eee3c436499c6f13a648e/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs#L64-L70

The above approach is simple, straightforward and statically verifiable as well. The only use of reflection is in GetCustomAttributes, which is essentially unavoidable because these assemblies are loaded dynamically.

Having been a witness for years to the same conversations between Cyrus and Mason where Mason claims trolling and abusive behavior, and talking extensively with both Cyrus and Mason on this topic in private, I don’t think Mason is seeing clearly. In my opinion there’s a hangup and Mason is projecting onto Cyrus things he’s run into elsewhere.

I guess not, now that I’m thinking about it, because state timing is important. I’m back to thinking that rooting things using module initializers is a feature not a bug, and it’s not (or shouldn’t be) that special compared to other things when it comes to diagnosing why linking isn’t working the way you want. Why should it be handled differently than a library that contains a method that calls GetMethod on a dynamic combination of type and name, or any other thing a library can do to cause unwanted rooting?

And even there, why can’t trimmers foreseeably become smart enough to handle module initializers safely, adding additional rooting or whatever hurdles the design requires?

Because the initializer is a root. Because for all the trimmer knows, this could be the entry point into a plugin and the entire point of the assembly is to provide the types that it’s referring to.

Honestly, I find the “it causes problems for trimming/linking” argument a little bit silly, particularly with the new emphasis .NET Core places on large numbers of small, highly specialized assemblies. If a module initializer is going to pull in code not relevant to the rest of the assembly, that kind of smells like an indication that this assembly could benefit from being broken up.

And even there, why can’t trimmers foreseeably become smart enough to handle module initializers safely, adding additional rooting or whatever hurdles the design requires?

There is no way for trimmers to know what depends on the module initializer side-effects. Trimmers have to unconditionally root the module initializers, there is no other reasonable alternative.

@CyrusNajmabadi Github unfortunately does not have a link to flag inappropriate posts or report harassment, so the thumbs down is really the only tool available to register my objection to your continued attempts to derail this conversation that you were never invited to.

@masonwheeler Can you clarify what the thumbs-down is for? It feels like the analyzer was appropriate here, calling out a concern here. As you yourself have mentioned, even your code needs to do additional stuff. So it feels like we should update our docs to call this out, and then allow you (and others) who have encountered this to take those steps, and then suppress the analyzer from that point using the simple approaches available.

Note. Discussed this with Mason in gitter. This sounds be pretty trivial to disable with a single line in a root editorconfig.