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

[just for feedback] Flycheck individual function #11342

Closed
wants to merge 1 commit into from

Conversation

HKalbasi
Copy link
Member

This is an attempt to display rustc errors while editing code. We can't run cargo check on whole project because it will burn the cpu and take some seconds to finish (is this the problem?), But we can check the under edit function by including it and header of what it uses.

For example, we want to check foo function in this code:

//- minicore: option, clone, derive
enum Gav {
    Var1(Option<Baz>),
    Var2,
}
struct Baz(i32);
#[derive(Clone)]
struct Bar;
#[derive(Clone)]
struct Foo(i32, Bar, Bar);

fn bar(x: Gav) -> i32 {
    match x {
        Var1 => Baz(34).0,
        Var2 => 43,
    }
}

fn foo_to_gav(x: Foo) -> Gav {
    todo!()
}

fn fo$0o(x: core::option::Option<Foo>) -> i32 {
    if let Some(x) = x {
        bar(foo_to_gav(x.clone()))
    } else {
        5
    }
}

This PR contains a code that generate this code:

struct _ra_S1;struct _ra_S2(i32,_ra_S1,_ra_S1,);pub enum _ra_E0<T>{None,Some(T,),}struct _ra_S0(i32,);enum _ra_E1{Var1(_ra_E0<_ra_S0>,),Var2,}fn _ra_f0(x: _ra_E1) -> i32{loop{}}fn _ra_f1(x: _ra_S2) -> _ra_E1{loop{}}pub trait _ra_T1
where
    Self: Sized,{pub fn clone(&self) -> Self{loop{}}}impl _ra_T1 for _ra_S2 {}
pub(crate) use _ra_S2 as Foo;pub(crate) use _ra_f1 as foo_to_gav;pub(crate) use _ra_E0::Some as Some;pub(crate) use _ra_f0 as bar;pub mod core {pub mod option {pub(crate) use super::super::_ra_E0 as Option;}}
fn main() { let _ = foo; }
fn foo(x: core::option::Option<Foo>) -> i32 {
    if let Some(x) = x {
        bar(foo_to_gav(x.clone()))
    } else {
        5
    }
}

The code is no where near complete, Its only a proof-of-concept and I just wanted to make sure it works. We can send this code to rustc (or clippy or ...), get the result, shift the span to match original input and show them (this is not implemented in this PR, and I don't exactly know how to do it)

Problems

  • Macros inside of target function: If we expand them, finding original spans will become hard (currently target function is included as is, so finding original span is trivial). If we keep them, it is against the point and slows the compile process (specially with proc macro). We can ignore function with macros in start.
  • Detecting under edit function: Can we find the last function that invalidated the cache? This part is not clear to me but I believe it should be possible.
  • Names: Diagnostics will contain something like "binary operation == cannot be applied to type _ra_E0<_ra_E0<_ra_S2>>" and we should restore original names.
  • Privacy: It brings everything in the visibility of the target function. We can emit native diagnostics for privacy errors easily, but it still can lead to inapplicable suggestions and such. If it becomes too bad in practice, we should preserve the structure of modules and crates.
  • False positive due wrong analysis: Mistakes in resolving functions and traits will lead to super wrong errors. We may want to disable in functions that analysis is not certain.
  • Something that I missed and will render it completely unusable in real world.

Any comments? Is it in the right direction? It is somehow hacky and we may want to do it in a more reasonable way, for example by calling special rustc apis designed for this propose.

@HKalbasi HKalbasi marked this pull request as draft January 25, 2022 12:51
@flodiebold
Copy link
Member

I think while it's a nice idea, there are several problems with this approach.

  1. Most importantly, we do not want to build so much on top of flycheck. Flycheck is a stopgap while we don't have enough diagnostics implemented natively. We want to move towards having more native diagnostics and sharing more code with rustc, and this would be a lot of complicated code to make this work that does not contribute towards that end goal.
  2. IMO the idea of trying to determine the "last edited" function isn't quite right. I think what we really want is to only compute diagnostics for the text ranges that the user is actually looking at, maybe expanding to the whole files. Ideally that'd work by the client requesting diagnostics explicitly, though alternatively maybe it could tell us the ranges it's currently focusing on separately.
  3. "We can't run cargo check on whole project because it will burn the cpu and take some seconds to finish (is this the problem?)" -- I'm not sure what you mean by "the problem". We do run cargo check on the whole project. We can't run it without saving because we don't have a way to provide rustc with the non-saved file contents and we don't want to use nightly-only APIs.

@HKalbasi
Copy link
Member Author

Most importantly, we do not want to build so much on top of flycheck. Flycheck is a stopgap while we don't have enough diagnostics implemented natively. We want to move towards having more native diagnostics and sharing more code with rustc, and this would be a lot of complicated code to make this work that does not contribute towards that end goal.

So we want to drop flycheck at some point? There are dozens of lint in rustc and hundereds in clippy, implementing all of them natively is duplicate effort and not a good idea IMO. I can see a shared lint library as a final solution, but a very very far one?

IMO the idea of trying to determine the "last edited" function isn't quite right. I think what we really want is to only compute diagnostics for the text ranges that the user is actually looking at, maybe expanding to the whole files. Ideally that'd work by the client requesting diagnostics explicitly, though alternatively maybe it could tell us the ranges it's currently focusing on separately.

Nothing prevent us from running it on all functions of a file. Running rustc on the example above takes 100ms, but running it on 10 copies of it still takes 100ms and 100 copies of it takes 300ms. So we can run it on all functions of a file on open in ~500ms, and all changed functions in ~100ms, showing diagnostics in every visible function.

We do run cargo check on the whole project

I mean, we don't do it on each keystroke. That sentence is just saying that there is a problem with current flycheck and this one is better.

@bjorn3
Copy link
Member

bjorn3 commented Jan 25, 2022

So we want to drop flycheck at some point? There are dozens of lint in rustc and hundereds in clippy, implementing all of them natively is duplicate effort and not a good idea IMO. I can see a shared lint library as a final solution, but a very very far one?

I think the idea is that eventually librarification of rustc will allow sharing most code, including lints, between rustc and rust-analyzer, with only the glue code being different to allow for eager batch compilation (rustc) vs incremental on-demand analysis (rust-analyzer).

@flodiebold
Copy link
Member

See also #3107 for some more context. Performance is not the main reason that we don't run check on type, the lack of a VFS that can be shared with rustc is.

@HKalbasi
Copy link
Member Author

I think the idea is that eventually librarification of rustc will allow sharing most code, including lints, between rustc and rust-analyzer, with only the glue code being different to allow for eager batch compilation (rustc) vs incremental on-demand analysis (rust-analyzer).

I know the plan, but progress on librarification is somehow slow. And a shared lint library is probably in the last steps. It depends on shared ast, shared types, shared hir, ... so I think it takes years.

See also #3107 for some more context. Performance is not the main reason that we don't run check on type, the lack of a VFS that can be shared with rustc is.

hmm, I tried auto save as suggested in that thread, it turned on laptop fans and some errors take ~5s to appear, but it wasn't as bad as I expected. I thought running it on save is some kind of throttling for saving system resources. Flycheck + autosave is not perfect, but it works, and this one is not flawless either, so we can close it.

@HKalbasi HKalbasi closed this Jan 26, 2022
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.

3 participants