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

LS: Add "fill struct fields" code action #6565

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

integraledelebesgue
Copy link
Collaborator

@integraledelebesgue integraledelebesgue commented Oct 31, 2024

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

@integraledelebesgue integraledelebesgue changed the base branch from spr/main/434c0351 to main October 31, 2024 17:28
@integraledelebesgue integraledelebesgue changed the base branch from main to spr/main/434c0351 October 31, 2024 17:28
@integraledelebesgue integraledelebesgue changed the base branch from spr/main/434c0351 to main October 31, 2024 17:32
@integraledelebesgue integraledelebesgue changed the base branch from main to spr/main/9fb1df2c October 31, 2024 17:32
Copy link
Collaborator Author

@integraledelebesgue integraledelebesgue 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 2 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @mkaput, @orizi, and @piotmag769)

a discussion (no related file):
I'm still working on this feature and e2e tests for it.


@integraledelebesgue integraledelebesgue changed the title Added "fill struct fields" code action LS: Add "fill struct fields" code action Oct 31, 2024
Copy link
Collaborator

@piotmag769 piotmag769 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 r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/ide/code_actions/mod.rs line 34 at r2 (raw file):

    );
    actions.extend(
        fill_struct_members::fill_struct_members(db, node.clone(), &params)

Shouldn't it be based on a diagnostic like in the loop above? This way you don't have to redetermine if it's needed.
Will require assigning error code to SemanticDiagnosticKind::MissingMember in SemanticDiagnosticKind::error_code

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 r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, 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.

Reviewed 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, and @piotmag769)

Copy link
Collaborator

@piotmag769 piotmag769 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 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, and @integraledelebesgue)


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 34 at r10 (raw file):

    let constructor_expr_id =
        db.lookup_expr_by_ptr(function_id, constructor_expr.stable_ptr().into()).ok()?;
    let constructor_semantic = match db.expr_semantic(function_id, constructor_expr_id) {

This is used much later, move it close to the usage pls


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 36 at r10 (raw file):

    let constructor_semantic = match db.expr_semantic(function_id, constructor_expr_id) {
        Expr::StructCtor(semantic) => semantic,
        _ => return None,

Prob worth logging an error here, it should never happen


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 91 at r10 (raw file):

        .collect::<Vec<_>>();

    let prefix = String::from(if !has_trailing_comma && last_important_element.is_some() {

Why don't declare it after the for loop with has_trailing_comma and last_important_element? This way you don't have to track where these vars are actually used

Copy link
Collaborator

@piotmag769 piotmag769 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 r9.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, and @integraledelebesgue)


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 75 at r10 (raw file):

    let struct_parent_module_id = concrete_struct_id.struct_id(db).parent_module(db);

    let arguments_to_complete = concrete_struct_members(db, concrete_struct_id)

This is a query, use it as one

Suggestion:

db.concrete_struct_members(concrete_struct_id)

@integraledelebesgue integraledelebesgue force-pushed the spr/main/0051578a branch 2 times, most recently from efe63a2 to 3f917bb Compare November 14, 2024 14:01
Copy link
Collaborator

@piotmag769 piotmag769 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 r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @integraledelebesgue)

Copy link
Collaborator Author

@integraledelebesgue integraledelebesgue 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, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @piotmag769)


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 36 at r10 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Prob worth logging an error here, it should never happen

Done.


crates/cairo-lang-language-server/src/ide/code_actions/fill_struct_fields.rs line 75 at r10 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This is a query, use it as one

Done.

Copy link
Collaborator

@piotmag769 piotmag769 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @Draggu)

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 r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @Draggu)

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 r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @Draggu)

@integraledelebesgue integraledelebesgue force-pushed the spr/main/0051578a branch 3 times, most recently from 07f5d86 to 79a7fea Compare November 14, 2024 17:07
@integraledelebesgue integraledelebesgue force-pushed the spr/main/0051578a branch 2 times, most recently from 8638f37 to 322fb00 Compare November 14, 2024 17:32
@integraledelebesgue integraledelebesgue changed the base branch from spr/main/9fb1df2c to main November 14, 2024 17:55
@integraledelebesgue integraledelebesgue added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit 89a00fe Nov 14, 2024
93 of 94 checks passed
@orizi orizi deleted the spr/main/0051578a branch November 28, 2024 13:20
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.

Provide intention to fill struct members
5 participants