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

Member access expressions #989

Merged
merged 39 commits into from
Mar 2, 2022
Merged

Member access expressions #989

merged 39 commits into from
Mar 2, 2022

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Dec 15, 2021

Support for member access expressions with syntax container.member, covering cases such as:

  • object.field
  • object.method(args)
  • package.namespace.class.member
  • object.(interface.member)
  • object.(class.member)

... and so on. Includes the rule for template name lookup as decided in #949.

@zygoloid zygoloid added the proposal A proposal label Dec 15, 2021
@zygoloid zygoloid added this to Draft in Proposals via automation Dec 15, 2021
@zygoloid zygoloid requested a review from a team December 15, 2021 03:10
@zygoloid zygoloid marked this pull request as ready for review January 19, 2022 23:51
@zygoloid zygoloid requested a review from a team as a code owner January 19, 2022 23:51
docs/design/expressions/README.md Outdated Show resolved Hide resolved
docs/design/expressions/README.md Outdated Show resolved Hide resolved
proposals/p0989.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/README.md Show resolved Hide resolved
docs/design/expressions/README.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
proposals/p0989.md Outdated Show resolved Hide resolved
proposals/p0989.md Outdated Show resolved Hide resolved
proposals/p0989.md Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
proposals/p0989.md Outdated Show resolved Hide resolved
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Largely LGTM. Some minor comments inline that just are about improving things, not really any changes.

Only thing seems like it'll have wider effect is being intentional around the naming of constructs here, especially "name" vs. "word". Feel free to submit if you see a clear resolution to everything here. But also fele free to poke for a fresh look if anything ends up changing enough to warrant it.

Comment on lines +95 to +97
Member access expressions associate left-to-right. If the member name is more
complex than a single _word_, an indirect member access expression can be used,
with parentheses around the member name:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
Member access expressions associate left-to-right. If the member name is more
complex than a single _word_, an indirect member access expression can be used,
with parentheses around the member name:
Member access expressions associate left-to-right. If the member name is more
complex than a single _word_, a member access _expression_ can be used,
with parentheses around the member name:

The word "indirect" for me adds more confusion than help, but the fact that it becomes an expression helps me. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see you really use these terms more heavily in the detailed design. I'll follow up there. This may not make sense to deal with until that is sorted.

Comment on lines +47 to +49
or an _indirect_ member access of the form:

- _member-access-expression_ ::= _expression_ `.` `(` _expression_ `)`
Copy link
Contributor

Choose a reason for hiding this comment

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

The term "indirect" here, and the distinction between "direct" and "indirect" doesn't really feel intuitive to me and for me gets confused with other uses of these terms. Any other ideas, maybe ones that more naturally reference the use of parentheses?

My ideas are to talk about "simple" vs "compound". I also might make it clear that this is about the specific member, so something like:

  • member-access-expression ::= _expression . member-name-expression

And then member name expresisons come in two forms. A simple and compound ones requiring paretheses:

  • member-name-expression ::= (word | member-access-expression ))

But wide open to other ideas here, and if nothing else works this is fine. Just not my favorite. =]

docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
docs/design/expressions/member_access.md Show resolved Hide resolved
docs/design/expressions/member_access.md Outdated Show resolved Hide resolved
@chandlerc chandlerc moved this from Draft to RFC in Proposals Mar 2, 2022
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Mar 2, 2022
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LGTM, ship it.

