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 proposal for explicit parameterless struct constructors #4459

Merged
merged 8 commits into from Mar 2, 2021

Conversation

cston
Copy link
Member

@cston cston commented Feb 24, 2021

No description provided.

PrivateConstructor z = default; // ok
```

For a type parameter `T` with a `new()` constraint, `new T()` is emitted as a call to `System.Activator.CreateInstance<T>()` which ignores any explicit parameterless constructor.
Copy link
Contributor

@HaloFour HaloFour Feb 24, 2021

Choose a reason for hiding this comment

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

My understanding is that this bug in the CLR was fixed:

dotnet/coreclr#10778

/cc @jnm2 #Resolved

Copy link
Contributor

@jnm2 jnm2 Feb 24, 2021

Choose a reason for hiding this comment

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

Also found this from a recent LDM:

https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-01-27.md#field-initializers

We're still interested in doing this: the Activator.CreateInstance bug was in a version of the framework that is long out of support at this point, and we have universal agreement behind the idea. #Resolved

Copy link
Member

@jaredpar jaredpar Feb 24, 2021

Choose a reason for hiding this comment

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

Think we should have an open issue section at the bottom to follow up with CLR on this point. #Resolved

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Activator stuff needs to be corrected.

proposals/csharp-10.0/parameterless-struct-constructors.md Outdated Show resolved Hide resolved
proposals/csharp-10.0/parameterless-struct-constructors.md Outdated Show resolved Hide resolved
PrivateConstructor z = default; // ok
```

For a type parameter `T` with a `new()` constraint, `new T()` is emitted as a call to `System.Activator.CreateInstance<T>()` which ignores any explicit parameterless constructor.
Copy link
Member

@jaredpar jaredpar Feb 24, 2021

Choose a reason for hiding this comment

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

Think we should have an open issue section at the bottom to follow up with CLR on this point. #Resolved

### Instance field initializers
_[Adapted from [classes.md#instance-variable-initializers](https://github.com/dotnet/csharplang/blob/master/spec/classes.md#instance-variable-initializers).]_
When an instance constructor has no constructor initializer `this(...)`, that constructor implicitly performs the initializations specified in the _variable_initializers_ of the instance fields.
This corresponds to a sequence of assignments that are executed immediately upon entry to the constructor and before the implicit invocation of the direct base class.
Copy link
Member

@jaredpar jaredpar Feb 24, 2021

Choose a reason for hiding this comment

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

There is no base class in this scenario but we should specify the behavior when the parameterless ctor chains to another ctor. When do initializers execute in that scenario? #Resolved

@ufcpp
Copy link

ufcpp commented Feb 24, 2021

// `= new()` is equivalent to `= default` and emitted as `.param[1] = nullref`
void m(S x = new S()) { }
struct S
{
    // If `S` has a parameterless constructor, should `new S()` in optional parameters be error or warning?
}
``` #Resolved

@cston cston marked this pull request as ready for review February 25, 2021 18:42
@cston
Copy link
Member Author

cston commented Feb 25, 2021

void m(S x = new S()) { }

Good catch, thanks @ufcpp.


In reply to: 784731182 [](ancestors = 784731182)

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Initial spec with open questions LGTM

@cston cston merged commit 14c7c29 into dotnet:master Mar 2, 2021
@cston cston deleted the struct-ctor branch March 5, 2021 20:08

There is a gap in type parameter constraint checking because the `new()` constraint is satisfied by a type parameter with a `struct` constraint (see [satisfying constraints](https://github.com/dotnet/csharplang/blob/master/spec/types.md#satisfying-constraints)).

As a result, the following will be allowed by the compiler but `Activator.CreateInstance<InternalConstructor>()` will fail at runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being late to the party. Can we avoid this problem by requiring public accessibility on the parameterless constructor?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately not really. The ecosystem today already contains struct with non-public ctors. That has to be accounted for in the design here. It would be possible for us to say we don't support them at a syntax level but we need to account for what happens when we see them in metadata.

Given we have to account for them in metadata I would allow them in source otherwise it creates an odd friction point.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it makes sense that we still have to handle types in the wild. But are we helping users by making it easy to step into this gap?
If possible, let's bring this as an open question at LDM tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants