External lint api #1
Conversation
|
It seems like the needed code is actually quite reasonable. I'll have a closer look over the week or the latest on the weekend. Thank you for putting this together ❤️ Have you already noticed some parts of the API design which doesn't work too well for RA? I saw some allocations, which are never cleaned up. I thought about limiting the lifetime of expressions and nodes to not be the full |
There was a problem hiding this comment.
Have you already noticed some parts of the API design which doesn't work too well for RA?
It was better than what I expected. I added some comments on code about things that can be better.
I saw some allocations, which are never cleaned up. I thought about limiting the lifetime of expressions and nodes to not be the full 'ast thinking
I think it is just a bumpalo away, and no change is needed in linter_api.
| node: &SourceFile, | ||
| config: &DiagnosticsConfig, | ||
| ) { | ||
| std::env::set_var("LINTER_LINT_CRATES", &config.external_lints); |
There was a problem hiding this comment.
Here I should add an env variable manually, which is not very ideal. I think an api which get string directly in addition to new_from_env helps.
There was a problem hiding this comment.
Agreed, I only added one interface, while figuring out how lint crates should be loaded. After an evaluation and the conclusion that dynamic libraries will be the way, at least for the start. It's safe to add such a function. A PR which adds that would be highly appreciated, but an issue would also already be helpful 🙃
| struct A { a: &'static str } | ||
|
|
||
| mod foo { | ||
| static X: u32 = 5; |
There was a problem hiding this comment.
This one is not linted, which I think is because process_krate doesn't check nested items.
There was a problem hiding this comment.
Interesting, I believe that modules are not fully visited. In general, there are many items missing. Thank you for the report
|
|
||
| impl<'ast> DriverContext<'ast> for RADriver<'ast> { | ||
| fn emit_lint( | ||
| &self, |
There was a problem hiding this comment.
Is it intentionally &self? By changing it to &mut self I can drop those RefCell fields in RADriver.
There was a problem hiding this comment.
Good question, this is mostly the result of evolving source code. Just using &self was also inspired by rustc way to implement these traits.
The main question here is how concurrency should be handled. By only using &self the adapter passes responsibility to the driver. The driver could potentially check multiple crates at once. On the other hand, we could just use RefCell in the adapter itself 🤔
| crate_id: CrateId, | ||
| ) -> Option<ItemType<'ast>> { | ||
| let cx = &driver.ast_context.get().unwrap(); | ||
| let item_id = ItemId::new(crate_id, driver.id_map.borrow().len() as u32); |
There was a problem hiding this comment.
At ast level r-a doesn't have id for items, so I made it in this way. Is it possible / better to use &'ast Item instead of ItemId?
There was a problem hiding this comment.
What does ra use to identify items then?
I changed the ItemId to just be a u64 which can be used by the driver however they want. In theory, a pointer address can also be stored in it. The problem with this solution, is that it requires the item to already be created, which goes a bit against lazy evaluation, which was also wished for if I'm correct.
Awesome, this has already helped me a lot!
When will we then clear the memory, though 🤔. The current setup basically expects that any object with the |
cc @xFrednet
This is just a proof of concept and many things are done poorly.
Actually running it requires a clone of
rust_lintingrepo as a sibling directory of this repo.