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.

Sign up for GitHub

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

Jump to bottom

Remove ExplicitSelf from HIR #33505

Merged
merged 3 commits into from
May 16, 2016
Merged

Remove ExplicitSelf from HIR #33505

merged 3 commits into from
May 16, 2016

Conversation

petrochenkov
Copy link
Contributor

self argument is already kept in the argument list and can be retrieved from there if necessary, so there's no need for the duplication.
The same changes can be applied to AST, I'll make them in the next breaking batch.
The first commit also improves parsing of method declarations and fixes #33413.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented May 9, 2016

LGTM, cc @nrc, @Manishearth, @jseyfried

eddyb
eddyb reviewed May 9, 2016
View reviewed changes
src/libsyntax/ast.rs
impl Arg {
#[unstable(feature = "rustc_private", issue = "27812")]
#[rustc_deprecated(since = "1.10.0", reason = "use `from_self` instead")]
Copy link
Member

Choose a reason for hiding this comment

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

This is actually sort of cool 👍.

@nrc
Copy link
Member

nrc commented May 9, 2016

I think this is good.

Question (more important for an AST change than the HIR): can we tell the difference between &self and self: &Self?

@petrochenkov
Copy link
Contributor Author

@nrc

Question (more important for an AST change than the HIR): can we tell the difference between &self and self: &Self?

Yes,
&self is self: &_
self: &Self is self: &Self.
(There are no AST changes in this PR though, except that the first Arg is constructed more precisely.)

@nrc
Copy link
Member

nrc commented May 9, 2016

@petrochenkov hmm, that seems fine for the HIR, but not ideal for the AST. We should maybe try something else there.

@petrochenkov
Copy link
Contributor Author

&self is self: &_

Hm, with this PR fn f(self: _) and fn f(self: &_) start compiling when manually written in the source code (they didn't before).
If this is not acceptable, I'll have to add one more flag to HIR indicating that one of the self shortcuts was indeed used.

@nrc
Copy link
Member

nrc commented May 9, 2016

If this is not acceptable, I'll have to add one more flag to HIR indicating that one of the self shortcuts was indeed used.

This feels like something which should be caught and reported during lowering, rather than being stored in the HIR (to clarify, I think we should catch this before landing this PR).

@bors
Copy link
Contributor

bors commented May 10, 2016

☔ The latest upstream changes (presumably #33443) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

I've added a special check for self: _ and self: &_ in lowering.rs

@bors
Copy link
Contributor

bors commented May 11, 2016

☔ The latest upstream changes (presumably #33425) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov
Copy link
Contributor Author

ping @eddyb or @nrc

@nrc
Copy link
Member

nrc commented May 14, 2016

r? nrc

@bors
Copy link
Contributor

bors commented May 14, 2016

☔ The latest upstream changes (presumably #33532) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
syntax: Refactor parsing of method declarations
212d5d4 
Fix spans and expected token lists, fix rust-lang#33413 + other cosmetic improvements
Add test for rust-lang#33413
Convert between `Arg` and `ExplicitSelf` precisely
Simplify pretty-printing for methods
@petrochenkov
Remove ExplicitSelf from HIR
766f9b5
@petrochenkov
Add checks for self: _ and self: &_
a62a690
@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov petrochenkov mentioned this pull request May 15, 2016
Merged
@nrc
Copy link
Member

nrc commented May 15, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 15, 2016

📌 Commit a62a690 has been approved by nrc

@nrc
Copy link
Member

nrc commented May 15, 2016

@bors: r-

(Whoops, wrong PR)

nrc
nrc reviewed May 15, 2016
View reviewed changes
src/libsyntax/ast.rs
/// `self`, `mut self`
Value(Ident),
/// `&'lt self`, `&'lt mut self`
Region(Option<Lifetime>, Mutability, Ident),
Copy link
Member

Choose a reason for hiding this comment

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

Borrowed is better than Region, I think

Copy link
Member

Choose a reason for hiding this comment

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

oh nm, this is just moved code

@nrc
Copy link
Member

nrc commented May 15, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 15, 2016

📌 Commit a62a690 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 16, 2016

⌛ Testing commit a62a690 with merge e87cd7e...

bors added a commit that referenced this pull request May 16, 2016
@bors
Auto merge of #33505 - petrochenkov:self, r=nrc
e87cd7e 
Remove ExplicitSelf from HIR

`self` argument is already kept in the argument list and can be retrieved from there if necessary, so there's no need for the duplication.
The same changes can be applied to AST, I'll make them in the next breaking batch.
The first commit also improves parsing of method declarations and fixes #33413.

r? @eddyb
@bors
Copy link
Contributor

bors commented May 16, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit a62a690 into rust-lang:master May 16, 2016
This was referenced May 16, 2016
Merged
Merged
@mcarton mcarton mentioned this pull request May 17, 2016
Merged
@kriomant
Copy link

kriomant commented May 24, 2016 •
edited
Loading

Seems like this PR broke my compiler plugin. Can't you please help to fix it?

UPD: problem solved (see topic)

Manishearth added a commit to Manishearth/rust that referenced this pull request May 26, 2016
@Manishearth
Rollup merge of rust-lang#33644 - petrochenkov:selfast, r=nrc
6c990bf 
 The AST part of rust-lang#33505.
rust-lang#33505 isn't landed yet, so this PR is based on top of it.

r? @nrc

plugin-[breaking-change] cc rust-lang#31645 @Manishearth
Manishearth added a commit to Manishearth/rust that referenced this pull request May 27, 2016
@Manishearth
Rollup merge of rust-lang#33644 - petrochenkov:selfast, r=nrc
7905452 
 The AST part of rust-lang#33505.
rust-lang#33505 isn't landed yet, so this PR is based on top of it.

r? @nrc

plugin-[breaking-change] cc rust-lang#31645 @Manishearth
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fn f(*) {} successfully compiles in method position
5 participants
@petrochenkov @eddyb @nrc @bors @kriomant