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

Require definite assignment of all fields if struct includes any field initializers #57925

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

cston
Copy link
Member

@cston cston commented Nov 22, 2021

See proposals/csharp-10.0/parameterless-struct-constructors.md:

Instance fields (other than fixed fields) must be definitely assigned in struct instance constructors that do not have a this() initializer (see struct constructors).

Definite assignment of struct instance fields is required within synthesized and explicit parameterless constructors.

Fixes #57870

@@ -43,7 +43,7 @@ internal class FlowAnalysisPass
if (method.ReturnsVoid || method.IsIterator || method.IsAsyncEffectivelyReturningTask(compilation))
{
// we don't analyze synthesized void methods.
if ((method.IsImplicitlyDeclared && !method.IsScriptInitializer) ||
if ((method.IsImplicitlyDeclared && !method.IsScriptInitializer && (!method.IsParameterlessConstructor() || method.IsDefaultValueTypeConstructor(requireZeroInit: true))) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it a little brittle that we do this specifically for synthesized void methods. Basically, whenever a new synthesized void method is added to the compiler/language, we could forget to change this. Would it be possible to remove the condition, identify the problematic void methods that we don't want to analyze, and then introduce a new condition which excludes those methods more specifically? That way when a new kind of synthesized void method comes about it will just be analyzed by default and the author will have to make a special effort to opt out of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to keep this change simple if possible in case we want to port this change to other branches. I've logged #58012 to update the condition separately.

@cston
Copy link
Member Author

cston commented Nov 29, 2021

@dotnet/roslyn-compiler please review, thanks.

@cston
Copy link
Member Author

cston commented Nov 29, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@@ -43,7 +43,7 @@ internal class FlowAnalysisPass
if (method.ReturnsVoid || method.IsIterator || method.IsAsyncEffectivelyReturningTask(compilation))
{
// we don't analyze synthesized void methods.
if ((method.IsImplicitlyDeclared && !method.IsScriptInitializer) ||
if ((method.IsImplicitlyDeclared && !method.IsScriptInitializer && (!method.IsParameterlessConstructor() || method.IsDefaultValueTypeConstructor(requireZeroInit: true))) ||
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 29, 2021

Choose a reason for hiding this comment

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

(!method.IsParameterlessConstructor() || method.IsDefaultValueTypeConstructor(requireZeroInit: true))

It looks like this condition will be false for a synthesized parameter-less constructor of a class. Therefore, we are going to Analyze it, which we didn't do before. I assume that was not the intent of the change. #Closed

Copy link
Member Author

@cston cston Nov 29, 2021

Choose a reason for hiding this comment

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

Good catch, thanks. I've updated the if condition.

I'm open to suggestions on how to improve the readability of the if though. It feels like the call to IsDefaultValueTypeConstructor(bool requireZeroInit) is the complicated part. One possibility is to change that method to return a value that describes the constructor (such as the enum below), but that might mean performing extra work for callers that don't care about requireZeroInit.

enum ValueTypeConstructorKind
{
    None,
    Explicit,
    ImplicitZeroInit,
    ImplicitWithFieldInitializers,
}

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

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 2)

@cston cston merged commit 21055e1 into dotnet:main Nov 29, 2021
@cston cston deleted the 57870 branch November 29, 2021 22:11
@ghost ghost added this to the Next milestone Nov 29, 2021
@cston cston modified the milestones: Next, 17.1.P2 Nov 29, 2021
cston added a commit to cston/roslyn that referenced this pull request Jan 26, 2022
RikkiGibson added a commit that referenced this pull request Feb 16, 2022
…implicit parameterless constructor (#59055)

* Report error if 'record struct' constructor calls default parameterless constructor (#58339)

* Improve error reporting for 'this()' initializer from 'record struct' constructor

* Require definite assignment of all fields if struct includes any field initializers (#57925)
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.

Parameterless struct initialization re-uses values
3 participants