-
Notifications
You must be signed in to change notification settings - Fork 1.5k
ref
parameters, arguments, returns and val
returns
#5434
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
Conversation
ref
parameters, arguments, returns and let
returnsref
parameters, arguments, returns and val
returns
I added a small section saying that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generally looking good to me, minor/editorial comments only inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replying to a few more conversation threads on old content, most of which I think are now resolved.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
proposals/p5434.md
Outdated
// ❌ Invalid. | ||
let ref Size:! i32 = OuterSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is a good example of the previous rule -- if this were both valid without ref
and valid outside a class, I think it should be valid with ref
inside a class too. (I think it's invalid outside a class because we only said that ref
can be used with :
, not :!
, and I think it's invalid without ref
in a class because we don't say that we allow let
declarations in classes at all.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed :!
to :
in both places. Looks like my fingers have gotten to used to typing :!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, isn't this still invalid because you can't have a let
binding in a class? (If we allow a let
in a class to declare a static member, I'd expect let ref
to also be valid.) I think the mention of classes here is redundant, except if we want to point out that -- unlike C++ -- we don't have any way to introduce references as class members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested an edit to hopefully clarify that this is more trying to document the intent of not (yet) having ref bindings as fields, despite there not being a syntactic way to really get there at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested edit works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit has been applied
} | ||
|
||
// ✅ Valid | ||
fn Valid1(bound p: i32*) -> ref i32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean to apply bound
to a pointer, especially a pointer to a pointer? What does bound ref p: i32*
mean?
If we follow the clang::lifetimebound
model, bound
would apply to the pointee of the top-level pointer only, but that's inconsistent with what it would mean for a non-copy-value-representation by-value function parameter, where it would apply to the lifetime of the pointer itself. Is it OK that we have that inconsistency (especially in generic code)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reading of the clang::lifetimebound
model, and what I've tried to propose is that bound
means "return may reference bound parameters, or anything transitively reachable from them."
https://clang.llvm.org/docs/AttributeReference.html#lifetimebound
By default, a reference is considered to refer to its referenced object, a pointer is considered to refer to its pointee, a std::initializer_list is considered to refer to its underlying array, and aggregates (arrays and simple structs) are considered to refer to all objects that their transitive subobjects refer to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang::lifetimebound
doesn't give transitive reachability, only direct reachability through that value (and through its subobjects if it's an aggregate). So for example:
[[clang::lifetimebound]] int **p
means that*p
is bound and**p
is not. (And alsop
itself is not, because that's not meaningful in C++.)[[clang::lifetimebound]] X x
, withstruct X { int a; int *p; int **q; }
means that*x.p
is bound,*x.q
, but**x.q
is not.- Reference parameters are treated like pointer parameters.
(where "object X is bound" means "object X is required to live at least as long as the return value of the function"). [[clang::lifetimebound]]
doesn't have a mechanism to constrain indirect lifetimes (eg, int**
or string_view
).
This doesn't quite match what we're doing in Carbon, because values also potentially have lifetime in Carbon (whereas in C++ function parameters are always var
s), so for a bound p: i32**
, we need to know whether p
is bound, *p
is bound, and/or **p
is bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with saying that the exact semantics of bound
are not being decided in this proposal and being left to a future memory safety proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested an edit to capture this below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested edit works for me.
> different value representation is used. | ||
> Either it needs to restrict to a `const ref` style representation (to prevent | ||
> slicing) or it needs to have a model for the semantics when a different value | ||
> representation is used. | ||
|
||
### Interop with C++ `const &` and `const` methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Commenting here since there's no better place for it)
If I understand correctly, the "Deferred initialization from values and references" section below is basically just saying that the compiler can rewrite -> Foo
to -> let Foo
if that would be safe and correct (see also discussion here). If so, it would be nice to say so explicitly, and remove or relocate all the verbiage that amounts to explaining the semantics of -> let
. It's fine if that's a TODO, I just want to make sure it doesn't get lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this would remove the explanation of the semantics of -> val Foo
?
I also think we don't yet know the exact model we want here in terms of value representation, and so not sure there is anything to really do here yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this would remove the explanation of the semantics of
-> val Foo
?
Sorry, I was unclear. It seems like that section's explanation of the alternate semantics that the compiler is allowed to give to -> Foo
, is also an explanation of the semantics of -> val Foo
. If so, that material should either be replaced with a cross-reference to the canonical documentation for -> val Foo
, or it should be relocated and rephrased in order to become the canonical documentation for -> val Foo
(leaving behind a cross-reference to the new location).
I also think we don't yet know the exact model we want here in terms of value representation, and so not sure there is anything to really do here yet.
You think it's too soon to even have a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replied on three comment threads here; other than those I'm happy with this, and I don't think there's anything significant there, other than wanting more clarity as to what exactly bound
means, given that there seems to be a mismatch between [[clang::lifetimebound]]
and what Carbon wants for bound
applied to value bindings with pointer value reperesentations -- but I'm also happy with that being explicitly deferred in this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have suggested edits that should address the last blocking comment threads, so approving this for leads to merge once these are in!
> different value representation is used. | ||
> Either it needs to restrict to a `const ref` style representation (to prevent | ||
> slicing) or it needs to have a model for the semantics when a different value | ||
> representation is used. | ||
|
||
### Interop with C++ `const &` and `const` methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this would remove the explanation of the semantics of -> val Foo
?
I also think we don't yet know the exact model we want here in terms of value representation, and so not sure there is anything to really do here yet.
|
||
Note that `ref` is disallowed inside `var` since that would be redundant. | ||
|
||
### Mututation restriction on objects bound to a value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to look for something we can use, but I'm comfortable with that being a follow-up proposal to add a good definition. Also don't think its blocking given that we have an implicit one and the clarification needed in this proposal.
} | ||
|
||
// ✅ Valid | ||
fn Valid1(bound p: i32*) -> ref i32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested an edit to capture this below.
proposals/p5434.md
Outdated
// ❌ Invalid. | ||
let ref Size:! i32 = OuterSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested an edit to hopefully clarify that this is more trying to document the intent of not (yet) having ref bindings as fields, despite there not being a syntactic way to really get there at this point.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
ref
instead ofvar
or the default. It will bind to reference argument expressions in the caller and produces a reference expression in the callee.ref
binding can't be rebound to a different object.addr
, and is not restricted to theself
parameter.ref
binding, like a value binding, can't be used in fields of classes or structs.self
ref
parameters are also marked withref
.ref
,val
, orvar
. These control the category of the call expression invoking the function, and how the return expression is returned.ref
binding isnocapture
andnoalias
.bound
.