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 inlay hints for function call args #1473

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shruti2522
Copy link
Contributor

@shruti2522 shruti2522 commented Jul 4, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

Screenshot from 2024-07-05 12-38-18

no inlay hint for builtin functions and system module functions args:

Screenshot from 2024-07-06 21-40-19

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@shruti2522 shruti2522 force-pushed the func-call-hint branch 2 times, most recently from 64bb5cc to 5df763d Compare July 4, 2024 10:36
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented Jul 4, 2024

CI failed. cc @shruti2522

@Peefy
Copy link
Contributor

Peefy commented Jul 4, 2024

@He1pa Could you help review it?

@Peefy
Copy link
Contributor

Peefy commented Jul 4, 2024

You can rebase the main branch and fix the CI errors.

Signed-off-by: shruti2522 <[email protected]>

make fmt

Signed-off-by: shruti2522 <[email protected]>

add inlay hints for function call args

Signed-off-by: shruti2522 <[email protected]>
@shruti2522 shruti2522 marked this pull request as draft July 5, 2024 04:57
@He1pa
Copy link
Contributor

He1pa commented Jul 5, 2024

for z = func([x: ]a, [y: ]y = 1, [z: ]z = 2) , [y: ] and [z: ] are not needed. I perfer z = func([x: ]a, y = 1, z = 2)

@shruti2522
Copy link
Contributor Author

for z = func([x: ]a, [y: ]y = 1, [z: ]z = 2) , [y: ] and [z: ] are not needed. I perfer z = func([x: ]a, y = 1, z = 2)

Yes. I have removed inlay hints for kwargs.

@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9804032628

