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

internal: Handle trait alias definitions #14184

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Feb 21, 2023

Part of #2773

This PR adds a bunch of structs and enum variants for trait aliases. Trait aliases should be handled as an independent item because they are semantically distinct from traits.

I basically started by adding TraitAlias{Id, Loc} to hir_def::item_tree and iterated adding necessary stuffs until compiler stopped complaining what's missing. Let me know if there's still anything I need to add.

I'm opening up this PR for early review and stuff. I'm planning to add tests for IDE functionalities in this PR, but not type-related support, for which I put FIXME notes.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2023
@flodiebold
Copy link
Member

Looks good to me 🎉

crates/hir/src/symbols.rs Outdated Show resolved Hide resolved
@lowr
Copy link
Contributor Author

lowr commented Feb 23, 2023

Update: I began adding test, which quickly made me realize that I forgot to impl ToDef - which wasn't straightforward because that trait pretty much requires one AST type to map to one HIR type whereas (previously) ast::Trait maps to either hir::Trait or hir::TraitAlias. There are 2 ways to solve:

  • map ast::Trait to Either<hir::Trait, hir::TraitAlias>
  • split ast::TraitAlias from ast::Trait

I've implemented taking the second approach as it seemed simpler, but I'm happy to hear opinions and thoughts.

EDIT: I've experimented with the first approach and I found it somewhat cumbersome to deal with Either every time we get def from ast::Trait. I'm inclined toward my current implementation.

@lowr lowr marked this pull request as ready for review February 25, 2023 12:05
@lowr
Copy link
Contributor Author

lowr commented Feb 25, 2023

This is now ready for full review. The commits are starting to get cluttered, I can squash relevant commits after review if desired.

Fully working IDE functionalities:

  • doc_links
  • file_structure
  • move_item
  • navigation_target
  • runnables
  • highlight_related

Partially working IDE functionalities (generally working for trait alias itself, but needs type support for full functionalities):

  • goto_definition
  • goto_type_definition
  • hover
  • inlay_hints
  • moniker
  • references
  • rename
  • signature_help

non-applicable functionalities: all others

@bors
Copy link
Contributor

bors commented Mar 1, 2023

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

@lowr
Copy link
Contributor Author

lowr commented Mar 3, 2023

Rebased and organized commits. There's no diff in the final tree modulo now resolved conflict (which was merely one import statement). This PR should be good to go!

@Veykril
Copy link
Member

Veykril commented Mar 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2023

📌 Commit f8eac19 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 3, 2023

⌛ Testing commit f8eac19 with merge 6756294...

@bors
Copy link
Contributor

bors commented Mar 3, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 6756294 to master...

@bors bors merged commit 6756294 into rust-lang:master Mar 3, 2023
@lowr
Copy link
Contributor Author

lowr commented Mar 4, 2023

Might be worth noting explicitly in the changelog that we haven't implemented trait alias in our type system.

FYI, I was going to implement type support for trait alias, but after seeing rust-lang/rust#107614, I'm putting it on hold until types/lang teams decide what to do with it.

@lnicola lnicola changed the title Handle trait alias definitions internal: Handle trait alias definitions Mar 4, 2023
@HKalbasi
Copy link
Member

@lowr Do you know what is the current state of trait aliases? Did the dust settle?

@lowr
Copy link
Contributor Author

lowr commented Jul 25, 2023

@HKalbasi I'm not really sure about its current state. The PR I mentioned in my last comment has been merged but without any decisions involved (which is reasonable as it's an unstable feature after all).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants