-
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
Handle captured primary constructor parameters in readonly context. #66685
Handle captured primary constructor parameters in readonly context. #66685
Conversation
@@ -81,9 +81,7 @@ private CapturedParametersFinder(SynthesizedPrimaryConstructor primaryConstructo | |||
// We should define GeneratedNameKind entry (e..g P looks free to use) and add a helper to GeneratedNames that produces the name. | |||
// I'd also just keep the name as short as possible to avoid unnecessary metadata bloat. Could be just <name>P. | |||
string name = "<" + parameter.Name + ">PC__BackingField"; | |||
|
|||
// PROTOTYPE(PrimaryConstructors): Ever read-only? |
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.
Would it be possible to also make the field read-only if it's never mutated?
Theoretically, yes. But we don't mark fields readonly based on how they are used.
@cston, @dotnet/roslyn-compiler For the second review |
1 similar comment
@cston, @dotnet/roslyn-compiler For the second review |
@@ -1041,37 +1123,11 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess, | |||
// S has a mutable field x, then c.f.x is not a variable because c.f is not | |||
// writable. |
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 removing this comment since the logic (and the comment) have moved to CanModifyReadonlyField()
.
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 removing this comment since the logic (and the comment) have moved to
CanModifyReadonlyField()
.
I intentionally left this comment here as well. For the benefit of the reader.
// x.F = ref y; | ||
Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "x.F").WithArguments("x.F").WithLocation(6, 9) | ||
); | ||
} |
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.
Do we allow capture of
ref struct
parameters inref struct
primary constructors?
No. We disallow capturing of in
, ref
and out
parameters regardless of context. Lambdas don't allow that too. Here is a quote from the speclet: "Capturing is not allowed for ref, in and out parameters. This is similar to a limitation for capturing in lambdas."
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.
Is capturing allowed for ref struct
parameters passed by value though? If not, the spec and the error message should mention that explicitly.
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.
Is capturing allowed for ref struct parameters passed by value though? If not, the spec and the error message should mention that explicitly.
Similar to lambdas, ref structs cannot be captured when they are used as primary constructor parameters. This is covered by IllegalCapturingDueToRefness_01
and there is a PROTOTYPE comment there to adjust the wording for primary constructors. I will mention that fact in the speclet as well.
dotnet/csharplang#6938