Details

  • 43 of 44 (97.73%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 71.65%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/advanced_resolver/node.rs 43 44 97.73%
Totals Coverage Status
Change from base Build 9803208081: 0.02%
Covered Lines: 56612
Relevant Lines: 79012

💛 - Coveralls

Signed-off-by: shruti2522 <[email protected]>

remove inlay hints for kwargs

Signed-off-by: shruti2522 <[email protected]>

remove kw_name

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

add inlay hints for function call args

Signed-off-by: shruti2522 <[email protected]>

make fmt

Signed-off-by: shruti2522 <[email protected]>

add inlay hints for function call args

Signed-off-by: shruti2522 <[email protected]>

test ci

Signed-off-by: shruti2522 <[email protected]>

test ci

Signed-off-by: shruti2522 <[email protected]>

fix indexing error

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

update test snaps

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>
@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9804032628

Details

  • 46 of 47 (97.87%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 71.654%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/advanced_resolver/node.rs 46 47 97.87%
Files with Coverage Reduction New Missed Lines %
kclvm/sema/src/advanced_resolver/node.rs 1 87.66%
Totals Coverage Status
Change from base Build 9803208081: 0.03%
Covered Lines: 56615
Relevant Lines: 79012

💛 - Coveralls

Signed-off-by: shruti2522 <[email protected]>
@shruti2522 shruti2522 marked this pull request as ready for review July 5, 2024 07:26
@shruti2522
Copy link
Contributor Author

shruti2522 commented Jul 5, 2024

fixed CI @Peefy

@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9804685946

Details

  • 45 of 46 (97.83%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 71.654%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/advanced_resolver/node.rs 45 46 97.83%
Totals Coverage Status
Change from base Build 9803208081: 0.03%
Covered Lines: 56616
Relevant Lines: 79013

💛 - Coveralls

Signed-off-by: shruti2522 <[email protected]>

remove inlay hints for system module functions

Signed-off-by: shruti2522 <[email protected]>

change function name

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

remove hints for builtin func calls

Signed-off-by: shruti2522 <[email protected]>
@coveralls
Copy link
Collaborator

coveralls commented Jul 6, 2024

Pull Request Test Coverage Report for Build 9820424669

Details

  • 103 of 105 (98.1%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 71.675%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/advanced_resolver/node.rs 87 89 97.75%
Totals Coverage Status
Change from base Build 9803208081: 0.05%
Covered Lines: 56676
Relevant Lines: 79074

💛 - Coveralls

kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
@Peefy Peefy requested a review from He1pa July 8, 2024 02:28
@Peefy
Copy link
Contributor

Peefy commented Jul 8, 2024

I think there is a small problem here that when a function parameter is input with a literal constant, there is actually no parameter symbol here, right? Can you provide more ideas or suggestions? cc @He1pa

@He1pa
Copy link
Contributor

He1pa commented Jul 8, 2024

I think there is a small problem here that when a function parameter is input with a literal constant, there is actually no parameter symbol here, right? Can you provide more ideas or suggestions? cc @He1pa

There should be an Expression Symbol

kclvm/sema/src/builtin/system_module.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

shruti2522 commented Jul 8, 2024

I have changed the logic to get the value symbol for args, but the inlay hints aren't working. Please review @Peefy @He1pa

if let TypeKind::Function(func_ty) = &ty.kind {
let func_params = &func_ty.params;

if func_params.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can not use do_arguments_symbol_resolve_with_hint here?

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

I guess the snapshots suggest some position error is arising here, maybe the node key doesn't render the correct position of the arg.

-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────
  220   220 │             data: None,
  221   221 │         },
  222   222 │         InlayHint {
  223   223 │             position: Position {
        224 │+                line: 19,
        225 │+                character: 4,
        226 │+            },
        227 │+            label: LabelParts(
        228 │+                [
        229 │+                    InlayHintLabelPart {
        230 │+                        value: "y: ",
        231 │+                        tooltip: None,
        232 │+                        location: None,
        233 │+                        command: None,
        234 │+                    },
        235 │+                ],
        236 │+            ),
        237 │+            kind: None,
        238 │+            text_edits: None,
        239 │+            tooltip: None,
        240 │+            padding_left: None,
        241 │+            padding_right: None,
        242 │+            data: None,
        243 │+        },
        244 │+        InlayHint {
        245 │+            position: Position {
  224   246 │                 line: 21,
  225   247 │                 character: 3,
  226   248 │             },
  227   249 │             label: LabelParts(
┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
  284   306 │             padding_left: None,
  285   307 │             padding_right: None,
  286   308 │             data: None,
  287   309 │         },
  288       │-        InlayHint {
  289       │-            position: Position {
  290       │-                line: 27,
  291       │-                character: 9,
  292       │-            },
  293       │-            label: LabelParts(
  294       │-                [
  295       │-                    InlayHintLabelPart {
  296       │-                        value: "x: ",
  297       │-                        tooltip: None,
  298       │-                        location: None,
  299       │-                        command: None,
  300       │-                    },
  301       │-                ],
  302       │-            ),
  303       │-            kind: None,
  304       │-            text_edits: None,
  305       │-            tooltip: None,
  306       │-            padding_left: None,
  307       │-            padding_right: None,
  308       │-            data: None,
  309       │-        },
  310       │-        InlayHint {
  311       │-            position: Position {
  312       │-                line: 27,
  313       │-                character: 12,
  314       │-            },
  315       │-            label: LabelParts(
  316       │-                [
  317       │-                    InlayHintLabelPart {
  318       │-                        value: "y: ",
  319       │-                        tooltip: None,
  320       │-                        location: None,
  321       │-                        command: None,
  322       │-                    },
  323       │-                ],
  324       │-            ),
  325       │-            kind: None,
  326       │-            text_edits: None,
  327       │-            tooltip: None,
  328       │-            padding_left: None,
  329       │-            padding_right: None,
  330       │-            data: None,
  331       │-        },
  332   310 │     ],
  333   311 │ )

Need some suggestions here to figure out the issue because the logic seems fine @Peefy @He1pa

@He1pa
Copy link
Contributor

He1pa commented Jul 8, 2024

node

Two problems here:

  1. node_symbol_map.get() not always get value symbol. there are some other kind of symbols. but you get symbol from values by index. so insert hint y: to line 19.(this index is in expression but you get from value)
  2. for value symbol, this will get the definition symbol instead of ref symbol(for a, actually it will get symbol in line 1 instead of line 29). so the inlay hint will insert to line 1

The second question is a bit complicated for now, we can skip it.

    pub fn do_arguments_symbol_resolve_with_hint(
        &mut self,
        args: &'ctx [ast::NodeRef<ast::Expr>],
        kwargs: &'ctx [ast::NodeRef<ast::Keyword>],
        with_hint: bool,
        params: Vec<Parameter>,
    ) -> anyhow::Result<()> {
        for (arg, param) in args.iter().zip(params.iter()) {
            self.expr(arg)?;
            let symbol_data = self.gs.get_symbols_mut();

            if let Some(arg_ref) = symbol_data
                .symbols_info
                .node_symbol_map
                .get(&self.ctx.get_node_key(&arg.id))
            {
                match arg_ref.kind {
                    // crate::core::symbol::SymbolKind::Value => {
                    //     if let Some(value) = symbol_data.values.get_mut(arg_ref.get_id()) {
                    //         if with_hint {
                    //             value.hint = Some(SymbolHint::VarHint(param.name.clone()));
                    //         }
                    //     }
                    // }
                    crate::core::symbol::SymbolKind::Expression => {
                        if let Some(expr) = symbol_data.exprs.get_mut(arg_ref.get_id()) {
                            if with_hint {
                                expr.hint = Some(SymbolHint::VarHint(param.name.clone()));
                            }
                        }
                    }
                    _ => {}
                }
            }
        }

You also need to modify the definition of hint in SymbolKind::Expression
inlay hint will be z[: any] = func(a, [y: ]1, z = 2) if it work

Signed-off-by: shruti2522 <[email protected]>
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

4 participants