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

REWRITE ALL THE THINGS #66

Merged
merged 65 commits into from
Feb 9, 2021
Merged

REWRITE ALL THE THINGS #66

merged 65 commits into from
Feb 9, 2021

Conversation

regexident
Copy link
Owner

@regexident regexident commented Dec 5, 2020

I started refactoring the project, moving things from depending on 'rustc_…' (rustc internals) to 'ra_ap_…' (rust-analyzer internals)

Progress:

  • $ cargo modules tree (now called $ cargo modules generate tree)
  • $ cargo modules graph (now called $ cargo modules generate graph)

Since we're moving from performing analysis on a project's AST to analysis on its HIR we should be able to fix a bunch more issues in the long term, which right now see kind of hard if not impossible to fix:

It should also make these features feasible:


(Once finished this will fix #63.)

@regexident regexident force-pushed the refactor branch 3 times, most recently from a914f0c to b3fdc98 Compare December 9, 2020 18:43
@regexident regexident force-pushed the refactor branch 2 times, most recently from aa0f2b4 to 7d304ce Compare December 9, 2020 19:01
@regexident
Copy link
Owner Author

Orphans working again in cargo modules tree

@kvark
Copy link
Contributor

kvark commented Dec 21, 2020

Please let me know when it would make sense to start testing it! Super excited to get that ball rolling on WR, once available.
Today, running the tree command isn't producing anything for me, just:

compositor_windows: pub

@regexident
Copy link
Owner Author

regexident commented Dec 23, 2020

@kvark: right now the target selection does a very, very hack-ish "select first library found", iirc.

As such it could be possible that the logic somehow picked the compositor-windows package as target. The compositor-windows crate contains just a single modules, which matches your terminal output.

I still need to add a proper CLI target selection feature, as well as add proper support for workspace handling.

Target selection is the next thing I'll be working on, as for the cargo modules graph logic I'll need to do some more reading up on how rust-analyzer handles imports (I also think the graph should not be restricted to explicit imports, but any used paths per module).


If your wanted to test the new output of cargo modules tree today I'd suggest cloning the rust-lang/cargo project and testing it via cargo run --release -- tree /path/to/cargo. It's one of the projects I'm using to test things.

Quick heads-up: You will notice the tool is excruciatingly slow compared to current master (even when running as release build). This is due to the switch from analyzing rustc's pure AST to analyzing rust-analyzer's HIR, which is a more involved process. There's no free lunch, sadly.

@kvark
Copy link
Contributor

kvark commented Jan 7, 2021

Ok, I see. Thank you for the info! Graph is the stuff I need, and I'm waiting eagerly for it to be available :)

@regexident regexident marked this pull request as ready for review February 2, 2021 12:07
@regexident regexident force-pushed the refactor branch 2 times, most recently from 22e3f49 to e7c782c Compare February 4, 2021 09:27
@regexident
Copy link
Owner Author

@kvark Commit f7d226f removed any remaining ra_ap_hir values from the graph's nodes, which were preventing persisting the graph to disk.

As such adding a --cache <FILE> (and a complementary --ignore-cache, I suppose) should now just be a matter of some minor elbow grease.

The idea would be to have cargo-modules skip the src -> AST -> HIR -> graph steps and jump straight to the graph, loading it from disk if a previously persisted graph is found at <FILE>. The persisted graph would be tagged with cargo-module's current version and outdated caches ignored.

@kvark
Copy link
Contributor

kvark commented Feb 4, 2021

So that would allow me to re-launch it with different focus points quickly? Or is it something else?

@regexident
Copy link
Owner Author

Once implemented, yes. That's the idea at least.

But first I want to update the readme to reflect the changes, maybe add a CHANGES file, merge this PR and publish a v0.5.

Just wanted to update you on the progress.

@regexident regexident merged commit 358b0b8 into master Feb 9, 2021
@regexident regexident deleted the refactor branch February 9, 2021 08:55
Copy link

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

It looks like you implemented the hir visitor, but I have been trying to find that in this code (so that I can extract it as we talked about) and gave up lol.

@regexident
Copy link
Owner Author

@pksunkara I did, but it ended up being cleaner for my use-case to just explicitly drive the module walking via recursion, so I removed the visitor in d9ed9ae. 😄

I do still plan however to create a PR with it for cargo-up, as it should be quite useful for others. I just haven't gotten to it yet. 😇

@pksunkara
Copy link

🙏

I am just happy that someone else is finally using the ra_ap_* crates. I put quite a bit of work into getting that release process done.

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.

Using rust-analyzer
3 participants