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

Adjust assert to account for error scenarios #66202

Merged

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jan 3, 2023

Related to #66046

The effect of a constructor initializer is to prepend a statement to the effect of

S..ctor()
{
    S..otherCtor(out this, ...);
    // ... original constructor body statements
}

The point of this assert is that if a constructor initializer is present, it doesn't make sense for implicitly initialized fields to also be present, since you cannot access this until after the constructor initializer runs, which fully assigns this.

In error scenarios we can still end up referencing instance fields before the constructor initializer, so we relax the assert to allow the "unexpected" implicit initializations in this case.

Auto-default logic runs simply as a part of definite assignment analysis, so I think there is not a ton to be gained by skipping it when a constructor initializer is present. We could consider avoiding adding to the implicitlyInitializedFields when we notice a constructor initializer is present, but IMO this isn't necessary, and it's a little simpler to just adjust this assert to reflect our expectations. I'm open to discussion on this point, though.

In the future, if this assert fails, it means we are using an unassigned field before the constructor initializer has run in code without errors.

@AlekseyTs @dotnet/roslyn-compiler for review.

@RikkiGibson RikkiGibson requested a review from a team as a code owner January 3, 2023 20:17
@@ -4091,6 +4091,48 @@ .maxstack 2
");
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider linking to issue

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv jcouv self-assigned this Jan 3, 2023
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 4, 2023

@RikkiGibson As I mentioned in #66046: "Please keep the issue active until that assert is re-enabled." That is in the feature branch.

@RikkiGibson RikkiGibson requested a review from AlekseyTs January 9, 2023 20:10
@RikkiGibson
Copy link
Contributor Author

@AlekseyTs Let me know if the adjusted assert seems reasonable to you. Thanks

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)

@AlekseyTs
Copy link
Contributor

@RikkiGibson Please assign the issue to me once you merge

@RikkiGibson RikkiGibson merged commit 13fc136 into dotnet:main Jan 10, 2023
@RikkiGibson RikkiGibson deleted the auto-default-ctor-initializer-error branch January 10, 2023 22:22
@ghost ghost added this to the Next milestone Jan 10, 2023
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023
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.

5 participants