General discussion about rust-analyzer as a driver about marker HOT 12 OPEN

Nice! Though unfortunately there were no movement there recently, per the zulip thread.

True, on the other hand, it sounds like their goals partially align with the one from this repo. It's good to know that there are not two projects working on the same thing.

I think a close to rustc api + option to static linking instead of dynamic linking will solve the performance problem.

That's actually true, static linking could become an option again. For user crates, I discarded that because the driver would need to be recompiled and all the related functions. But for basic lints in the projects themselves, this could be a valid option 👍. Though I guess there will still be more overhead due to type conversion etc. We'll see, adding benchmarks is somewhere on the roadmap, but still very far off ^^

Will look forward to it.

I'll ping you in that PR. Currently, I'm trying to reduce or at least clean up the boilerplate code.

That's fine! I may miss the zulip mentions, but will catch up github mentions.

Awesome, there doesn't seem to be any conversation on Zulip. At one point, it would be nice to have our own channel, but for now, I prefer GH 🙃

from marker.

My current idea was to make a mixture. Items and bodies are lazy and only get converted when actually requested. A driver would then still need to convert an entire body to send it to a lint crate, but not the entire AST.

That would be great. I think r-a currently rebuilds hir for the function under edit and type checks it from zero, and it is fine.

Btw, do you know how RA implements tests? That might also be good to consider.

For diagnostics, they are like rustc's ui tests, but the code is inside string in a #[test] function. Once I make spans working I can use them.

You are welcome to create a draft PR. I would like to see if it required changes in the API and how it's connected. This can also still wait if you don't think it would be of value :)

I will try to polish it and add spans and then create a PR in my fork for discussion on the code.

from marker.

This is definitely planned. The liner_api crate which will contain the AST (like) representation of the code is based on the Rust Reference. Only linter_driver_rustc lints with rustc internals and translates them to liner_api types. Which will then be passed over to the specific lint crates.

I know that rust-analyzer had a similar plan. However, that has stalled a bit AFAIK. I wanted to ask them on the status, but haven't gotten to that yet. 😅

And how we can help in that direction?

This project is currently a bit chaotic, since the infrastructure etc hasn't been fully designed yet. So there are a lot of loose ends which are not yet documented well. I plan to write a small contribution guide and issues once the base infrastructure is done (Planned for the end of the month). Until then, you could help with code reviews (Currently, I make one larger PR per week or so). Or try to do some coding, but that might cause a lot of merge conflict until the basic structure is in place.

If you don't mind me asking, what is your background? Have you worked on rust-analyzer? And what do you enjoy doing in Rust? 🙃

from marker.

Ah, so I was completely wrong, and the parts that are actually needed for implementing another driver (linting_adapter and linting_api) work in stable. I did some experiments and it looks very promising.

I know that rust-analyzer had a similar plan. However, that has stalled a bit AFAIK. I wanted to ask them on the status, but haven't gotten to that yet.

