Flow analysis of with-expressions#44644
Flow analysis of with-expressions#44644RikkiGibson merged 23 commits intodotnet:features/recordsfrom
Conversation
Changes records to generate init-only properties instead of get-only, and modifies the `with` expression to treat arguments as assignments to the left-hand side, with the safety rules of object initializers (meaning init-only assignments are allowed). Also changes records to generate a Clone method and a copy constructor. The clone calls the copy constructor and the copy constructor does a memberwise copy of the instance fields of the other type. This change also tries to simplify and reuse more pieces of binding and lowering from other initializer-based forms. The with-expression is now bound as though it were an assignment to a field or member access on a call to the Clone method.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 5)
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
Outdated
Show resolved
Hide resolved
…hesizedRecordClone.cs Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
…-expr-flow-analysis
… with-expr-flow-analysis
…-expr-flow-analysis
|
After dotnet/csharplang#3527 has been merged, please work with @agocke to draft spec language for definite assignment of with expressions to add to the records spec. |
| B b = new B(""hello"", null); | ||
| if (flag) | ||
| { | ||
| b.X.ToString(); // PROTOTYPE(records) |
There was a problem hiding this comment.
// PROTOTYPE(records) [](start = 28, length = 21)
Please replace PROTOTYPE with link to issue.
Note, I agree this should work as we map constructor parameters to properties (just like we do for tuples or Nullable<T>).
| // due to limitations of object initializer analysis. | ||
| // Tracking in https://github.com/dotnet/roslyn/issues/44759 | ||
| comp.VerifyDiagnostics(); | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
The following nullability scenarios should already work because of the sharing of logic, but may be worth capturing:
x with { P1 = (other = null), P2 = other.M() } // possible dereferenceb with { X = M(out s), Y = s }
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 20)
|
|
||
| data class B(string? X) | ||
| { | ||
| public B? Clone() => new B(X); |
There was a problem hiding this comment.
It sounds like some upcoming spec changes are probably going to make this necessary to define in metadata if we want to test this scenario.
agocke
left a comment
There was a problem hiding this comment.
LGTM, although you may want to rebase this onto the feature branch before merging
|
I think I will squash this PR when merging. @agocke |
Fixes #44672
Related to #40726
/cc @agocke @dotnet/roslyn-compiler