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

[masonwheeler]

I opened this in the analyzers repo, only to have it promptly closed with a request to open it here.

Diagnostic analyzer CA2255 escribes the ModuleInitializer attribute as being only for application initialization and "advanced code generator scenarios." But the most obvious use case for this feature is nothing of the sort: this was practically tailor-made for plugin registration.

Apparently this analyzer was created as a result of the discussion at #43268, which somehow fails to even consider the possibility that plugin systems exist and might want to use this. And... well... start from a bad premise and it's easy to reach the wrong conclusion.

Upon migrating from .NET 5 to .NET 6, every single plugin project in my Solution is being harassed by this new analyzer. It needs to stop existing, as it is based upon a fundamental misunderstanding of the way ModuleInitializer is being used in real-world code.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I opened this in the analyzers repo, only to have it promptly closed with a request to open it here.

Diagnostic analyzer CA2255 escribes the ModuleInitializer attribute as being only for application initialization and "advanced code generator scenarios." But the most obvious use case for this feature is nothing of the sort: this was practically tailor-made for plugin registration.

Apparently this analyzer was created as a result of the discussion at #43268, which somehow fails to even consider the possibility that plugin systems exist and might want to use this. And... well... start from a bad premise and it's easy to reach the wrong conclusion.

Upon migrating from .NET 5 to .NET 6, every single plugin project in my Solution is being harassed by this new analyzer. It needs to stop existing, as it is based upon a fundamental misunderstanding of the way ModuleInitializer is being used in real-world code.

Author:	masonwheeler
Assignees:	-
Labels:	area-System.Runtime.CompilerServices, untriaged
Milestone:	-

[CyrusNajmabadi]

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

[masonwheeler]

The point is not to hide the problem, but rather to fix it.

[CyrusNajmabadi]

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.

[masonwheeler]

The analyzer seems to work towards its intended purpose.

That's the entire point: its intended purpose is wrong. It is based on an incorrect premise. "As designed" isn't particularly meaningful when the design itself is faulty.

Frankly, this whole thing is a bit mystifying, given that I was active in the discussion surrounding the initial design of this feature and I mentioned plugin systems as a primary use case multiple times. So how did it get overlooked entirely in the discussion that led to the creation of this analyzer?

[CyrusNajmabadi]

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.

[masonwheeler]

I don't think it was overlooked.

It was never mentioned once. What do you call that?

[ericstj]

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

[jkotas]

Yes, it has always been the behavior of module initializers. It matches the behavior of regular static constructors. Loading a type e.g. using Type.GetType does not trigger the type static constructor either.

As the warning says, module initializers are only for advanced scenarios.

[masonwheeler]

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.

Yes, this is why, when the system was being set up, I advocated for a runtime change that would run module initializers eagerly rather than lazily. Unfortunately we didn't get that, but to be honest it's fairly easy to work around by incorporating a call to RuntimeHelpers.RunModuleConstructor into your plugin loader code.

[ericstj]

I guess in my case, if I would have to have the discoverer run RunModuleConstructor I'd just as soon create a higher fidelity contract that could pass in a logger / parameters / other-context to the initialization method. That way the discoverer is not running arbitrary code from modules that happened to be probed, and plugins have a better developer experience around initializing.

[CyrusNajmabadi]

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.

[CyrusNajmabadi]

@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.

[masonwheeler]

@ericstj What is this "discoverer" you're talking about? The stuff you say makes it sound like the plugin system is somehow groping about blindly through some swathe of the file system and checking every DLL it comes across, rather than the usual approach of having plugins set up in a well-defined-by-convention place that exists just for them.

[terrajobst]

@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.

[masonwheeler]

@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.

[danmoseley]

@CyrusNajmabadi is welcome in all discussions in this repo.

[jnm2]

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"

I agree with this. I'm still scratching my head over why module initializers are safer or more appropriate in app projects than in library projects.

I'd rather https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2255 talked about the specific gotchas such as Assembly.GetTypes()/reflection not running module initializers or static constructors and suggesting RunModuleConstructor or alternative approaches like Eric would suggest.

The part that I do think is excellent is this:

If library code declares a method with the ModuleInitializerAttribute, it can interfere with application initialization and also lead to limitations in that application's trimming abilities. Library code should therefore not utilize the ModuleInitializerAttribute attribute.

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? Can this paragraph talk about the current state of trimming rather than making a for-all-time statement about it?

[jkotas]

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.

[masonwheeler]

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.

[jnm2]

I also don't see the problem with saying that the module initializer roots things. Good! The trimmer should tell you if you're rooting things unnecessarily and there is a better option (like static constructor).

[masonwheeler]

@jnm2 That's an interesting idea. Can you think of an algorithmic way to define "necessary/unnecessary" in this context?

[jnm2]

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?

[terrajobst]

@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.

[masonwheeler]

@terrajobst Are you on Gitter? That might be a better place to discuss this. Suffice it to say, I have no problem whatsoever with "people holding contrarian viewpoints." As you can see from this thread, I'm perfectly capable of discussing things reasonably with people who disagree with me. There's significantly more to this than that.

[jnm2]

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.

[jnm2]

Mason, I have to stick to my morals knowing you won't like me sharing what I see. It doesn't bother me.

[CyrusNajmabadi]

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.

[DaZombieKiller]

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.

[masonwheeler]

@DaZombieKiller I'm afraid I don't follow. You're saying I should replace a registration system that's simple, straightforward, and statically verifiable with one that depends on reflection because it will work better? 😕

[DaZombieKiller]

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.

[masonwheeler]

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

[MichalStrehovsky]

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.

[MichalStrehovsky]

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):

runtime/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

Lines 64 to 70 in b83fee9

	// RunModuleConstructor causes the module constructor for the given type to be triggered
	// in the current domain. After this call returns, the module constructor is guaranteed to
	// have at least been started by some thread. In the absence of module constructor
	// deadlock conditions, the call is further guaranteed to have completed.
	//
	// This call will generate an exception if the specified module constructor threw an
	// exception when it ran.

[masonwheeler]

The implementation of RunModuleConstructor on CoreCLR is in ReflectionInvocation_RunModuleConstructor.

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.

Module constructor is similar to a DllMain - it's a special method executed under a lock. It's better not to have one.

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?

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

That's pretty much exactly how it's done in Delphi, except it's not DllMain; it's initialization code. (It's possible that initialization is run inside DllMain as a package is loaded, but I don't believe that's the case. I'd have to double-check to be sure, though.) initialization is essentially what I'm trying to emulate here, because it's so fantastically useful.

[DaZombieKiller]

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.)

[MichalStrehovsky]

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.

[jeffhandley]

Thanks for sharing those ideas, @DaZombieKiller. I had not considered static abstracts in interfaces and generic attributes as a good pairing for this, but that looks very approachable. 💯

I appreciate the discussion here, and it seems a good understanding is in place now for why we're discouraging the module initializer approach for plugins. I'm closing the issue.