Skip to content

Do not synthesize a parameterless constructor for struct types; report error if struct has field initializers but no constructors#58581

Merged
cston merged 6 commits intodotnet:release/dev17.1from
cston:no-synthesized-ctor
Jan 6, 2022
Merged

Do not synthesize a parameterless constructor for struct types; report error if struct has field initializers but no constructors#58581
cston merged 6 commits intodotnet:release/dev17.1from
cston:no-synthesized-ctor

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Jan 3, 2022

@ghost ghost added the Area-Compilers label Jan 3, 2022
…t error if struct has field initializers but no constructors
@cston cston force-pushed the no-synthesized-ctor branch from 3d1c93d to ed9f37c Compare January 3, 2022 23:40
@cston cston marked this pull request as ready for review January 3, 2022 23:45
@cston cston requested a review from a team as a code owner January 3, 2022 23:45
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

@cston
Copy link
Copy Markdown
Contributor Author

cston commented Jan 4, 2022

@dotnet/roslyn-compiler, please review, thanks.


// no need to emit the default ctor, we are not emitting those
if (methodSymbol.IsDefaultValueTypeConstructor(requireZeroInit: true))
if (methodSymbol.IsDefaultValueTypeConstructor(requireZeroInit: false))
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jan 4, 2022

Choose a reason for hiding this comment

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

IsDefaultValueTypeConstructor

Since we no longer synthesize parameter-less struct constructors, it feels like the implementation of this method should change, Perhaps we even can drop the requireZeroInit parameter altogether. #Closed


// no need to emit the default ctor, we are not emitting those
if (methodSymbol.IsDefaultValueTypeConstructor(requireZeroInit: true))
if (methodSymbol.IsDefaultValueTypeConstructor(requireZeroInit: false))
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jan 4, 2022

Choose a reason for hiding this comment

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

requireZeroInit: false

The logic here looks confusing. Why is it the right thing to ignore a non-zeroinit parameter-less constructor? Presumably the constructor does something useful beyond just zero initializing the instance. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The logic here looks confusing.

I agree, it is confusing. We're ignoring implicitly-declared parameterless constructors only though, not explicitly-declared constructors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're ignoring implicitly-declared parameterless constructors only though, not explicitly-declared constructors.

In that case the parameter must be true.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like either the parameter is no longer meaningful, or the logic here is incorrect.

// If there are field initializers and an explicit constructor with parameters
// (that is, more than one constructor), the implicit parameterless constructor
// is treated as the zero-init constructor and does not execute field initializers.
return constructors.Length > 1 || !containingType.HasFieldInitializers();
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jan 4, 2022

Choose a reason for hiding this comment

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

HasFieldInitializers

It looks like this API is not used for anything else and we can remove it. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks.

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3), assuming CI is passing.

@cston cston requested a review from a team January 5, 2022 00:37
@cston
Copy link
Copy Markdown
Contributor Author

cston commented Jan 5, 2022

@dotnet/roslyn-compiler, please review, thanks.

@cston cston changed the base branch from main to release/dev17.1 January 5, 2022 22:20
@cston cston changed the base branch from release/dev17.1 to main January 5, 2022 22:21
@cston cston force-pushed the no-synthesized-ctor branch from 3611015 to 7782494 Compare January 5, 2022 22:33
@cston cston changed the base branch from main to release/dev17.1 January 5, 2022 22:33
@cston cston force-pushed the no-synthesized-ctor branch from fd7ea66 to 3363d91 Compare January 5, 2022 22:59
{
public struct IsReadOnlyAttribute
{
private int F = 1; // requires synthesized parameterless .ctor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

synthesized

This comment seems incorrect now.

Copy link
Copy Markdown
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.

LGTM (commit 6), other than the comment that should be updated.

@cston cston added this to the 17.1.P3 milestone Feb 21, 2022
Viir added a commit to Viir/bots that referenced this pull request Aug 31, 2022
Update volatile process program code to avoid CS8983 (A 'struct' with field initializers must include an explicitly declared constructor)

After the changes in C# discussed at dotnet/csharplang#5552
For the upstream changes, see dotnet/roslyn#58581

For discussion of the change, also see dotnet/sdk#23971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants