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
Merged

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

merged 2 commits into from Oct 8, 2019

Conversation

ice1000
Copy link
Contributor

@ice1000 ice1000 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
Copy link
Contributor Author

ice1000 commented Sep 27, 2019

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

@flodiebold
Copy link
Member

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
Copy link
Contributor Author

ice1000 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
Copy link
Member

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
Copy link
Contributor

bors bot 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
Copy link
Contributor Author

ice1000 commented Oct 8, 2019

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

@ice1000
Copy link
Contributor Author

ice1000 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
Copy link
Member

matklad 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
Copy link
Contributor Author

ice1000 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
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
Copy link
Contributor

bors bot commented Oct 8, 2019

Build succeeded

@bors bors bot merged commit b043358 into rust-lang:master Oct 8, 2019
@ice1000 ice1000 deleted the sticky-fingers branch October 8, 2019 11:55
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.

Does not infer Self
3 participants