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

Binding operators #3720

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

Binding operators #3720

wants to merge 71 commits into from

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Feb 22, 2024

Define the binding operation used to compute the result of x.y, p->y, x.(C.y), and p->(C.y) as calling a method from user-implementable interfaces.

@josh11b josh11b added proposal A proposal proposal draft Proposal in draft, not ready for review labels Feb 22, 2024
@josh11b josh11b marked this pull request as ready for review February 29, 2024 00:36
@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 Feb 29, 2024
Copy link
Contributor

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

I think there's good detail here on ValueBind and RefBind. I do have several questions about extend api; it's not clear to me what the implementation of this would look like, so I'd like to have more specifics.

proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
Co-authored-by: Jon Ross-Perkins <[email protected]>
proposals/p3720.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.

I think this is looking really great, checking with other leads to see if we're ready to merge.

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.

Broadly I'm very happy with this. I have some specific concerns around how we bottom out and a potential coherence issue arising from the interaction with this and our approach to indexing, but I think those are relatively minor details.

proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
proposals/p3720.md Outdated Show resolved Hide resolved
josh11b and others added 5 commits May 14, 2024 11:46
proposals/p3720.md Outdated Show resolved Hide resolved
This was referenced May 15, 2024
// `T.(y)` is `y.((U as TypeBind(T)).Op)()`.
// For a facet value, which includes all type values, `T` and
// an expression `y` of type `U`, `T.(y)` is
// `y.((U as TypeBind(T)).Op)()`.
interface TypeBind(T:! type) {
let Result:! type;
Copy link
Contributor

@zygoloid zygoloid May 17, 2024

Choose a reason for hiding this comment

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

Are there coherence concerns here? For a given x.(y), we might not know whether x is a facet value.

I don't think we can use Bind here, because we'd want to require that if M impls TypeBind(T) with .Result = R then M impls Bind(type) with .Result = R (and in general, forall [F is a facet type] M impls Bind(F) with .Result = R). But R needs to depend on T, and T isn't in scope in that impl of Bind.

I suppose the upshot of this is that the "facet on the left of ." check must be done in the definition of the generic, and if it turns out that the left-hand expression is a facet once we know its type, that's just too late, and you get the ValueBind / RefBind behavior instead. Something similar comes up elsewhere, where we say a direct member access into a facet doesn't use an overloadable binding operator.

How much should we be worried about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My lack of worry mostly comes from not having an alternative to compare it to.

// `r` is a reference expression and so uses `RefBind`
r.F() == r.(C.F)()
== r.(__C_F)()
== (*inlined_method_call_compiler_intrinsic(
Copy link
Contributor

@zygoloid zygoloid May 17, 2024

Choose a reason for hiding this comment

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

This makes me wonder: does this need to be a primitive dereference, rather than this desugaring to a call to Pointer.Dereference? I think not, because we'd end up in ValueBind rather than RefBind; do you agree?

I think we shouldn't use a primitive dereference here unless we really need to: while there might still be an argument for avoiding the extra work of an impl lookup for the * here, if we want to make * bypass implementation lookup for pointers, we may want to do that centrally in the rewrite for * rather than only in this one case. Also, as a future extension, we might want to allow RefBind.Op to return a value that implements Pointer where .ValueT == Result rather than requiring it to return exactly Result*, at which point we'd need the full dereference machinery here.

proposals/p3720.md Show resolved Hide resolved
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.

Looks good to me; I think this is ready to go ahead. There's one thing I'd like double-checked here, and one more coherence concern, but I think the latter is pre-existing so shouldn't block this proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants