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

feat(lsp): show rust doc when hovering over things #5225

Merged
merged 10 commits into from
Mar 18, 2024

Conversation

0xLucqs
Copy link
Contributor

@0xLucqs 0xLucqs commented Mar 8, 2024

what's displayed for then_some

Screenshot 2024-03-08 at 16 19 42

And for check_ecdsa_signature (if it started with /// instead of //)
Screenshot 2024-03-08 at 16 32 03


This change is Reviewable

@0xLucqs 0xLucqs requested review from mkaput and orizi March 8, 2024 14:38
@0xLucqs 0xLucqs force-pushed the lucas/main branch 3 times, most recently from 05da1c9 to b5336a9 Compare March 8, 2024 15:32
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @mkaput)


crates/cairo-lang-language-server/src/lib.rs line 1686 at r3 (raw file):

    }
}

doc

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)


crates/cairo-lang-language-server/src/lib.rs line 1686 at r3 (raw file):

Previously, orizi wrote…

doc

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @mkaput)


crates/cairo-lang-language-server/src/lib.rs line 1700 at r4 (raw file):

pub fn get_definition_location(
    db: &RootDatabase,
    uri: Url,

Can't you accept file id instead of uri? You seem to extract it twice.

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)


crates/cairo-lang-language-server/src/lib.rs line 1700 at r4 (raw file):

Previously, orizi wrote…

Can't you accept file id instead of uri? You seem to extract it twice.

Done.

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Also i'd love to get read of the other hints because i don't think they provide any useful information but maybe i'm missing something ? I think this approach could be enough for everything, even for imports etc

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @LucasLvy and @orizi)


crates/cairo-lang-language-server/src/lib.rs line 1483 at r5 (raw file):

            // add new line to have a proper formatting.
            doc += "\n";
        }

I think the logic for obtaining documentation should be a query in the semantic model in the compiler rather then polluting LS logic

Code quote:

    // Get its parent to get the trivia.
    if let Some(parent_node) = node.parent() {
        let mut is_cairo_string = false;
        // Get the trivia and remove the node text.
        let lines = if let Some(split) = parent_node.get_text(db.upcast()).rsplit_once('\n') {
            split.0.to_string()
        } else {
            "".to_string()
        };
        let mut doc = "".to_string();
        for line in lines.lines() {
            // Skip everything else than doc
            if !line.trim_start().starts_with("///") {
                continue;
            }
            // Remove the leading whitespaces and `///`
            let line = line.trim().trim_start_matches("///").trim_start().to_string();
            // If we reach ``` it means that we either begin a code section or end a code section.
            if line.starts_with("```") {
                // If is_cairo_string is true it means that we end the code block so append the code
                // in a cairo formatted string.
                hints.push(if is_cairo_string {
                    MarkedString::from_language_code("cairo".to_owned(), doc)
                } else {
                    // else append a regular string.
                    MarkedString::from_markdown(doc)
                });
                // Switch the variable to properly format the following block.
                is_cairo_string = !is_cairo_string;
                // reset the string to start the next block.
                doc = "".to_string();
                continue;
            }
            // Append the current block string.
            doc += line.as_str();
            // add new line to have a proper formatting.
            doc += "\n";
        }

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @LucasLvy and @mkaput)


crates/cairo-lang-language-server/src/lib.rs line 869 at r5 (raw file):

            let file_uri = params.text_document_position_params.text_document.uri;
            let file_id = file(db, file_uri.clone());
            let position = params.text_document_position_params.position;

Suggestion:

            let file_id = file(db, file_uri);
            let position = params.text_document_position_params.position;

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @LucasLvy)

@0xLucqs 0xLucqs force-pushed the lucas/main branch 2 times, most recently from 46ed8a7 to 894a38c Compare March 11, 2024 19:28
@0xLucqs 0xLucqs requested review from orizi and mkaput March 11, 2024 19:28
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r6, all commit messages.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @LucasLvy and @orizi)


crates/cairo-lang-defs/src/db.rs line 107 at r6 (raw file):

    fn documentation(&self, item_id: LookupItemId) -> String;
    /// Gets the the definition + documentation above.
    fn documentation_with_definition(&self, item_id: LookupItemId) -> (String, String);

Querying documentation and definition as separate queries would be same thing in terms of performance, and that setup would allow getting definition without even touching documentation logic. Could you drop the documentation part from this query?


crates/cairo-lang-defs/src/db.rs line 107 at r6 (raw file):

    fn documentation(&self, item_id: LookupItemId) -> String;
    /// Gets the the definition + documentation above.
    fn documentation_with_definition(&self, item_id: LookupItemId) -> (String, String);

I think we'd like to have some unit-style tests for these queries, that will also focus on edge cases, aren't we @orizi?


crates/cairo-lang-defs/src/db.rs line 309 at r6 (raw file):

        item_id.stable_location(db).syntax_node(db).get_text_without_trivia(db.upcast());
    // Remove the function implementation if it's a function.
    let definition = if let Some(index) = definition.find('{') {

The following snippets bust this implementation as I understand things:

const FOO = { 52 };
#[macro(x = { 2 })]
fn foo() {}

You must traverse the AST here, and cut out function/module bodies subtrees manually, to make this correct.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 288 at r6 (raw file):

}

fn documentation(db: &dyn DefsGroup, item_id: LookupItemId) -> String {

doc

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @mkaput and @orizi)


