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

Allow unqualified name lookup for class members #2287

Merged
merged 14 commits into from
Nov 9, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Oct 13, 2022

Allow unqualified name lookup in multiple situations:

  • For classes and interfaces, whether inside the class scope or within an
    out-of-line function definition.
  • For namespaces, when the namespace is used in a declaration.

zygoloid
zygoloid previously approved these changes Oct 13, 2022
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like @chandlerc to double-check this is what we expected after #875.

@jonmeow jonmeow changed the title Update class design to reflect #875 and include alternatives. Allow unqualified name lookup for class members Oct 21, 2022
@jonmeow jonmeow requested a review from zygoloid October 21, 2022 17:42
@jonmeow
Copy link
Contributor Author

jonmeow commented Oct 21, 2022

Since this PR is still floating around, as is #2286, I'm just switching to a proposal format.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good, and follows the principle in #875.

As overall design direction, I think we should apply this approach broadly, not only to classes but also to impls, adapters, and interfaces with default functions. I think I'm OK with specifying a rule for only classes now, and extending it with separate proposals for the other contexts, but I wonder if it would be less work overall to tackle all of those contexts in the same proposal.

proposals/p2287.md Outdated Show resolved Hide resolved
docs/design/classes.md Show resolved Hide resolved
- [Interoperability with and migration from existing C++ code](/docs/project/goals.md#interoperability-with-and-migration-from-existing-c-code)
- This behavior will be similar to how C++ works.

## Alternatives considered
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like us to consider the alternative of treating me as an ordinary parameter within method bodies, meaning that (modulo aliasing) it can be accessed only by explicitly naming it. In C++, I have often found it to be a readability obstacle that you can't easily find the places in a method body that access *this. Treating me as an explicitly-declared parameter gives us a clear path to avoiding that problem, so if we're not going to take it, I think we should explain why.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this changes the use of me at all? My impression was that this only applies to unqualified lookup that doesn't use the instance at all (or *this from C++), and only uses the type's scope.

Copy link
Contributor

@zygoloid zygoloid Oct 21, 2022

Choose a reason for hiding this comment

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

I think it would be a good idea for the proposal to say something about this to avoid confusion. My interpretation is that given:

class X {
  var v: i32;
  fn F[me: Self]() {
    v = 1;
    X.v = 1;
    me.v = 1;
    me.(v) = 1;
    me.(X.v) = 1;
  }
}
  • For v = 1;, unqualified lookup finds X.v, so this is equivalent to X.v = 1;, not me.v = 1;
  • For X.v = 1;, we are referring to an instance member without an instance, which is an error.
  • For me.v = 1;, we perform instance binding and assign to the member v of me.
  • For me.(v) = 1;, unqualified lookup finds X.v, so this is equivalent to me.(X.v) = 1;
  • For me.(X.v) = 1;, we perform instance binding and assign to the X member v of me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the missing part of zygoloid's example (see 981-985 in classes.md).

Note, my intent was just to fix out-of-date information and address what I thought would be a non-contentious change. I don't see me as either:

  1. I'm not familiar with the rationale behind disallowing me. references, nor could I find it considered anywhere.

  2. Requiring me. is consistent with C++ and seems more ergonomic to write (I don't think it's been a source of confusion in C++ code). Open design idea: implicit scoped function parameters #1974 also offers a way to make it work: me could be an implicit scoped parameter.

I'm not readily familiar with the argument for not supporting lookups with me. There may be refactoring reasons for disallowing it, but I would think those would be shared with unqualified name lookup in general.

Because the issue hasn't been addressed previously and shares problems with unqualified name lookup on classes, I think it's fair to ask here. That said, I also don't feel comfortable resolving this, so I've added notes for an open question.

Is the open question a reasonable answer for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I overlooked the distinction between name lookup and instance binding, so I thought this was proposing to e.g. make v = 1; be valid and equivalent to me.v = 1 in @zygoloid's example. I've added a suggestion to the abstract that would have cleared up my confusion. With that change or something like it, I don't think we even need the open question (but it doesn't hurt to have it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want some reference for this -- do you have an alternative reference that I can use in place of the open question?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would stress either way, the open question seems fine.

docs/design/classes.md Outdated Show resolved Hide resolved
docs/design/classes.md Outdated Show resolved Hide resolved
- [Interoperability with and migration from existing C++ code](/docs/project/goals.md#interoperability-with-and-migration-from-existing-c-code)
- This behavior will be similar to how C++ works.

## Alternatives considered
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this changes the use of me at all? My impression was that this only applies to unqualified lookup that doesn't use the instance at all (or *this from C++), and only uses the type's scope.

docs/design/classes.md Show resolved Hide resolved
Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

@zygoloid :

As overall design direction, I think we should apply this approach broadly, not only to classes but also to impls, adapters, and interfaces with default functions. I think I'm OK with specifying a rule for only classes now, and extending it with separate proposals for the other contexts, but I wonder if it would be less work overall to tackle all of those contexts in the same proposal.

I've tried addressing more of this, but note that forward declarations aren't defined in many of these cases, so I've added an open question noting that.

docs/design/classes.md Outdated Show resolved Hide resolved
proposals/p2287.md Outdated Show resolved Hide resolved
- [Interoperability with and migration from existing C++ code](/docs/project/goals.md#interoperability-with-and-migration-from-existing-c-code)
- This behavior will be similar to how C++ works.

## Alternatives considered
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the missing part of zygoloid's example (see 981-985 in classes.md).

Note, my intent was just to fix out-of-date information and address what I thought would be a non-contentious change. I don't see me as either:

  1. I'm not familiar with the rationale behind disallowing me. references, nor could I find it considered anywhere.

  2. Requiring me. is consistent with C++ and seems more ergonomic to write (I don't think it's been a source of confusion in C++ code). Open design idea: implicit scoped function parameters #1974 also offers a way to make it work: me could be an implicit scoped parameter.

I'm not readily familiar with the argument for not supporting lookups with me. There may be refactoring reasons for disallowing it, but I would think those would be shared with unqualified name lookup in general.

Because the issue hasn't been addressed previously and shares problems with unqualified name lookup on classes, I think it's fair to ask here. That said, I also don't feel comfortable resolving this, so I've added notes for an open question.

Is the open question a reasonable answer for now?

docs/design/classes.md Outdated Show resolved Hide resolved
- For classes and interfaces, whether inside the class scope or within an
out-of-line function definition.
- For namespaces, when the namespace is used in a declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This only changes the rules for name lookup, not for instance binding, so an unqualified name still cannot be used to refer to an instance member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about the open question to the proposal section.


## Proposal

Allow unqualified name lookup which will use the appropriate scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a little funny to have the abstract be more concrete than the proposal section. Maybe swap the content of the two sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The abstract and the PR description are supposed to match. The text in the abstract is giving the amount of detail I believe to be correct here.

The text here is once sentence because of the paragraphs that follow it; that's my intent, to use sub-headers to explain what's only briefly noted in the abstract. It didn't seem long enough to be worth duplicating the abstract here.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think the abstract and proposal text makes sense to me at this point? the abstract is a summary, and the subsections here seem to directly correspond so I think this is OK now?

- [Interoperability with and migration from existing C++ code](/docs/project/goals.md#interoperability-with-and-migration-from-existing-c-code)
- This behavior will be similar to how C++ works.

## Alternatives considered
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I overlooked the distinction between name lookup and instance binding, so I thought this was proposing to e.g. make v = 1; be valid and equivalent to me.v = 1 in @zygoloid's example. I've added a suggestion to the abstract that would have cleared up my confusion. With that change or something like it, I don't think we even need the open question (but it doesn't hurt to have it).

jonmeow added a commit that referenced this pull request Oct 24, 2022
@josh11b josh11b added proposal A proposal proposal rfc Proposal with request-for-comment sent out labels Oct 26, 2022
@jonmeow
Copy link
Contributor Author

jonmeow commented Nov 7, 2022

Just noting, it's been 2 weeks since last activity.

docs/design/classes.md Outdated Show resolved Hide resolved
proposals/p2287.md Outdated Show resolved Hide resolved
proposals/p2287.md Outdated Show resolved Hide resolved
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.

With the clarifications and things marked as out-of-scope, this LGTM.


## Proposal

Allow unqualified name lookup which will use the appropriate scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think the abstract and proposal text makes sense to me at this point? the abstract is a summary, and the subsections here seem to directly correspond so I think this is OK now?

- [Interoperability with and migration from existing C++ code](/docs/project/goals.md#interoperability-with-and-migration-from-existing-c-code)
- This behavior will be similar to how C++ works.

## Alternatives considered
Copy link
Contributor

Choose a reason for hiding this comment

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

I would stress either way, the open question seems fine.

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.

Confirmed with other leads, and we're happy to move forward here.

We've been trying to not stress too much about the exact polish level of proposal text, so I think where its at is fine. And we can (and should) keep updating and improving the design wording as needed in follow-up patches. =D

@jonmeow jonmeow merged commit 14cc716 into carbon-language:trunk Nov 9, 2022
@jonmeow jonmeow deleted the class-lookup branch November 9, 2022 22:34
@chandlerc chandlerc added the documentation An issue or proposed change to our documentation label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An issue or proposed change to our documentation proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants