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

Support inferring&completing `Self` type in enum/struct/union definitions #1924

Merged
merged 2 commits into from Oct 8, 2019

Conversation

@ice1000
Copy link
Contributor

commented Sep 27, 2019

Signed-off-by: ice1000 ice1000kotlin@foxmail.com

An attempt to fix #1908.
This code works, but I believe the implementation is ugly. Please give me suggestions!

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

I've updated the comment in #1908 with a screenshot.

@flodiebold

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Note that this should also work for structs; also, Self should refer to the whole type with its type parameters, not just the enum. E.g.:

struct List<T> {
    elem: T,
    next: Option<Box<Self>>,
}

(For a test, I'd shorten that to struct List<T> { next: Self } -- it doesn't matter that this wouldn't compile.)

We already resolve Self in impl blocks, and this is very similar. The Resolver works by having a stack of scopes of various kinds, one of them ImplBlock. When building a Resolver for an impl block, we push such a scope on the stack here. The ImplBlockScope is then handled in a few places in resolve.rs; when resolving Self as a type, we return TypeNs::SelfType. And that's then mainly handled here during type lowering to resolve the target type of the impl.

I think I'd suggest adding an AdtScope(Adt) in the Scope enum, and an AdtSelfType(Adt) in the TypeNs enum. (Note that this is different from the existing TypeNs::Adt, because that one takes type parameters, while Self always gets the type parameters from the definition.) The scope should be handled basically the same way as the impl block scope; we just need to take care to push it in the resolver methods of Struct, Enum and Union. And when handling TypeNs::AdtSelfType in lower.rs, you can just use the ty method to get the definition type.

(Handling the scope here will also mean we'll complete Self in struct/enum definitions, by the way.)

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Changed implementation into Scope::AdtSelfScope, please take a look @flodiebold

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
crates/ra_hir/src/code_model.rs Outdated Show resolved Hide resolved
crates/ra_hir/src/resolve.rs Outdated Show resolved Hide resolved
crates/ra_hir/src/ty/infer.rs Outdated Show resolved Hide resolved
@flodiebold

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Looks good to me apart from what @matklad already wrote! You could add a test for completing Self in this situation (see here).

Feel free to r+ with @matklad's suggestions:

bors delegate+

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

✌️ ice1000 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

I'm gonna apply the review suggestions locally instead of using GitHub.

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

I'm very nervous. It's my first time to summon bors for merging a PR.

@ice1000 ice1000 changed the title Support inferring `Self` type in enum definitions Support inferring&completing `Self` type in enum/struct/union definitions Oct 8, 2019
@matklad

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@ice1000 we have a rather extensive test suite, so the probability that something breaks spectacularly is pretty low :) Just reply with bors r+ comment

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Between the darkness and the dawn, a PR bot rises!

bors r+

bors bot added a commit that referenced this pull request Oct 8, 2019
Merge #1924
1924: Support inferring&completing `Self` type in enum/struct/union definitions r=ice1000 a=ice1000

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

An attempt to fix #1908.
This code works, but I believe the implementation is ugly. Please give me suggestions!

Co-authored-by: ice1000 <ice1000kotlin@foxmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

@bors bors bot merged commit b043358 into rust-analyzer:master Oct 8, 2019
2 checks passed
2 checks passed
bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ice1000 ice1000 deleted the ice1000:sticky-fingers branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.