Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ECMA augments regarding instance and type construction. #110343

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MichalPetryka
Copy link
Contributor

@MichalPetryka MichalPetryka commented Dec 3, 2024

Multiple initializations of instances and types might violate runtime invariants, we should forbid them then as discussed in #109679.

Users are not expected to have been relying on the behaviour being legal, especially since multiple type initializations are already resulting in invalid behaviour due to JIT optimizations.

cc @jkotas @tannergooding @EgorBo as discussed.

Multiple initializations of instances and types might violate runtime invariants, we should forbid it then as discussed in dotnet#109679.

Users are not expected to have been relying on the behaviour being legal, especially since multiple type initializations are already resulting in invalid behaviour due to JIT optimizations.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 3, 2024
Copy link
Contributor

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

@hamarb123
Copy link
Contributor

hamarb123 commented Dec 3, 2024

Is there any reason to go this far? Wouldn't it be more sensible to say that it's just unsupported for all the types in the BCL & special types like delegates, enums (not that these can even have ctors), etc. (+ probably also anything else that doesn't explicitly document it as fine, but this library documentation part about other libs in this bracket doesn't seem like a concern for ECMA-335, other than at most as a comment to point out what should be common sense) - but that doesn't mean it should necessarily be invalid in every single case as a blanket rule (not that I wouldn't find such code not suspiscious) (like how we might say it's unsupported to take a ref readonly and write to it using Unsafe in general, but that doesn't mean it's actually wrong in every single scenario, just highly suspiscious that known mutable memory is being passed as readonly and then treated as mutable again later). An example usage where it might not be wrong is calling instance constructor on an object from GetUninitializedObject (assuming the constructor actually initialises all fields obviously, since idk if we guarantee that it's already zeroed).

@jkotas
Copy link
Member

jkotas commented Dec 3, 2024

Is there any reason to go this far? Wouldn't it be more sensible to say that it's just unsupported

ECMA spec is concerned about verifiable code only. It touches on valid unverifiable code here and there, but it is very far from having a full precise definition of valid unverifiable code. In general, it leaves the behavior of valid unverifiable code as undefined.

I see things like calling constructor multiple times as a potentially valid unverifiable code.


The following is added to the section "II.10.5.3 Type initializer":

> Type initializers shall not be called explicitly from user code. Users intending to guarantee the type initializer has been executed shall use the `System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor` method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type initializers shall not be called explicitly from user code

This contradicts "A type initializer shall be executed exactly once for any given type, unless explicitly
called by user code." in the next chapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is changed with the next paragraph.


Section "II.10.5.3.1 Type initialization guarantees" is changed so that the guarantee number 3 now states the following:

> A type initializer shall be executed exactly once for any given type, unless the previous attempt resulted in a `System.TypeInitializationException` being thrown at the location that triggered it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless the previous attempt resulted in a System.TypeInitializationException being thrown at the location that triggered it.

I am not sure what this is trying to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a .cctor throws an exception, the type is not marked as initialized so the .cctor will be called again by the runtime until it succeeds.

Copy link
Member

@jkotas jkotas Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not correct. If the .cctor throws exception, the runtime won't try to run the .cctor again. The failure is cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know that, I feel like older versions of legacy Mono might've been non compliant with this cause I recall it being rerun in Unity.


The following is added to the section "II.10.5.1 Instance constructor":

> Instance constructors shall not be executed multiple times for a single object instance. Explicit calls to constructors on object instances from user code are only permitted when calling instance constructors of the base type inside of instance constructors of the derived type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. Try this:

class A
{
    A() : this(1)
    {
    }

    A(int x)
    {
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you may want to update the wording in the call instruction verification rules that have the same mistake: "The call instruction can also be used to call an object’s base class constructor, or to initialize a value type location by calling an appropriate constructor, both of which are treated as special cases by verification.".

It may be sufficient to just fix the call verification rules to be more precise instead of duplicating it here.

@hamarb123
Copy link
Contributor

I see things like calling constructor multiple times as a potentially valid unverifiable code.

If this is the direction we go, I'd prefer if we explicitly mention something along the lines of the above somewhere at least. I think it would be good to better document stuff around valid unverifiable code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants