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

Provide intention to fill struct members #5945

Closed
mkaput opened this issue Jul 2, 2024 · 15 comments · Fixed by #6565
Closed

Provide intention to fill struct members #5945

mkaput opened this issue Jul 2, 2024 · 15 comments · Fixed by #6565
Assignees
Labels
help wanted Extra attention is needed

Comments

@mkaput
Copy link
Member

mkaput commented Jul 2, 2024

Implement the following intention in CairoLS:

Image

@mkaput mkaput added this to CairoLS Jul 2, 2024
@mkaput mkaput converted this from a draft issue Jul 2, 2024
@mkaput mkaput added help wanted Extra attention is needed ide labels Jul 2, 2024
@the-first-elder
Copy link

Hi @mkaput can you provide a link to this on the codebase

@mkaput
Copy link
Member Author

mkaput commented Jul 18, 2024

Hey! Here's the starting point:

/// Generate code actions for a given diagnostic.
///
/// # Arguments
///
/// * `db` - A reference to the Salsa database.
/// * `node` - The syntax node where the diagnostic is located.
/// * `diagnostic` - The diagnostic for which to generate code actions.
/// * `params` - The parameters for the code action request.
///
/// # Returns
///
/// A vector of [`CodeAction`] objects that can be applied to resolve the diagnostic.
fn get_code_actions_for_diagnostic(
db: &AnalysisDatabase,
node: &SyntaxNode,
diagnostic: &Diagnostic,
params: &CodeActionParams,
) -> Vec<CodeAction> {
let code = match &diagnostic.code {
Some(NumberOrString::String(code)) => code,
Some(NumberOrString::Number(code)) => {
debug!("diagnostic code is not a string: `{code}`");
return vec![];
}
None => {
debug!("diagnostic code is missing");
return vec![];
}
};
match code.as_str() {
"E0001" => {
vec![rename_unused_variable::rename_unused_variable(
db,
node,
diagnostic.clone(),
params.text_document.uri.clone(),
)]
}
code => {
debug!("no code actions for diagnostic code: {code}");
vec![]
}
}
}

You can follow the implementation of unused variable intention as the hooking point. First, you need to assign an error code to this particular diagnostic; you can grep for it by error message.

You will need to get information about struct members. This is provided in the SemanticGroup (this trait-Salsa group is implemented by AnalysisDatabase) by this query:

/// Returns the members of a struct.
#[salsa::invoke(items::structure::struct_members)]
fn struct_members(
&self,
struct_id: StructId,
) -> Maybe<OrderedHashMap<SmolStr, semantic::Member>>;

@mkaput mkaput moved this from Backlog to In Progress in CairoLS Jul 18, 2024
@the-first-elder
Copy link

the-first-elder commented Jul 22, 2024

Hi @mkaput I'm having issues getting the struct_Id, also there is no fix struct field error, i created an error code for missing members

@piotmag769
Copy link
Collaborator

@the-first-elder

there is no fix struct field error, i created an error code for missing members

That's good, that's what was supposed to be done!

I'm having issues getting the struct_Id

You need to get the StructId from the SyntaxNode when you already recognised that the MissingMember diagnostic was returned for the node. Since you can deduct (from the diagnostic) that the SyntaxNode contains struct expression, you need to find an expr node of appropriate kind (probably SyntaxKind::ExprStructCtorCal, but not sure about that). Checking the code from dot_completions may be helpful. Then when you get the type, do this:

let (_, long_ty) = peel_snapshots(db, ty);
if let TypeLongId::Concrete(ConcreteTypeId::Struct(concrete_struct_id)) = long_ty {
db.concrete_struct_members(concrete_struct_id).ok()?.into_iter().for_each(
|(name, member)| {

and you have the member names 🙂

You can check this PR, maybe it will help you with implementation

@mkaput
Copy link
Member Author

mkaput commented Aug 2, 2024

@the-first-elder any progress? how can we help?

@the-first-elder
Copy link

This is what i have currently done, I do need some guidance in getting it right.

pub fn fill_struct_fields(
    db: &AnalysisDatabase,
    node: &SyntaxNode,
    diagnostic: Diagnostic,
    uri: Url,
) -> CodeAction {
    // Resolve the StructId from the SyntaxNode
    let struct_id = resolve_struct_id(db, node).expect("Failed to resolve StructId");

    let members = get_struct_members(db.upcast(), struct_id)
    .expect("Failed to extract struct members");


    // Create the text edit
    let text_edit = TextEdit {
        range: diagnostic.range,
        new_text: format!("{{ {} }}", generate_default_values(&members)),
    };

    CodeAction {
        title: "Fill missing struct fields".to_string(),
        edit: Some(WorkspaceEdit {
            changes: Some(HashMap::from([(uri, vec![text_edit])])),
            document_changes: None,
            change_annotations: None,
        }),
        diagnostics: Some(vec![diagnostic]),
        ..Default::default()
    }
}

fn resolve_struct_id(db: &AnalysisDatabase, node: &SyntaxNode) -> Option<StructId> {
    let syntax_db = db.upcast();

    if node.kind(syntax_db) == SyntaxKind::ExprStructCtorCall {

        let lookup_item_id = db.find_lookup_item(&node)?;
        let function_id = lookup_item_id.function_with_body()?;
        let expr_id = db.lookup_expr_by_ptr(function_id, node.stable_ptr()).ok()?;
        let semantic_expr = db.expr_semantic(function_id, expr_id);

        let ty = semantic_expr.ty();
        if ty.is_missing(db) {
            return None;
        }

        let (_, long_ty) = peel_snapshots(db, ty);
        if let TypeLongId::Concrete(ConcreteTypeId::Struct(concrete_struct_id)) = long_ty {
            return Some(concrete_struct_id.struct_id(db));
        }
    }

    None
}

fn get_struct_members(db: &dyn SemanticGroup, struct_id: StructId) -> Maybe<OrderedHashMap<SmolStr, Member>> {
    db.struct_members(struct_id)
}


fn generate_default_values(members: &OrderedHashMap<SmolStr, Member>) -> String {
    members.iter().map(|(name, _)| format!("{}: default_value", name)).collect::<Vec<_>>().join(", ")
}

@piotmag769
Copy link
Collaborator

@the-first-elder looks like a solid work! Have you tried if it works? You can do it via building the LS from source and setting

{    
    "cairo1.languageServerPath": "/Users/piotrmagiera/SWM/Dev/cairo/target/release/cairo-language-server",
    "cairo1.preferScarbLanguageServer": false,
 }

in .vscode/settings.json You can also access the file via command/crtl + shift + P + > ... open user settings (JSON) shortcut.

Regarding the implementation I am worried that checking if the syntax node is of kind ExprStructCtorCall directly may not work, it will be probably one of the parents of the node.

Regarding the default_value that you want to fill the struct members with I think it would be reasonable to check if the type has Default implementation: if so, we can just drop Default::default() there. Not sure what we should do if there is no Default implemented for the member, any idea @mkaput?

@mkaput
Copy link
Member Author

mkaput commented Aug 5, 2024

Regarding the default_value that you want to fill the struct members with I think it would be reasonable to check if the type has Default implementation: if so, we can just drop Default::default() there. Not sure what we should do if there is no Default implemented for the member, any idea @mkaput?

Just put () (unit value/empty tuple) in such case and expect people to fix compilation errors.

@the-first-elder
Copy link

i get this when i try to hover Request textDocument/hover failed.

@piotmag769
Copy link
Collaborator

Does it happen on your branch? Or the main one?

@the-first-elder
Copy link

mine but same on main as well

@piotmag769
Copy link
Collaborator

Could you provide reproduction for that in another issue?

@mkaput
Copy link
Member Author

mkaput commented Aug 20, 2024

Unassigning due to lack of activity.

@mkaput mkaput moved this from In Progress to Backlog in CairoLS Aug 20, 2024
@integraledelebesgue integraledelebesgue self-assigned this Oct 14, 2024
@mkaput mkaput moved this from Backlog to Todo in CairoLS Oct 14, 2024
@mkaput mkaput moved this from Todo to Backlog in CairoLS Oct 14, 2024
@poolcleaner6
Copy link

It might be helpful to outline the main areas where incremental updates could have the biggest impact, especially if the goal is to improve responsiveness with large files. Is there any specific guidance on this?

@mkaput
Copy link
Member Author

mkaput commented Nov 4, 2024

@poolcleaner6 I'm responding on CairoLS Telegram channel, that's a more proper place for this topic than this issue :)

@integraledelebesgue integraledelebesgue linked a pull request Nov 4, 2024 that will close this issue
@github-project-automation github-project-automation bot moved this from In Progress to Done in CairoLS Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants