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

Add self argument destructuring. Closes #7613. #12566

Closed
wants to merge 1 commit into from

Conversation

qpliu
Copy link
Contributor

@qpliu qpliu commented Feb 26, 2014

No description provided.

SelfStatic => None,
SelfValue(pat) | SelfRegion(_,_,pat) | SelfUniq(pat) => pat,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Does ExplicitSelf really need to contain the pattern?

Copy link
Member

Choose a reason for hiding this comment

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

Seems the place of least resistance to add it, to me.

Copy link
Member

Choose a reason for hiding this comment

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

ExplicitSelf is only alive because we can't use Self in all methods, otherwise I would've killed it completely.
Nothing uses the bundled pattern, other than the code extracting it to produce the proper ast::Arg structure, which is only needed in the parser, which has direct access to the pattern anyway.

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 would think that pretty printing justifies the addition of the pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Pretty printing could just take the pattern out of the first argument, couldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. It'll certainly make diffs much smaller.

@emberian
Copy link
Member

Great work! debuginfo probably needs updating. cc @michaelwoerister

@emberian
Copy link
Member

The PR looks fine to me. However, I think its test coverage is a bit lacking. In particular, patterns in trait declarations are not covered. For example:

trait Foo {
    fn get_val(&self) -> int;
    fn do_stuff(&self @ SomeGarbage(a, b)) -> int { self.get_val() + 2 }
}

should not be allowed (the trait doesn't know what type Self is to destructure it).

If at all possible, I think the ideal syntax, to be in line with other arguments, would be pattern: &self. Then you could write fn cross(&Vector(a, b, c): &self, &Vector(d, e, f): &Vector) -> Vector, which I think is much more appealing.

@eddyb
Copy link
Member

eddyb commented Feb 26, 2014

I think pattern: &self is a bit misleading, because (mut )?XXselfYY is equivalent to (a hypothetical) (mut )?self: XXSelfYY.
We really need to discuss what to do about UFCS and Self, because the cases where we want to use it in impls are piling up.

@brson
Copy link
Contributor

brson commented Feb 27, 2014

Thanks!

Let's please hold off on merging this for further discussion, since there has been no discussion about this feature lately and I had no plans to implement it in the near future.

@brson
Copy link
Contributor

brson commented Mar 6, 2014

Sorry @qpliu I didn't mean to completely stop progress on this PR. This is a cool feature. I just want to make sure that new features are added with increased consideration.

I'll put this subject on the agenda for the next weekly meeting: #12566

@brson
Copy link
Contributor

brson commented Mar 11, 2014

Sorry for the delay.

Per today's meeting, we have a different plan to make self arguments destructurable. With universal function-call syntax (UFCS #11938) there will not be any distinction between static methods and instance methods - they will both be 'associated functions'. At that point any function who's first argument is the self type will be callable with method syntax, and self, &self, and &mut self are just sugar for i.e. self: &Self, and destructuring on the self argument can be done as normal by not using the self-sugar.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants