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: add hints for schema config expr #1589

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 65 additions & 3 deletions kclvm/sema/src/advanced_resolver/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,24 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for AdvancedResolver<'ctx> {
}

fn walk_config_expr(&mut self, config_expr: &'ctx ast::ConfigExpr) -> Self::Result {
self.walk_config_entries(&config_expr.items)?;
let (start, _end) = (self.ctx.start_pos.clone(), self.ctx.end_pos.clone());

let pre_pos = Position {
filename: start.filename.clone(),
line: start.line.clone(),
column: start.column.map(|c| if c >= 1 { c - 1 } else { 0 }),
};

let with_hint = if let Some(stmt) = self.ctx.program.pos_to_stmt(&pre_pos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The complexity of this function is a bit high, and other methods can be used to determine it.

Besides, why add hint only for the assign_stmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess hints have to applied only for anonymous schema configs, like

schema Name:
    name: str

n: Name = {
    name = "kcl"
}

that's why I used assign_stmt to determine if type annotations exist for the same, if yes then hints are applied, otherwise not.

Copy link
Contributor

Choose a reason for hiding this comment

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

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n: Name = {
    name = "foo"
    config: {
        count: 1
    }
}

The config in the Name schema also needs hints e.g.

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n: Name = [Name ] { # Name schema hints
    name = "foo"
    config: [Config ] { # Config schema hints
        count: 1
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-08-28 18-21-55

hints are being applied for the config too

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this code, an assign stmt with the type annotation and the right value is a schema expr.

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n Name = Name { # Name schema hints
    name = "foo"
    config: [Config ] { # Config schema hints
        count: 1
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-08-28 18-35-23

I guess we need to add check for schema_expr too

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that there are many such grammars, not just checking schema_exp or assign_stmt, but there needs to be a more effective way.

match stmt.node {
ast::Stmt::Assign(ref assign_stmt) => assign_stmt.ty.is_some(),
_ => false,
}
} else {
false
};

self.walk_config_entries_with_hint(&config_expr.items, with_hint)?;
Ok(None)
}

Expand Down Expand Up @@ -1879,6 +1896,14 @@ impl<'ctx> AdvancedResolver<'ctx> {
pub(crate) fn walk_config_entries(
&mut self,
entries: &'ctx [ast::NodeRef<ast::ConfigEntry>],
) -> anyhow::Result<()> {
self.walk_config_entries_with_hint(entries, false)
}

pub(crate) fn walk_config_entries_with_hint(
&mut self,
entries: &'ctx [ast::NodeRef<ast::ConfigEntry>],
with_hint: bool,
) -> anyhow::Result<()> {
let (start, end) = (self.ctx.start_pos.clone(), self.ctx.end_pos.clone());

Expand All @@ -1887,8 +1912,8 @@ impl<'ctx> AdvancedResolver<'ctx> {

self.enter_local_scope(
&self.ctx.current_filename.as_ref().unwrap().clone(),
start,
end,
start.clone(),
end.clone(),
kind,
);

Expand All @@ -1899,6 +1924,43 @@ impl<'ctx> AdvancedResolver<'ctx> {
.set_owner_to_scope(*cur_scope, owner);
}

if with_hint {
if let Some(schema_symbol) = schema_symbol {
let symbol_data = self.gs.get_symbols_mut();
if let Some(schema_data) = symbol_data.get_symbol(schema_symbol) {
if let Some(ty) = &schema_data.get_sema_info().ty {
let mut expr_symbol = ExpressionSymbol::new(
Peefy marked this conversation as resolved.
Show resolved Hide resolved
format!("@{}", ty.ty_str()),
end.clone(),
end.clone(),
None,
);

expr_symbol.sema_info = SymbolSemanticInfo {
ty: self
.ctx
.node_ty_map
.borrow()
.get(&self.ctx.get_node_key(&ast::AstIndex::default()))
.cloned(),
doc: None,
};

expr_symbol.hint = Some(SymbolHint {
pos: start.clone(),
kind: SymbolHintKind::VarHint(schema_data.get_name().to_owned()),
});

symbol_data.alloc_expression_symbol(
expr_symbol,
self.ctx.get_node_key(&ast::AstIndex::default()),
self.ctx.current_pkgpath.clone().unwrap(),
);
}
}
}
}

let mut entries_range = vec![];
for entry in entries.iter() {
entries_range.push((
Expand Down
5 changes: 5 additions & 0 deletions kclvm/tools/src/LSP/src/inlay_hints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,9 @@ mod tests {
test_schema_arg_hint,
"src/test_data/inlay_hints/schema_args/schema_args_hint.k"
);

inlay_hints_test_snapshot!(
test_schema_config_hint,
"src/test_data/inlay_hints/schema_config_hint/schema_config_hint.k"
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
source: tools/src/LSP/src/inlay_hints.rs
assertion_line: 127
expression: "format!(\"{:#?}\", res)"
---
Some(
[
InlayHint {
position: Position {
line: 3,
character: 10,
},
label: LabelParts(
[
InlayHintLabelPart {
value: "Name: ",
tooltip: None,
location: None,
command: None,
},
],
),
kind: None,
text_edits: None,
tooltip: None,
padding_left: None,
padding_right: None,
data: None,
},
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
schema Name:
name: str

n: Name = {
name = "kcl"
}
Loading