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

Flow analysis of with-expressions #44644

Merged

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented May 28, 2020

Fixes #44672
Related to #40726

/cc @agocke @dotnet/roslyn-compiler

agocke and others added 6 commits May 26, 2020 19:53
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.
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.

Done with review pass (iteration 5)

agocke and others added 2 commits May 28, 2020 23:49
…hesizedRecordClone.cs

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
@gafter gafter self-requested a review May 30, 2020 01:12
@gafter
Copy link
Member

gafter commented Jun 1, 2020

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.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

B b = new B(""hello"", null);
if (flag)
{
b.X.ToString(); // PROTOTYPE(records)
Copy link
Member

@jcouv jcouv Jun 1, 2020

Choose a reason for hiding this comment

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

// 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>).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// due to limitations of object initializer analysis.
// Tracking in https://github.com/dotnet/roslyn/issues/44759
comp.VerifyDiagnostics();
}
Copy link
Member

Choose a reason for hiding this comment

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

} [](start = 8, length = 1)

The following nullability scenarios should already work because of the sharing of logic, but may be worth capturing:

  1. x with { P1 = (other = null), P2 = other.M() } // possible dereference
  2. b with { X = M(out s), Y = s }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

Done with review pass (iteration 20)


data class B(string? X)
{
public B? Clone() => new B(X);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM, although you may want to rebase this onto the feature branch before merging

@RikkiGibson
Copy link
Contributor Author

I think I will squash this PR when merging. @agocke

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 23)

@RikkiGibson RikkiGibson merged commit 0ae1008 into dotnet:features/records Jun 2, 2020
@RikkiGibson RikkiGibson deleted the with-expr-flow-analysis branch June 2, 2020 16:36
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.

6 participants