-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Support ref field assignment in object initializers #62584
Conversation
Should be #62120? In reply to: 1182886657 |
fac37f2
to
e657188
Compare
{ | ||
// D = { ..., <identifier> = <expr>, ... }, where D : dynamic | ||
var memberName = ((IdentifierNameSyntax)leftSyntax).Identifier.Text; | ||
boundLeft = new BoundDynamicObjectInitializerMember(leftSyntax, memberName, implicitReceiver.Type, initializerType, hasErrors: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 The binding of dynamic LHS was moved into BindObjectInitializerMember
which handled other bindings of LHS. #Resolved
isObjectInitializer = true; | ||
return this.ParseObjectInitializerNamedAssignment(); | ||
} | ||
else | ||
{ | ||
return this.ParseExpressionCore(); | ||
// <expr> | ||
// ref <expr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 This case and the dictionary initializer case above remain errors in binding, but the parser now tolerates ref
(more graceful failure). #Resolved
@cston @RikkiGibson for review. Thanks |
@@ -4866,10 +4877,10 @@ private BoundExpression MakeBadExpressionForObjectCreation(SyntaxNode node, Type | |||
|
|||
case BoundKind.ArrayAccess: | |||
case BoundKind.PointerElementAccess: | |||
return boundMember; | |||
return CheckValue(boundMember, valueKind, diagnostics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We were returning so bypassing the CheckValueKind
check below.
Without this fix, we'd have no error in RefInitializer_OnArray
or RefInitializer_OnPointer
} | ||
if (expr is BoundArrayAccess) | ||
{ | ||
return "array access"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
"; | ||
var comp = CreateCompilation(source, parseOptions: TestOptions.Regular11); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"; | ||
var comp = CreateCompilation(source); | ||
comp.VerifyDiagnostics(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added RefInitializer_AssignInParameter
{ | ||
public dynamic D; | ||
} | ||
ref struct R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
var source = @" | ||
int x = 42; | ||
var r = new dynamic { field = ref x }; // 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps:
int i = 42;
var r = new R<dynamic> { F = ref i };
ref struct R<T>
{
public ref T F;
}
``` #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added RefInitializer_DynamicField
I think this In reply to: 1189357786 In reply to: 1189357786 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:152 in 6980e20. [](commit_id = 6980e20, deletion_comment = True) |
Yes, I've reverted that line just after pushing. It was over-eager search & replace In reply to: 1189360227 |
// var r = new R() { field = ref x }; // 1 | ||
Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion10, "field").WithArguments("ref fields", "11.0").WithLocation(3, 19), | ||
// (6,15): error CS8936: Feature 'ref fields' is not available in C# 10.0. Please use language version 11.0 or greater. | ||
// _ = r2 with { field = ref x }; // 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we separately have a test which verifies the use site diagnostic when assigning by value here? #Resolved
[Fact] | ||
public void RefInitializer_RHSTypeMustMatch() | ||
{ | ||
// The right operand must be an expression that yields an lvalue designating a value of the same type as the left operand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider testing a case where there is an implicit conversion from the RHS type to the LHS type. #Resolved
// var r = new C { a = ref x }; // 1 | ||
Diagnostic(ErrorCode.ERR_BadEventUsage, "a").WithArguments("C.a", "C").WithLocation(3, 17), | ||
// (6,3): error CS0070: The event 'C.a' can only appear on the left hand side of += or -= (except when used from within the type 'C') | ||
// c.a = ref x; // 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to verify the behavior here on e.g. void M() { this.a = ref x; }
or void M() { this.a += ref x; }
. #Resolved
comp.VerifyDiagnostics( | ||
// (3,34): error CS8173: The expression must be of type 'dynamic' because it is being assigned by reference | ||
// var r = new R<dynamic> { F = ref i }; // 1 | ||
Diagnostic(ErrorCode.ERR_RefAssignmentMustHaveIdentityConversion, "i").WithArguments("dynamic").WithLocation(3, 34) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider including a success case here (e.g. dynamic i = 42
). #Resolved
Diagnostic(ErrorCode.ERR_RefReturnLocal, "field = ref x").WithArguments("x").WithLocation(36, 21) | ||
); | ||
|
||
source = @" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider makign this a separate test and/or including a comment which indicates what the difference is between the two scenarios. #Resolved
Container r = default; | ||
{ | ||
int x = 42; | ||
var r = new Container { item = { field = ref x } }; // 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider including some success cases. #Resolved
|
||
[Theory] | ||
[InlineData(LanguageVersion.CSharp10)] | ||
[InlineData(LanguageVersionFacts.CSharpNext)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I expected use of LanguageVersion.CSharp11
here instead. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 6)
} | ||
"; | ||
var comp = CreateCompilation(source); | ||
comp.VerifyDiagnostics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the generated code run successfully? Consider running the code. #Resolved
* upstream/main: (53 commits) Remove 'mangleName' parameter in PENamedTypeSymbolNonGeneric (dotnet#62813) Produce errors for 'partial file record' (dotnet#62686) [EnC] Allow renaming methods, properties, events etc. (dotnet#62364) Update AbstractFileHeaderDiagnosticAnalyzer.cs Rename file Make static Move tests Refactor Support ref field assignment in object initializers (dotnet#62584) Fix cref tags on generated VB's SyntaxFactory (dotnet#62578) Simplify Simplify Simplify Simplify Simplify Simplify Simplify Simplify Move helper methods Remove serialization ...
Fixes #62120
Corresponding spec update: dotnet/csharplang#6277
Relates to test plan #59194