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

Replace keyword is with impls #2483

Merged
merged 30 commits into from
Mar 23, 2023
Merged

Replace keyword is with impls #2483

merged 30 commits into from
Mar 23, 2023

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Dec 21, 2022

Use the keyword impls instead of is when writing a where constraint that a type variable needs to implement an interface or named constraint.

What was previously (provisionally) written:

fn Sort[T:! Container where .ElementType is Ordered](x: T*);

will now be written:

fn Sort[T:! Container where .ElementType impls Ordered](x: T*);

@josh11b josh11b added proposal A proposal proposal draft Proposal in draft, not ready for review labels Dec 21, 2022
@josh11b josh11b marked this pull request as ready for review December 21, 2022 21:38
@josh11b josh11b requested a review from chandlerc December 21, 2022 21:38
@github-actions github-actions bot added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels Dec 21, 2022
@github-actions github-actions bot requested a review from KateGregory December 21, 2022 21:38
@github-actions github-actions bot added the explorer Action items related to Carbon explorer code label Dec 21, 2022
@github-actions github-actions bot requested a review from geoffromer December 21, 2022 21:38
proposals/p2483.md Outdated Show resolved Hide resolved
@jonmeow
Copy link
Contributor

jonmeow commented Dec 21, 2022

I'm trying to figure out the best way to put this:

  1. What is impl short for if we were to treat it as a word? Is it the same as with "implements" for impls, or do you think of it as something longer, like "implementation of"?
  2. Will newcomers to the language understand that when we have both, one is not a pluralization of the other?
  3. In documentation, can we also replace "impls" where it's used to mean a plural of impl to be something else?

For example on my point 3, look for uses of "impls" in https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md. Regardless of the rest, I think these will also need to be cleaned up in order to reduce confusion. But I have no idea what to suggest, to my point 2.

@josh11b
Copy link
Contributor Author

josh11b commented Dec 21, 2022

  1. What is impl short for if we were to treat it as a word? Is it the same as with "implements" for impls, or do you think of it as something longer, like "implementation of"?

impl comes from Rust, where I think it is short for "implementation of," as in "implementation of Display for i32." This matches the Carbon convention where the introducer keywords correspond to nouns ("implementation", "function", "variable", "class") not verbs ("implement" or "implements").

@jonmeow
Copy link
Contributor

jonmeow commented Dec 21, 2022

Might also be good to rename pre-existing uses of impls in code in order to avoid confusion (maybe impl_list?)

@josh11b
Copy link
Contributor Author

josh11b commented Dec 21, 2022

Added some text about this concern to the alternatives considered section: 5c31cb5

@jonmeow
Copy link
Contributor

jonmeow commented Dec 22, 2022

Added some text about this concern to the alternatives considered section: 5c31cb5

I think you might be misunderstanding my concern, because the text focuses on the expanded meaning instead of the two possible readings of "impls". This PR also does not fix existing uses of "impls" in order to resolve ambiguity.

For example:

  • generics/details.md has multiple cases, such as "Parameterized impls" (particularly in the Table of Contents)
  • explorer code uses "impls" in variable names.
  • There are proposals such as "Generics details 7: final impls #983", linked from the bottom of details.md
  • "impls" is also going to come up in spoken discussion.

My concern is that, in creating a second possible reading for "impls", it's harder to understand both code and documentation. Context is not always present: I don't think the above examples have much that can be relied on by readers to disambiguate. I think I'll find it challenging to understand the above cases; I would expect this to be particularly difficult for non-native English speakers.

But if you disagree, please at least edit the existing uses of "impls" to remove ambiguity. I believe we should avoid using "impls" when it does not mean impls. "impls" is at least an improvement for markdown, and that's why I suggest "impl_list" in explorer code where we still need a valid variable name.

@jonmeow
Copy link
Contributor

jonmeow commented Dec 22, 2022

My concern is specifically with the choice of impls. If you used the keyword implements instead, that would avoid the ambiguity I'm noting, but I assume it was avoided due to length. Would something like has work for you, or maybe has_impl?

Is this similar to requires from C++20?

@josh11b
Copy link
Contributor Author

josh11b commented Dec 22, 2022

I filed a question-for-leads issue #2495 and posted a question in the #naming channel on Discord about the word to use for this keyword.

@jonmeow
Copy link
Contributor

jonmeow commented Mar 20, 2023

Thanks for the doc edits. :)

@josh11b josh11b requested review from geoffromer and jonmeow March 21, 2023 22:21
@josh11b
Copy link
Contributor Author

josh11b commented Mar 21, 2023

I believe this is now ready for review.

@chandlerc
Copy link
Contributor

FWIW, it would seem useful for this to be split up into maybe proposal, doc-updates, toolchain updates, and explorer updates....

Would doing that at this point create too much git hassle? Fine if so, I'll try to work through it in phases.

josh11b added a commit to josh11b/carbon-lang that referenced this pull request Mar 22, 2023
@josh11b
Copy link
Contributor Author

josh11b commented Mar 22, 2023

FWIW, it would seem useful for this to be split up into maybe proposal, doc-updates, toolchain updates, and explorer updates....

Would doing that at this point create too much git hassle? Fine if so, I'll try to work through it in phases.

I've created #2707 and moved all the explorer and toochain changes into that PR.

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.

Approving as I think the critical and controversial part has been clearly resolved on the issue. =]

The doc updates all LG. I've left two minor suggestions on the proposal. LG and feel free to land with the suggestions. Also happy to chat more if any don't make sense.

proposals/p2483.md Outdated Show resolved Hide resolved
proposals/p2483.md Outdated Show resolved Hide resolved
@josh11b josh11b merged commit 642fcd3 into carbon-language:trunk Mar 23, 2023
@josh11b josh11b deleted the impls branch March 23, 2023 04:45
josh11b added a commit that referenced this pull request Mar 28, 2023
This PR is making two main changes to the Explorer and Toolchain:
- Replace the `is` keyword in `where SomeType is SomeInterface` with `impls`, so it is `where SomeType impls SomeInterface`
- Rewrite uses of the "impls" to something else to avoid, frequently "`impl` declarations" or "implementations", to avoid confusion with the `impls` keyword.

---------

Co-authored-by: Geoff Romer <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Sep 23, 2023
Includes proposals:
- #990
- #2188
- #2138
- #2200
- #2360
- #2760
- #2964
- #3162

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 and #2483;
- "binding patterns", like `x: i32`, and "bindings" like `x`.

---------

Co-authored-by: Chandler Carruth <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code 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.

4 participants