starkware-libs / cairo

Cairo is the first Turing-complete language for creating provable programs for general computation.
Apache License 2.0
1.46k stars 450 forks source link

bug: LSP completion dialog should only recommend public members of a module #5660

Open misicnenad opened 1 month ago

misicnenad commented 1 month ago

Bug Report

Cairo version: 2.6.3

Current behavior:

When defining a module with some public members and trying to import said members, a completion dialog appears, but the language server includes non-public members in the dialog. Additionally, the lang. server includes non-members in the dialog (in this case ShapeGeometry).

Example below: image

Trying to import non-public members (and non-members) results in an error of course: image image image

Importing a public member works as expected: image

Expected behavior:

The completion dialog should only include actual module members and only those that can actually be imported.

Steps to reproduce:

Related code:

trait ShapeGeometry<T> {
    fn boundary(self: T) -> u64;
}

mod circle {
    use super::ShapeGeometry;

    #[derive(Drop)]
    pub struct Circle {
        pub radius: u64,
    }

    impl CircleGeometry of ShapeGeometry<Circle> {
        fn boundary(self: Circle) -> u64 {
            (2 * 314 * self.radius) / 100
        }
    }
}

use circle::

Other information:

Related issue: https://github.com/starkware-libs/cairo/issues/5664

Utilitycoder commented 1 month ago

Hi, I would love to work on this. I however need pointers to where this should be fixed.

mkaput commented 1 month ago

Task is yours. Thanks ♥️

The entry point for completion logic is here:

https://github.com/starkware-libs/cairo/blob/c6d602981962f759ff66c81ecfb75e2e24c66d66/crates/cairo-lang-language-server/src/ide/completion/mod.rs#L19-L59

This looks to be ::-completion, which is computed here:

https://github.com/starkware-libs/cairo/blob/c6d602981962f759ff66c81ecfb75e2e24c66d66/crates/cairo-lang-language-server/src/ide/completion/completions.rs#L115-L188

As you can see, this is basically just a banch of ifs for various AST items.

I guess you simply have to put appropriate filters here:

https://github.com/starkware-libs/cairo/blob/c6d602981962f759ff66c81ecfb75e2e24c66d66/crates/cairo-lang-language-server/src/ide/completion/completions.rs#L137-L148


BTW It would be awesome if you'd also contribute completion E2E tests, as you're touching this. There are none yet, but they should be pretty similar to hover test for example:

https://github.com/starkware-libs/cairo/blob/main/crates/cairo-lang-language-server/tests/e2e/hover.rs https://github.com/starkware-libs/cairo/tree/main/crates/cairo-lang-language-server/tests/test_data/hover

Would you like to make this part? I don't want to enforce this on you. 😃

mkaput commented 2 weeks ago

@Utilitycoder hey, how's it going? Would you like some assistance from my side in this area?

Utilitycoder commented 1 week ago

@mkaput I have been sick but slowly getting back to optimum health. I will take a look and let you know If I'd be able to proceed with it.