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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jan 6, 2022
Merged

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

merged 6 commits into from
Jan 6, 2022

Conversation

cston
Copy link
Member

@cston cston commented Jan 3, 2022

@cston
Do not synthesize a parameterless constructor for struct types; repor…
ed9f37c 
…t error if struct has field initializers but no constructors
@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
Youssef1313
Youssef1313 suggested changes Jan 4, 2022
View reviewed changes
Copy link
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.

Should this be documented as a breaking change in https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20DotNet%207.md?

@cston
Copy link
Member Author

cston commented Jan 4, 2022

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

@Youssef1313 Youssef1313 mentioned this pull request Jan 4, 2022
Closed
@runfoapp runfoapp bot mentioned this pull request Jan 4, 2022
Closed
AlekseyTs
AlekseyTs reviewed Jan 4, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs Outdated
@@ -971,7 +971,7 @@ public override object VisitField(FieldSymbol symbol, TypeCompilationState argum
}

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

@AlekseyTs AlekseyTs Jan 4, 2022 •
edited
Loading

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

AlekseyTs
AlekseyTs reviewed Jan 4, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs Outdated
@@ -971,7 +971,7 @@ public override object VisitField(FieldSymbol symbol, TypeCompilationState argum
}

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

@AlekseyTs AlekseyTs Jan 4, 2022 •
edited
Loading

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

AlekseyTs
AlekseyTs reviewed Jan 4, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/MemberSymbolExtensions.cs
// 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
Contributor

@AlekseyTs AlekseyTs Jan 4, 2022 •
edited
Loading

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
Member Author

Choose a reason for hiding this comment

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

Removed, thanks.

AlekseyTs
AlekseyTs approved these changes Jan 4, 2022
View reviewed changes
Copy link
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
Member 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 changed the base branch from main to release/dev17.1 January 5, 2022 22:33
333fred
333fred reviewed Jan 6, 2022
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs
[Fact]
public void WellKnownTypeAsStruct_DefaultConstructor_IsReadOnlyAttribute_02()
{
var sourceAttribute =
@"#pragma warning disable 414
namespace System.Runtime.CompilerServices
{
public struct IsReadOnlyAttribute
{
private int F = 1; // requires synthesized parameterless .ctor
Copy link
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.

333fred
333fred approved these changes Jan 6, 2022
View reviewed changes
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.

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

@cston cston merged commit 3e4cc4c into dotnet:release/dev17.1 Jan 6, 2022
@cston cston deleted the no-synthesized-ctor branch January 7, 2022 04:38
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Feb 16, 2022
Closed
@cston cston added this to the 17.1.P3 milestone Feb 21, 2022
@vtcifer vtcifer mentioned this pull request Mar 29, 2022
Closed
Viir added a commit to Viir/bots that referenced this pull request Aug 31, 2022
@Viir
Adapt volatile process C# scripts to avoid error CS8983
b49110f 
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
Reviewers

@Youssef1313 Youssef1313 Youssef1313 requested changes

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
17.1.P3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cston @333fred @AlekseyTs @Youssef1313