Test plan for "module initializers" feature #40500
Comments
|
Based on dotnet/csharplang#2608, I was also thinking there would be tests that the obsoletion error would be suppressed when targeting C# 9. What about when you're using C# 8 and declaring your own ModuleInitializerAttribute and you leave out the ObsoleteAttribute? Should the compiler error when you apply that to the module even though there is no obsoletion error? Also, what if you declare your own ModuleInitializerAttribute with incorrect attribute usage and apply it e.g. to the assembly instead of to the module, or apply it to the module twice? |
gafter
added
Test-Gap
|
@jcouv Should I submit a PR to add LanguageVersion.CSharp9 yet? |
|
@jcouv I have a success case working! Next I'll add tests and implement diagnostics. master...jnm2:module_initializer |
|
@jcouv Should I wait till then to implement diagnostics? |
|
Probably safer. |
|
Although we have a draft spec for the feature, there are at least four or five different viable approaches to solving the problem in the language, and I don't have confidence that the draft spec is the approach we'll end up selecting. |
|
Should I proceed to implement the conclusion section of https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-04-08.md#module-initializers, or is it still too soon? |
|
@jnm2 The agreement on design seemed pretty firm (all static methods marked with an attribute contribute to module initialization). I think it's okay to go ahead. Tagging @RikkiGibson @jaredpar @gafter as FYI. |
|
Implementation questions:
Which of these error situations should be ignored and how should diagnostic IDs be assigned to the rest?
Just being dumb now if I wasn't before:
|
Probably not, because we will need to emit a call to it in the module initializer method, which as far as I can tell is just a special method on a special class.
Yes, it it also not permitted to have parameters
I don't see anything that should block this, but it is also pretty easy to wrap an extern method in some static method that you then mark with
Don't know exactly where this lives in the compiler, but the examining the behavior for static fields in partial classes is probably a good starting point.
Yes. I believe you add a member to the MessageID enum for the feature and then you modify the switch statement that indicates the required language version for the feature.
Error on these.
No error on these. We generally accept well-known attributes that are user-declared, as long as they match the shape to the extent we need. This is likely to be how your tests will work as well.
Error on this.
It's okay to let AttributeUsage help you automatically where it can and add special cases to the compiler just to fill in the gaps. For a ModuleInitializerAttribute placed on a non-method target, I recommend silently ignoring the attribute.
I recommend silently ignoring the redundant attributes.
I don't see how this can affect anything with static methods.
Only usage of well-known attribute constructors should affect compiler behavior. Therefore you should silently ignore
I think we should, for the scenario that you are using an old target framework and old language version with a new SDK and you try defining the attribute manually.
There's no problem with doing this. I would expect it to add a call to the static method within the generated module initializer. This would be a good test. |
|
Sorry if some of the questions got out of sync while I was writing. Let me know if you need further clarification on anything. |
We discussed this briefly but I don't think we answered it. There are pros and cons to permitting them
class A
{
private class B
{
[ModuleInitializer]
private static void M() { ... }
}
}would have to translate to something like void __ModuleInitializer() { A.<helper>(); } // generated
class A
{
internal static void <helper>() { B.<helper>(); } // generated
private class B
{
internal static void <helper>() { B.M(); } // generated
[ModuleInitializer]
private static void M() { ... }
}
} |
|
@gafter Is that something that would be safe to ban (allocating a diagnostic ID to it) for the time being? Hey, just thought of two more cases:
|
|
@jnm2 I would assume only accessible methods can be used for now (simpler). We'll confirm with LDM by email and share the answer back. |
|
I agree, looking at my notes I think we did decide that the method has to be accessible at the module level (effectively |
|
I'm ready for feedback on #43281. |
|
What does "calling in a deterministic order" mean? Deterministic across which variables? Source member reordering, source file reordering, moving members between partial classes, or simply compiler version changes? |
|
These are all the new diagnostic IDs I'm planning to add. Should any be combined? ERR_ModuleInitializerMethodMustBeInternalOrPublic = 8793,
ERR_ModuleInitializerMethodMustBeStatic = 8794,
ERR_ModuleInitializerMethodMustNotHaveParameters = 8795,
ERR_ModuleInitializerMethodMustReturnVoid = 8796,
ERR_ModuleInitializerMethodMustNotBeGeneric = 8797,
ERR_ModuleInitializerMethodMustNotBeContainedInGenericType = 8798, |
|
I think I'll have to change
internal class C
{
private class Nested
{
[ModuleInitializer]
internal static void M() { }
}
} |
|
Should there be some warning for [ModuleInitializer] on a static/nonstatic local function? |
I suggest erroring on ModuleInitializer on a local function because the local function will not be accessible.
Sounds good.
It's dealer's choice on this one, but consider combining 8795 and 8796, as well as 8797 and 8798.
It just means that when we compile with the same set of inputs, we should produce the calls in the same order every time. Once you make changes to the inputs we may call the module initializers in a different order. I suggest trying to do file+source position ordering, similar to how static initializers in partial classes work. Let me know if you need more specific guidance and I can try to look more deeply into how this works. |
|
Right now the same check that is causing local functions to be ignored is also what is causing operators/constructors/finalizers/accessors to be ignored. Should I produce an error for local functions and nothing else, or should I produce an error for more of these? (All valid 'method' attribute targets.) Ah, so the variable to guard against with deterministic ordering would be concurrency during compilation. |
|
I think of determinism as “run the same command with same inputs, get the same outputs”. Changing the order of inputs is changing an input.
Yes |
Agree. Even a
I think we should draw lessons here from |
|
Just to clarify, the question is whether to ignore [ModuleInitializer] on local functions entirely or to error. I'm currently ignoring them entirely because that's what fell out of the code I used to ignore [ModuleInitializer] entirely for operators, accessors, and finalizers per discussion above. (Early exit if targetSymbol.MethodKind != MethodKind.Ordinary.) I'm wondering if I should special-case local functions so that an error is produced while the other targets are still ignored, or if I should just produce an error for every target where MethodKind != Ordinary. I'm not sure, but I might have misunderstood @RikkiGibson here. I read this as saying [ModuleInitializer] should be silently ignored on operators, finalizers, and accessors (all of which are allowed by AttributeTargets.Method, but none of which are considered C# method syntax):
|
|
I was trying to describe cases where AttributeTargets would normally cause an error due to being placed on e.g. a type and not a method. No need to try and protect the user from themselves if they declare the attribute class with an unexpected AttributeTargets argument. Since AttributeTargets can't disallow usage on operators, finalizers, accessors, local functions, etc, let's have a diagnostic for when |
|
Thank you. I really appreciate your patience! |
|
@jcouv #43281 is about to merge which completes this item in your list:
[edit: oops, two of the items are part of the second PR; removed] What do you think about adding these items?
|
|
@jnm2 Added those two bullets. Thanks |
|
@jcouv Now that the language proposal, runtime spec, and initial implementation PR are merged, I opened dotnet/runtime#35749 as specified in your test plan above. |
|
@jnm2 this one could be a bit of a pain: "No warning when module initializer method initializes a static field with a non-nullable reference type" The compiler today expects that a field will be initialized in the same type it is declared in, either in an explicit constructor or with a field initializer. But with ModuleInitializerAttribute you could initialize an accessible static field in any type. I don't think we actually have a precedent for flow analyzing multiple methods to find out the initialization state of multiple types. We could potentially have special handling for a module initializer that is within the type that declares the field, treating it as running after the initializers/static constructor (because if the module initializer is really just a method on that type, its static constructor will run before the method). But people may get confused regardless if they have good reason to initialize the field from some other type. I think the thing to do here may be to ask that people initialize their non-nullable reference static fields with |
|
@RikkiGibson That makes sense to me. /cc @YairHalberstadt who brought up the scenario on Gitter |
|
Hi, looks like a great feature. Was just wondering - whether you are planning to add some sort of dependency feature between modules (like execute this init procedure only after that module has been initialized)? Thanks! |
@valdisiljuconoks That kind of dependency does not make sense, a module needs to be initialized before code from that module is executed, so delaying initialization until some condition is true is not possible (you wouldn't be able to execute dependent code) Module initialization however should already be lazy and only done when you try to execute something within the module (I don't know which exact conditions trigger module initialization). If you have a dependency that some other module be initialized before yours, even if you don't execute code from it, then you can easily encode this through a call to RuntimeHelpers.RunModuleConstructor (you can get the module reference by referencing a type in the assembly, or by following and resolving your own modules assembly references) |
Notably, a module initializer is not executed via Assembly.GetTypes(). This means that the assembly that GetTypes is being called on can't use module initializers to hook AssemblyResolve in time to avoid type load exceptions. See dotnet/csharplang#2608 (comment) and my comment below which links demos of this for .NET Core and .NET Framework. |
|
Probably described poorly. What I was looking for - is to try to see if module initializers could replace infrastructure that we are using from underlying CMS platform. They have initialization engine that is responsible for calling found init modules across all assemblies in app domain (done via scanning). Each init module could declare dependency - I can be called only after other init module has been called. That other module could be located in different assembly. You specify dependency via Was just wondering - whether this feature could be used instead. |
|
@RikkiGibson @jcouv The more I think about it, the less I like the idea of allowing ModuleInitializerAttribute to be emitted as a custom attribute. Leaving it on the method makes it look like it has meaning to the runtime. Is there any reason in favor of leaving it? It doesn't answer the question "is this MethodInfo called from the module initializer" because that depends on the compiler version that the method had been compiled with. |
|
This test plan was fully checked. I'll go ahead and close it. |
Championed issue: dotnet/csharplang#2608
Specification: https://github.com/dotnet/csharplang/blob/master/proposals/module-initializers.md
missing module init type:[ModuleInit(typeof(Missing))]module init type has a use-site error[ModuleInit(typeof(A))]whereAcan't be fully loaded (A defined in a library, but it's base type is referenced in another library which is not referenced in current/client library)No warning when module initializer method initializes a static field with a non-nullable reference typeseems to be a difficult analysis to reason about correctly. Test plan for "module initializers" feature #40500 (comment)static int field = default;when module initializer also writes tofieldBCL
LDM
FYI @jnm2
The text was updated successfully, but these errors were encountered: