Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Spec out as return only #6483
Spec out as return only #6483
Changes from 2 commits
ced7d7b
b74cee0
639e06b
ee380bf
32a22bf
99e3690
c25884f
a85f49c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's a little confusing that we're calling this the "safe-to-escape of the method return", and also in the previous set of bullets, but we ignore different arguments in each case.
I'm not sure how to make it clearer though. Perhaps just add a "where:" to the end of bullet 1 in each case.
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 was really trying to lean on method invocation rules here and essentially re-use them. Cause really that is what this is doing, using the method invocation rules to calculate the narrowest possible return and validate it's okay to assign to every
ref
/out
parameter. The method invocation rules are simple enough now, maybe just restate them here and note that this is effectively applyiyng method escape rules.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.
@cston here is the version where I don't rely on re-using method invocation rules. Do you think this is clearer?
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.
That sounds good. One concern though is step 2 refers to "a value with that safe-to-escape" but step 1 doesn't call the result a safe-to-escape value. Perhaps step 1 could be:
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.
@RikkiGibson are you okay with this change of the MAMM language?
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 of calling method as the "minimum" scope, so I found the language confusing.
Consider using a term like "narrowest" instead.
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.
In general I'd like if we could avoid saying bigger, smaller, minimum, etc. for scope, and say narrower, wider, narrowest, widest instead. Otherwise mentally switching between the spec and implementation becomes very difficult.
I imagine that when we invest in this area of the compiler later on, we could consider introducing a scope type where "narrower" scopes are also considered "smaller" per the comparison operators. I think that's one of several aspects of the implementation which makes understanding it more difficult.
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.
Okay, updated this section.
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 gave a lot of consideration to
[UnscopedRef] out
being a compilation error. Basically just cut off the problem. But it is realistic, although very very corner, that someone would have assigned and returned anout
to aref struct
byref
so decided to leave it.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.
One thing I considered briefly was having the STE of
[UnscopedRef] out
be CallingMethod, to ensure thatoutParam = M(ref outParam)
is an error. But it just started to feel too complicated. I'm happy with play stupid games, win stupid prizes as a resolution here.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.
Should this be "a return value where safe-to-escape is ..."?
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 it has to be a local. Consider that a constructor body has assignments, method calls, etc ... All of those need to happen on a value. If it's return only when binding the method body how do we evaluate the calls? A local works because it can be STS return only and RSTS containing method.
Yes that means there is a copy at the end. That would matter if we actually implemented ctors that way. Instead this is just how do we model ref safety analysis. A local mostly works.