crates/cairo-lang-defs/src/db.rs line 309 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

The following snippets bust this implementation as I understand things:

const FOO = { 52 };
#[macro(x = { 2 })]
fn foo() {}

You must traverse the AST here, and cut out function/module bodies subtrees manually, to make this correct.

2nd one is easy to fix, can have something for the constant case also. Never really worked with the tree so i'm not sure how "better" it is but i have a feeling that i'd need to have one case per node type which wouldn't be so much better. WDYT ?

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @LucasLvy and @orizi)


crates/cairo-lang-defs/src/db.rs line 309 at r6 (raw file):

Previously, LucasLvy (Lucas @ StarkWare) wrote…

2nd one is easy to fix, can have something for the constant case also. Never really worked with the tree so i'm not sure how "better" it is but i have a feeling that i'd need to have one case per node type which wouldn't be so much better. WDYT ?

let's discuss this offline

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @LucasLvy)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 106 at r7 (raw file):

    /// Gets the documentation above an item definition.
    fn get_item_documentation(&self, item_id: LookupItemId) -> String;

Suggestion:

Option<String>

crates/cairo-lang-defs/src/db.rs line 108 at r7 (raw file):

    fn get_item_documentation(&self, item_id: LookupItemId) -> String;
    /// Gets the the definition + documentation above.
    fn get_item_definition(&self, item_id: LookupItemId) -> String;

perhaps let's use Option to use None as "this item does not have definition"?

Suggestion:

Option<String>

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mkaput)


crates/cairo-lang-defs/src/db.rs line 108 at r7 (raw file):

Previously, mkaput (Marek Kaput) wrote…

perhaps let's use Option to use None as "this item does not have definition"?

I don't see a good reason to do that, if there's no documentation it'll display

definition
---

which is fine, if we return None we then either need to propagate (which we don't want because we still want to display the definition, or use if let Some which is equivalent to returning the empty string imo

Copy link
Contributor Author

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mkaput and @orizi)


crates/cairo-lang-defs/src/db.rs line 288 at r6 (raw file):

Previously, orizi wrote…

doc

the doc is in the trait. Should i copy paste it here also?

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @LucasLvy and @orizi)


crates/cairo-lang-defs/src/db.rs line 108 at r7 (raw file):

Previously, LucasLvy (Lucas @ StarkWare) wrote…

I don't see a good reason to do that, if there's no documentation it'll display

definition
---

which is fine, if we return None we then either need to propagate (which we don't want because we still want to display the definition, or use if let Some which is equivalent to returning the empty string imo

Technically, there's a difference between

fn foo() {}

and

///
fn foo() {}

IMO The compiler code should differentiate these (you might not want to display empty pop-up if there is no documentation hypothetically), while LS can simply do .unwrap_or_default(). Though to my taste, I'd skip the --- separator in this scenario.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 288 at r6 (raw file):

Previously, LucasLvy (Lucas @ StarkWare) wrote…

the doc is in the trait. Should i copy paste it here also?

even just a doc pointing to the trait marking it as its helper.


crates/cairo-lang-defs/src/db.rs line 304 at r7 (raw file):

        .join("\n")
}
fn get_item_definition(db: &dyn DefsGroup, item_id: LookupItemId) -> String {

same here.
and newline.

Suggestion:

fn get_item_definition(db: &dyn DefsGroup, item_id: LookupItemId) -> String {

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @mkaput)


crates/cairo-lang-defs/src/db.rs line 107 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Leaving decision to @orizi

Fine with a Todo here, but should be done soon

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy)

@0xLucqs 0xLucqs requested a review from orizi March 14, 2024 16:32
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 109 at r10 (raw file):

    fn get_item_documentation(&self, item_id: LookupItemId) -> Option<String>;
    // TODO: test that
    /// Gets the the definition of an item.

Suggestion:

    // TODO(assignee): Add tests.
    /// Gets the documentation above an item definition.
    fn get_item_documentation(&self, item_id: LookupItemId) -> Option<String>;
    // TODO(assignee): Add tests.
    /// Gets the the definition of an item.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 105 at r11 (raw file):

    fn module_dir(&self, module_id: ModuleId) -> Maybe<Directory>;

    // TODO(Marek): test that

nit

Suggestion:

mkaput

crates/cairo-lang-defs/src/db.rs line 108 at r11 (raw file):

    /// Gets the documentation above an item definition.
    fn get_item_documentation(&self, item_id: LookupItemId) -> Option<String>;
    // TODO(Marek): test that

nit

Suggestion:

mkaput

crates/cairo-lang-language-server/src/lib.rs line 1443 at r11 (raw file):

}

// TODO: test this

nit, this particular function won't be probably tested per se, but we'd rather focus on testing LSP commands

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy)


crates/cairo-lang-defs/src/db.rs line 109 at r10 (raw file):

    fn get_item_documentation(&self, item_id: LookupItemId) -> Option<String>;
    // TODO: test that
    /// Gets the the definition of an item.

Not resolved yet.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LucasLvy and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @LucasLvy)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @LucasLvy)

@0xLucqs 0xLucqs added this pull request to the merge queue Mar 18, 2024
Merged via the queue into starkware-libs:main with commit ba74ab1 Mar 18, 2024
43 checks passed
shramee pushed a commit to shramee/cairo that referenced this pull request Sep 17, 2024
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