What plan do you mean? I only know about shared parser library, which would make the ast equal between rustc and r-a, but it is no where near complete AFAIK (and it wouldn't help in HIR part, at least immediately).

If you don't mind me asking, what is your background? Have you worked on rust-analyzer? And what do you enjoy doing in Rust?

I made some contributions in r-a, mostly around const generic / const eval and LSIF (this is a format for dumping intellisense data about a code repository) and I'm mostly familiar with r-a internals. The duplicate efforts between r-a and rustc makes me sad, so I'm really interested in librarification projects.

And I see librarification potential here. Although the original goal of this project is stabilizing the lint implementer side, stabilizing (or just detaching from rustc internals, r-a can keep up with breaking changes) the driver side would enable r-a to reuse the lints from rustc, clippy, and even users.

You may ask: Isn't r-a already showing warnings from clippy and rustc? It is, but r-a's own lints are preferred, because they can be emitted in milliseconds, so they can be showed on each keystroke. There are already some duplicate lints and diagnostics from clippy and rustc, reimplemented in r-a, for this reason. And the whole thing is duplicate effort. Everything should be implemented and tested and bug fixed twice, there are different wordings and spans (the r-a one's is usually poorer), r-a lints can't be #[allow()] ed, ... With this project, I hope r-a will become able to share its own infrastructure with (some of) clippy and rustc lints, achieving quality of clippy lints with performance of r-a lints.

This project is currently...

Now that it is working on stable, I will try to make r-a a driver (I know that currently it isn't possible to make useful lints with this, I just want to see how things are going) and will provide feedback for what is needed to make it working, though you may want to ignore them until the dust settles.

from marker.

Ah, so I was completely wrong, and the parts that are actually needed for implementing another driver (linting_adapter and linting_api) work in stable. I did some experiments and it looks very promising.

Correct! You might want to take a look at: docs/internal to get a rough overview.

What plan do you mean?

First, on the rust-analyzer side, we are in the (slow) process of actually designing the "compiler API for 2nd and 3rd party consumers": HIR 2.0 for rust-analyzer - HackMD 15. Not much to see there at the moment.

Source

As said, I planned to reach out to them for a longer, but other things have taken precedence 😅 . Now it's pretty high on my todo list

And I see librarification potential here. Although the original goal of this project is stabilizing the lint implementer side, stabilizing (or just detaching from rustc internals, r-a can keep up with breaking changes) the driver side would enable r-a to reuse the lints from rustc, clippy, and even users.

Currently, there is no plan (at least from my side) to migrate clippy/rustc lints over to the stable interface. The goal of rustc is to be fast and create correct code. Clippy also uses a bunch of rustc internal information, which might not be available as part of the stable API. My goal with the AST design is to have a simple and usable format, even if it costs more when it comes to performance. So, IMO, the result will ideally be a new tool and not a replacement.

Isn't r-a already showing warnings from clippy and rustc?

Yes, it's using the JSON output of rustc warnings for that. Ideally, rust-analyzer can be attached as it's own driver. As you suggested. Then we can reuse the same lint crates :)

Now that it is working on stable, I will try to make r-a a driver (I know that currently it isn't possible to make useful lints with this, I just want to see how things are going) and will provide feedback for what is needed to make it working, though you may want to ignore them until the dust settles.

It would be super outstanding to have the basic skeleton to use rust-analyzer as a driver! Currently, I'm redesigning the adapter/lint-crate -> driver api. So, I would recommend not doing more than necessary until that PR. Or maybe waiting on it completely. You're obviously welcome to give feedback on the changes. 🙃

from marker.

Now it's pretty high on my todo list

Done: https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Status.20.22HIR.202.2E0.20for.20rust-analyzer.22/near/292562942

from marker.

I hope it was okay, that I've mentioned you in the discussion. I don't want to put pressure on you, just thought that it's worth mentioning. You don't have to do it if you don't have the time or interest etc 🙃

from marker.

party consumers": HIR 2.0 for rust-analyzer - HackMD 15. Not much to see there at the moment.

Nice! Though unfortunately there were no movement there recently, per the zulip thread.

Currently, there is no plan (at least from my side) to migrate clippy/rustc lints over to the stable interface....

I think a close to rustc api + option to static linking instead of dynamic linking will solve the performance problem. A convenient api can exist on top of it, maybe even in another crate (with heavy macros, ast matching, ...), and rustc api isn't that bad and low level either. And there is no need to move all rustc/clippy lints to the stable interface over night. They can support both interfaces forever, stable api for simple lints and unstable api for super performance sensitive and/or internals dependent lints.

I would recommend not doing more than necessary until that PR. Or maybe waiting on it completely.

Will look forward to it.

I hope it was okay, that I've mentioned you in the discussion. I don't want to put pressure on you, just thought that it's worth mentioning. You don't have to do it if you don't have the time or interest etc

That's fine! I may miss the zulip mentions, but will catch up github mentions.

from marker.

Hi @xFrednet

I found some more time on this, and made something. It doesn't support spans, so it isn't usable yet, but print results looks good. But I don't know how viable it is performance wise.

My first problem is, currently, driver can only call process_krate, which will traverse the whole AST and call methods of LintPass. This works for rustc, but not for r-a. Because r-a wants to run lints on each key stroke, but traversing the ast can take 10s of seconds. Current lints in r-a subscribe to changed nodes, and since changed nodes are O(depth_of_ast) which is small, performance is good. I think by providing a way for driver to directly call LintPass methods, this problem will be solved.

My second problem, which I think is more serious, is that currently, the whole ast should be translated to the linter_api representation before start. But it has the same problem as above. Current lints in r-a don't have this problem, because r-a update it's ast on each keystroke and can give pointer to new nodes to lints. Either I should find a way to update ast with changes (which I don't think is feasible) or linter_api should represent the ast in a lazy way. That is, each node can be converted O(1) and has links (id, callback, pointer, ...) to its children and parents. And lint can ask for children and parent nodes if it needs them. Presumably each lint do an O(1) work on each method of LintPass, so it needs O(1) ast nodes, so r-a problem will be solved. (But it might make rustc driver slower by a constant factor).

What do you think about those problems? I don't want to hijack this project, so feel free to deprioritize / ignore them.

from marker.

Current lints in r-a subscribe to changed nodes

I thought about enabling lint passes to mark which nodes are actually interesting. However, I feel like that would add a bunch of complexity with little benefit.

It's good to know that RA works like that :)

I think by providing a way for driver to directly call LintPass methods, this problem will be solved.

That's on my todo list and what I planned for something like RA. For now, I haven't implemented that, since the actual design of AST nodes is not set in stone (rust-marker/design#13).

Either I should find a way to update ast with changes (which I don't think is feasible) or linter_api should represent the ast in a lazy way.

This is something I was also contemplating when looking at how lint crates will be loaded. The problem is that lazy loading will add a bunch of overhead for each lookup, in comparison to just following a reference. My current idea was to make a mixture. Items and bodies are lazy and only get converted when actually requested. A driver would then still need to convert an entire body to send it to a lint crate, but not the entire AST. Depending on performance, we could also narrow the scope further.

I'm not sure if this is ideal, but some lint implementation might depend on the assumption that when a node is visited, all children will also be visited. Clippy has some of these lints. Guaranteeing this for all bodies could maybe be enough.

With the new AST nodes, I also try to make some node properties lazy as well. It might be an option to convert multiple notes if the properties are mostly lazy.

It's good that you're pointing this out, as its something worth considering.

What do you think about those problems? I don't want to hijack this project, so feel free to deprioritize / ignore them.

The API should ideally have a model that works for compilers and IDEs. I see compilers as the main focus, but dream of supporting RA (and other IDEs) as well. I appreciate the effort and feedback you give.

from marker.

I found some more time on this, and made something. It doesn't support spans, so it isn't usable yet, but print results looks good. But I don't know how viable it is performance wise.

Btw, do you know how RA implements tests? That might also be good to consider.

You are welcome to create a draft PR. I would like to see if it required changes in the API and how it's connected. This can also still wait if you don't think it would be of value :)

from marker.

For future reference: HKalbasi/rust-analyzer#1 ❤️

from marker.