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

extend api #3802

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from
Open

extend api #3802

wants to merge 27 commits into from

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Mar 20, 2024

Allow types to extend api other types, adding the names from the other type to its namespace, for forwarding and delegation use cases.

@josh11b josh11b added proposal A proposal proposal draft Proposal in draft, not ready for review labels Mar 20, 2024
josh11b added a commit to josh11b/carbon-lang that referenced this pull request Mar 20, 2024
@josh11b josh11b marked this pull request as ready for review May 4, 2024 01:30
@github-actions github-actions bot requested a review from zygoloid May 4, 2024 01:31
@josh11b josh11b added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels May 4, 2024
proposals/p3802.md Outdated Show resolved Hide resolved
proposals/p3802.md Outdated Show resolved Hide resolved
proposals/p3802.md Outdated Show resolved Hide resolved
declaration, so these lookups into `T` can succeed.

The general rule for resolving ambiguity for `extend`, which we apply here as
well, is that if lookup into `Box(T)` succeeds, then that result is used and no
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be easier to follow if it appeared after the Box example below or had a link to that example.

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 rewrote it a bit, but let me know if it would be better to move this until after the Box example.

proposals/p3802.md Outdated Show resolved Hide resolved
proposals/p3802.md Outdated Show resolved Hide resolved
proposals/p3802.md Outdated Show resolved Hide resolved
return {.ptr = self.Op(p->ptr)};
}
}
// (Plus impls for the other three combinations of ValueBind and RefBind.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Exploring these myself:

// <Ref(T) value>.<ref bindable> produces a Ref(R) value expression.
impl forall [T:! type, U:! RefBind(T)]
    U as ValueBind(Ref(T)) where .Result = Ref(U.Result) {
  fn Op[self: Self](p: Ref(T)) -> Result {
    return {.ptr = self.Op(p->ptr)};
  }
}

// <Ref(T) ref expr>.<ref bindable> produces a reference expression.
// Reference collapse: no added Ref(...) here.
impl forall [T:! type, U:! RefBind(T)]
    U as RefBind(Ref(T)) where .Result = U.Result {
  fn Op[self: Self](p: Ref(T)*) -> Result* {
    return self.Op(p->ptr);
  }
}

// <Ref(T) value>.<value bindable> produces a value expression.
// A value binding is performed to turn the Ref(T) into a T.
impl forall [T:! type, U:! ValueBind(T)]
    U as ValueBind(Ref(T)) where .Result = U.Result {
  fn Op[self: Self](p: Ref(T)) -> Result {
    return self.Op(*p.ptr);
  }
}

I think implementing U as RefBind(Ref(T)) in terms of U as ValueBind(T) is not possible. We can't produce a pointer as a result of the binding.

There's also overlap between the first and third impls. And the first and second violate the new coherence rule in #3720: given ref: Ref({.n: i32}), ref.n will either be a value expression of type Ref(i32) or a reference expression of type i32 depending on the expression category of ref.

I think maybe the right thing to do is to drop the second impl entirely, and have only two impls, both for ValueBind, with a match_first:

match_first {

// <Ref(T) value>.<ref bindable> produces a value expression of type `Ref(R)`.
impl forall [T:! type, U:! RefBind(T)]
    U as ValueBind(Ref(T)) where .Result = Ref(U.Result) {
  fn Op[self: Self](p: Ref(T)) -> Result {
    return {.ptr = self.Op(p->ptr)};
  }
}

// <Ref(T) value>.<value bindable> produces a value expression of type `R`.
// A value binding is performed to turn the Ref(T) into a T.
impl forall [T:! type, U:! ValueBind(T)]
    U as ValueBind(Ref(T)) where .Result = U.Result {
  fn Op[self: Self](p: Ref(T)) -> Result {
    return self.Op(*p.ptr);
  }
}

}

For this to really work, I think we'd need binding to fall back from RefBind to ValueBind if reference binding isn't possible, as discussed in #3720. The above pair of impls would then be implementing the same fallback semantics for Ref(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.

I don't think this works. #3720 only ever produces a reference expression (by automatically dereferencing the returned pointer) when the expression on the left side of the . is a reference expression. I think this makes Ref(T) impossible, and I should probably delete this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had not understood what you were proposing (that you can't get a reference expression, but can get a Ref(T) value that acts like one). I've made these changes, added the additional support for calls on Ref(T) values, and moved this section to future work, as discussed in open discussion on 05-16.

This application of `extend api` and binding was first described in
[this comment on #3720](https://github.com/carbon-language/carbon-lang/pull/3720/files/37e967ff53e89e345eecb49ec79c6dfbe18a3c54#r1513712572).

### Use case: with implicit conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: I'm parking this review here while looking at other things.

proposals/p3802.md Outdated Show resolved Hide resolved
proposals/p3802.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Should be ready for another look now.

return {.ptr = self.Op(p->ptr)};
}
}
// (Plus impls for the other three combinations of ValueBind and RefBind.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had not understood what you were proposing (that you can't get a reference expression, but can get a Ref(T) value that acts like one). I've made these changes, added the additional support for calls on Ref(T) values, and moved this section to future work, as discussed in open discussion on 05-16.

declaration, so these lookups into `T` can succeed.

The general rule for resolving ambiguity for `extend`, which we apply here as
well, is that if lookup into `Box(T)` succeeds, then that result is used and no
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 rewrote it a bit, but let me know if it would be better to move this until after the Box example.

Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the inactive label.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Aug 27, 2024
@josh11b josh11b added long term Issues expected to take over 90 days to resolve. and removed inactive Issues and PRs which have been inactive for at least 90 days. labels Aug 27, 2024
@zygoloid
Copy link
Contributor

@josh11b and I have both lost track of our state on this. We intend to return to this at some point but this PR is not a current priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long term Issues expected to take over 90 days to resolve. 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.

3 participants