We can tweak the nomenclature trivially afterward in this case -- I don't think it's reflective of a substantive misunderstanding in the design or intent in any way here, its just about finding the most clear way to describe it so that should be fine for a code review to fix forward. (If any real controversy comes up, can always make a leads question, but I don't think this is that big of a deal even with the current nomenclature.)

Ship it!

@chandlerc chandlerc moved this from RFC to Accepted in Proposals Mar 2, 2022
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Mar 2, 2022
@zygoloid zygoloid merged commit c3b8a01 into carbon-language:trunk Mar 2, 2022
@zygoloid zygoloid mentioned this pull request Mar 9, 2022
@zygoloid zygoloid deleted the proposal-member-access-expres branch March 11, 2022 01:04
zygoloid added a commit that referenced this pull request May 13, 2022
Implement initial support for `A.(B)` syntax, per #989. Specifically, this supports:

* `object.(Type.member)` for instance members,
* `object.(Interface.member)` for instance and non-instance members,
* `object.(Type.(Interface.member))` for instance members,
* `Type.(Interface.member)` for non-instance members.

Three new AST nodes are introduced:

* `CompoundFieldAccessExpression` represents the `A.(B)` syntax.
* `MemberName` is a `Value` that represents the result of evaluating an expression such as `Type.member` or `Interface.member` or `Type.(Interface.member)`.
* `TypeOfMemberName` is the type of a `MemberName` value.

In order to handle members of classes and interfaces which have corresponding declarations and may need substitution into their types, and members of structs which don't have declarations but also don't need substitution, a class `Member` is introduced that can refer to either of these kinds of member.

Co-authored-by: Geoff Romer <gromer@google.com>
Co-authored-by: Jon Meow <jperkins@google.com>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Support for member access expressions with syntax `container.member`, covering cases such as:

* `object.field`
* `object.method(args)`
* `package.namespace.class.member`
* `object.(interface.member)`
* `object.(class.member)`

... and so on. Includes the rule for template name lookup as decided in #949.

Co-authored-by: josh11b <josh11b@users.noreply.github.com>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Implement initial support for `A.(B)` syntax, per #989. Specifically, this supports:

* `object.(Type.member)` for instance members,
* `object.(Interface.member)` for instance and non-instance members,
* `object.(Type.(Interface.member))` for instance members,
* `Type.(Interface.member)` for non-instance members.

Three new AST nodes are introduced:

* `CompoundFieldAccessExpression` represents the `A.(B)` syntax.
* `MemberName` is a `Value` that represents the result of evaluating an expression such as `Type.member` or `Interface.member` or `Type.(Interface.member)`.
* `TypeOfMemberName` is the type of a `MemberName` value.

In order to handle members of classes and interfaces which have corresponding declarations and may need substitution into their types, and members of structs which don't have declarations but also don't need substitution, a class `Member` is introduced that can refer to either of these kinds of member.

Co-authored-by: Geoff Romer <gromer@google.com>
Co-authored-by: Jon Meow <jperkins@google.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 19, 2023
First step in updating `docs/design/generics/details.md`. It
incorporates changes from proposals: #989 #2138 #2173 #2200 #2360 #2964
#3162 , but there are still more changes from those proposals to be
made.

It also switches away from suggesting static-dispatch witness tables,
and creates an appendix to describe that decision.

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>
github-merge-queue bot pushed a commit that referenced this pull request Oct 3, 2023
Continued from part 1: #3231. Second step updating
`docs/design/generics/details.md`. There remains some work to
incorporate proposal #2200.

- The biggest changes are incorporating much of the text of proposals:
  - #2173
  - #2687
- It incorporates changes from proposals:
  - #989
  - #1178
  - #2138
  - #2200
  - #2360
  - #2964
  - #3162
- It also updates the text to reflect the latest thinking from leads
issues:
  - #996
  - #2153 -- most notably deleting the section on `TypeId`.
- Update to rule for prioritization blocks with mixed type structures
from [discussion on
2023-07-18](https://docs.google.com/document/d/1gnJBTfY81fZYvI_QXjwKk1uQHYBNHGqRLI2BS_cYYNQ/edit?resourcekey=0-ql1Q1WvTcDvhycf8LbA9DQ#heading=h.7jxges9ojgy3)
- Adds reference links to proposals, issues, and discussions relevant to
the text.
- Also tries to use more precise language when talking about
implementations, to avoid confusing `impl` declaration and definitions
with the `impls` operator used in `where` clauses, an issue brought up
in
  - #2495 
  - #2483

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal accepted Decision made, proposal accepted proposal A proposal
Projects
No open projects
Proposals
Accepted
Development

Successfully merging this pull request may close these issues.

None yet

5 participants