Skip to content

Use LHS of ref-assignment for ref-readonly#24904

Merged
agocke merged 1 commit intodotnet:features/ref-reassignmentfrom
agocke:ref-readonly-assignment
Feb 27, 2018
Merged

Use LHS of ref-assignment for ref-readonly#24904
agocke merged 1 commit intodotnet:features/ref-reassignmentfrom
agocke:ref-readonly-assignment

Conversation

@agocke
Copy link
Member

@agocke agocke commented Feb 16, 2018

The spec for ref-reassignment is that the LHS of the ref-reassignment
expression is the ref-readonlyness of the LValue produced by the
expression. That is, if the LHS of a ref assignment is readonly, the
resulting LValue is as well.

This PR also fixes a related issue for ref-like assignments, where the
resulting lifetime of the assignment was decided based on the RHS,
instead of the LHS.

Fixes #24874

@agocke agocke requested review from a team and VSadov February 16, 2018 23:31
Copy link
Member

Choose a reason for hiding this comment

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

I would just call CheckValueKind on the left recursively.

There are other value kinds such as "ref-returnable". Lets just defer all checks to the LHS. - for the same reason we would want to defer writeability.

Will need a test for "ref-returnable", I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

But there's no recursion here, is there? A ref assignment must have a variable on the left hand, so this can't go any deeper than one level. This also may make error production worse because CheckValueKind would produce an error on the LHS of the assignment, even though there's nothing wrong with the LHS here, only the expression as a whole.

Copy link
Member Author

@agocke agocke Feb 17, 2018

Choose a reason for hiding this comment

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

I guess we are duplicating code here by checking the readonlyness in two places. I'll call CheckValueKind.

@agocke agocke force-pushed the ref-readonly-assignment branch from 8f087d1 to 25a5185 Compare February 20, 2018 20:44
Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@VSadov
Copy link
Member

VSadov commented Feb 21, 2018

@dotnet-bot test windows_release_unit64_prtest please

@agocke
Copy link
Member Author

agocke commented Feb 21, 2018

ping @dotnet/roslyn-compiler for one more review

@agocke
Copy link
Member Author

agocke commented Feb 22, 2018

ping @dotnet/roslyn-compiler

1 similar comment
@agocke
Copy link
Member Author

agocke commented Feb 26, 2018

ping @dotnet/roslyn-compiler

Copy link
Member

Choose a reason for hiding this comment

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

return [](start = 20, length = 6)

Nit: should have empty line after }

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:

@agocke agocke force-pushed the ref-readonly-assignment branch from 64f67df to 6663476 Compare February 27, 2018 20:51
The spec for ref-reassignment is that the LHS of the ref-reassignment
expression is the ref-readonlyness of the LValue produced by the
expression. That is, if the LHS of a ref assignment is readonly, the
resulting LValue is as well.

This PR also fixes a related issue for ref-like assignments, where the
resulting lifetime of the assignment was decided based on the RHS,
instead of the LHS.

Fixes dotnet#24874
@agocke agocke force-pushed the ref-readonly-assignment branch from 6663476 to 5c47a8c Compare February 27, 2018 21:11
@agocke agocke merged commit 6156582 into dotnet:features/ref-reassignment Feb 27, 2018
@agocke agocke deleted the ref-readonly-assignment branch February 27, 2018 22:22
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.

3 participants