Parameterless struct constructors: Remaining work items #54811
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcouv
force-pushed
the
parameterless-ctors
branch
from
a3b4f10 to
5fc555d
Compare
cston
reviewed
Jul 19, 2021
| } | ||
| } | ||
|
|
||
| bool skipInitializer = false; |
cston
reviewed
Jul 19, 2021
| constructor.Initializer == null ? null : bodyBinder.BindConstructorInitializer(constructor.Initializer, diagnostics), | ||
| skipInitializer ? new BoundNoOpStatement(constructor, NoOpStatementFlavor.Default) | ||
| : initializer == null ? null | ||
| : bodyBinder.BindConstructorInitializer(initializer, diagnostics), |
There was a problem hiding this comment.
I think that's how we've indented such nested conditionals before.
There was a problem hiding this comment.
If it is, then when I next stumble onto those cases I'll reformat for readability. The way this is formatted is very confusing.
cston
reviewed
Jul 19, 2021
| constructor.Body == null ? null : (BoundBlock)bodyBinder.BindStatement(constructor.Body, diagnostics), | ||
| constructor.ExpressionBody == null ? | ||
| null : | ||
| bodyBinder.BindExpressionBodyAsBlock(constructor.ExpressionBody, | ||
| constructor.Body == null ? diagnostics : BindingDiagnosticBag.Discarded)); | ||
| } | ||
|
|
||
| internal static bool IsDefaultValueTypeConstructor(NamedTypeSymbol type, ConstructorInitializerSyntax initializerSyntax) |
cston
reviewed
Jul 19, 2021
| var comp = CreateCompilation(src); | ||
| comp.VerifyDiagnostics(); | ||
| CompileAndVerify(comp, expectedOutput: "(hello, hello, True, hello)"); | ||
| } |
There was a problem hiding this comment.
Are we testing calls to the following constructors?
record struct S4
{
public string field = "hello";
public S4(object o) : this() { }
}
record struct S5()
{
public string field = "hello";
public S5(object o) : this() { }
}
record struct S6(string other)
{
public string field = "hello";
public S6() : this(null) { }
}
``` #Closed
|
@333fred @dotnet/roslyn-compiler for second review. Thanks |
cston
approved these changes
Jul 20, 2021
333fred
reviewed
Jul 20, 2021
| comp.VerifyDiagnostics( | ||
| // (13,5): error CS8862: A constructor declared in a record with parameter list must have 'this' constructor initializer. | ||
| // S3(object o) { } // 1 | ||
| Diagnostic(ErrorCode.ERR_UnexpectedOrMissingConstructorInitializerInRecord, "S3").WithLocation(13, 5), |
There was a problem hiding this comment.
Can we improve this error message? I don't understand what it's telling me. #WontFix
There was a problem hiding this comment.
I suppose this is probably out of scope for this PR, but I really do find this confusing.
There was a problem hiding this comment.
As I mentioned offline, this error message correctly describes the problem. But we can certainly improve the wording separately from the parameterless struct ctor feature work if you have a suggestion.
333fred
approved these changes
Jul 20, 2021
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Jul 23, 2021
* upstream/main: (249 commits) Switch back queue name to default (dotnet#55064) Fix 'code model' with file scoped namespaces Map documents to be reanalyzed back from compile-time to design-time documents (dotnet#55054) Update MSBuild Workspace test projects target framework Enable CA1069 for ErrorCode and MessageID (dotnet#54695) Dev16->17 updates Update global.json Record completion of "parameterless struct constructor" feature (dotnet#55049) Generalize rude edit messages to be applicable to both Hot Reload and EnC (dotnet#55012) Update azure-pipelines-official.yml Update azure-pipelines-integration.yml Merge pull request dotnet#54992 from jaredpar/so-big Parameterless struct constructors: Remaining work items (dotnet#54811) Update docs/wiki/Diagnosing-Project-System-Build-Errors.md update queue name Dev16->17 changes Fix test Fix 'move type' with file scoped namespaces Fix 'match folder and namespace' with file scoped namespaces Log NFW ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is addressing remaining work items in test plan: #51698
See spec for expected execution of field initializers and
: this()representing a default value type constructor:https://github.com/dotnet/csharplang/blob/main/proposals/parameterless-struct-constructors.md#executing-field-initializers