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

LSP inlay hints for str, int, float, bool, any types #1431

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

shruti2522
Copy link
Contributor

@shruti2522 shruti2522 commented Jun 23, 2024

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

  • N
  • Y

fix #1244

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

kclvm/tools/src/LSP/src/inlay_hints.rs
kclvm/tools/src/LSP/src/capabilities.rs
kclvm/tools/src/LSP/src/lib.rs
kclvm/tools/src/LSP/src/main.rs
kclvm/tools/src/LSP/src/request.rs
kclvm/tools/src/LSP/Cargo.toml

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

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

cc @He1pa @Peefy

@coveralls
Copy link
Collaborator

coveralls commented Jun 23, 2024

Pull Request Test Coverage Report for Build 9631229019

Details

  • 1 of 108 (0.93%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 71.154%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/tools/src/LSP/src/main.rs 0 1 0.0%
kclvm/tools/src/LSP/src/capabilities.rs 0 7 0.0%
kclvm/tools/src/LSP/src/request.rs 1 20 5.0%
kclvm/tools/src/LSP/src/inlay_hints.rs 0 80 0.0%
Files with Coverage Reduction New Missed Lines %
kclvm/tools/src/LSP/src/main.rs 1 0.0%
Totals Coverage Status
Change from base Build 9596528207: -0.1%
Covered Lines: 55615
Relevant Lines: 78161

💛 - Coveralls

@shruti2522 shruti2522 changed the title [WIP] feat: add LSP inaly-hint provider [WIP] LSP inaly-hint provider Jun 23, 2024
@shruti2522 shruti2522 changed the title [WIP] LSP inaly-hint provider [WIP] LSP inlayhint provider Jun 23, 2024
@shruti2522
Copy link
Contributor Author

shruti2522 commented Jun 28, 2024

KCL Inlay Hints

1. Introduction

  • Inlay hints are provided for the cases when there is no explicit type declaration.
  • [x] is used for the hints where x represents the type name.
  • Hints are displayed only for left valued variables without explicit type annotations.

2. Design principles

TypeKind::Str|Int|Float|Bool|Any

  • Hints show the type enclosed in brackets [: x] if it is not already assigned.
    Examples:
    • int/float: n [: float] = 1
    • str: x [: str] = "kcl"
    • bool: value [: bool] = True
    • any: func = lambda x [: any] { x }

TypeKind::Schema

  • schema definition:

    for example,

    schema Persons:
      n1 [: int] = n - 1
      n2 [: int] = n1 - 1
      n: int
      value: int
    

    No inlay hints applied for the following cases:

    • schema signature string

      schema Persons:
      
    • schema attributes with type definition

        n: int
        value: int
      
  • schema assign statements:

    • for type definition no hints are provided

      p: Person = Person{}
      
    • hints when there is no type definition

      p [: Person] = Person{}
      

TypeKind::Function

  • for function definition

    func = lambda x[: any]{
          x
    }
    
  • for function call

    z = func([x:] y)
    

    where, y is the argument and x is the param name in func definition.

  • no inlay hints for builtin function calls

TypeKind::List

  • for list assignments

    _list1 [: [int]] = [1, 2, 3]
    _list2 [: [str]] = ["kcl","lang"]
    

TypeKind::Dict

  • for dict assignments

    data1 [: {str:str}] = {
      if False: "key1": "value1"
      "key2": "value2"
      if True: "key3": "value3"
    }
    

TypeKind::NumberMultiplier

  • for number multiplier assign stmts

    x0: units.NumberMultiplier = 1M
    x1 [: number_multiplier] = x0
    

TypeKind::Union

  • for union types the name of the union type is provided as inlay hint

    type num = int | float
    n [: num] = 1
    
  • no inlay hint for type assignment

    n: num = 1
    

@shruti2522
Copy link
Contributor Author

please review the above design doc @He1pa @Peefy

@Peefy
Copy link
Contributor

Peefy commented Jun 28, 2024

Good Job! 👍 I will only add one implementation point: we only need to display hint for left valued variables without explicit type annotations.

@He1pa
Copy link
Contributor

He1pa commented Jul 1, 2024

for func definition, I think the first [: fn func] is also unnecessary. but need hints for params. I prefer

func = lambda x[: any]{
  x
}

for func call, y is the argument. x is not type for y, it is a param name in func definition, so the hint [x:] before y

z = func([x:] y)

I don’t recommend you to complete all the work in one PR, so you can put the design doc in the track issue instead of the PR.

@shruti2522
Copy link
Contributor Author

Inlay hints for TypeKind::Str|Int|Float|Bool|Any

Screenshot from 2024-07-02 21-06-46

@shruti2522 shruti2522 changed the title [WIP] LSP inlayhint provider LSP inlay hints for str, int, float, bool, any types Jul 2, 2024
@shruti2522 shruti2522 marked this pull request as ready for review July 2, 2024 15:44
@coveralls
Copy link
Collaborator

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9763995728

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 97 (2.06%) changed or added relevant lines in 4 files are covered.
  • 1758 unchanged lines in 39 files lost coverage.
  • Overall coverage increased (+0.2%) to 71.449%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/tools/src/LSP/src/main.rs 0 1 0.0%
kclvm/tools/src/LSP/src/capabilities.rs 0 2 0.0%
kclvm/tools/src/LSP/src/request.rs 2 18 11.11%
kclvm/tools/src/LSP/src/inlay_hints.rs 0 76 0.0%
Files with Coverage Reduction New Missed Lines %
kclvm/runtime/src/value/val_func.rs 1 80.0%
kclvm/runtime/src/_kclvm.rs 1 25.0%
kclvm/ast/src/ast.rs 1 83.38%
kclvm/driver/src/toolchain.rs 1 88.12%
kclvm/tools/src/LSP/src/main.rs 1 0.0%
kclvm/sema/src/builtin/option.rs 1 0.0%
kclvm/ast/src/token.rs 2 57.53%
kclvm/tools/src/LSP/src/goto_def.rs 2 94.96%
kclvm/sema/src/builtin/system_module.rs 2 99.88%
kclvm/query/src/query.rs 3 36.96%
Totals Coverage Status
Change from base Build 9596528207: 0.2%
Covered Lines: 56196
Relevant Lines: 78652

💛 - Coveralls

for symbol_ref in symbols {
if let Some(symbol) = gs.get_symbols().get_symbol(*symbol_ref) {
let (start, end) = symbol.get_range();
if has_type_assignment(program, &start) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this information should be obtained by extending the API of symbol, such as is_declaration/as_declaration (in the sema module), rather than recalculating it in the syntax tree, because for a large number of symbol computations, it requires a lot of computation. cc @He1pa Could you help review it and give more comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that hint information should not be calculated here. Perhaps it is more appropriate to calculate it when traversing the AST in AdvancedResolver. My suggestion is to store hint-related information in the symbol's sema_info. You can abstract a struct to record this information, and use this struct toconstruct the results in lsp.

use lsp_types::{InlayHint, InlayHintLabelPart, Position as LspPosition, Range};
use std::convert::TryInto;

pub fn inlay_hints(file: &str, gs: &GlobalState, program: &Program) -> Option<Vec<InlayHint>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add more comments on public functions.

match &ty.kind {
TypeKind::Str | TypeKind::Bool | TypeKind::Int | TypeKind::Float | TypeKind::Any => {
label_parts.push(InlayHintLabelPart {
value: format!("[: {}]", ty.ty_str()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
value: format!("[: {}]", ty.ty_str()),
value: format!(": {}", ty.ty_str()),

@Peefy
Copy link
Contributor

Peefy commented Jul 3, 2024

In addition to assign_stmt, other syntax including types and variables that may needs inlay hints.

  • function parameters and return types
pub struct LambdaExpr {
    pub args: Option<NodeRef<Arguments>>,
    pub body: Vec<NodeRef<Stmt>>,
    pub return_ty: Option<NodeRef<Type>>,
}
func = lambda x[: any] [-> any] {
  x
}
  • args of function calling parameters.
pub struct CallExpr {
    pub func: NodeRef<Expr>,
    pub args: Vec<NodeRef<Expr>>,
    pub keywords: Vec<NodeRef<Keyword>>,
}
z = func([x:] y)
  • Loop and quant expr variables
values = [1, 2, 3]
data = all x [: [int]] in values {
    x >= 1
}

let mut label_parts = Vec::new();

match &ty.kind {
TypeKind::Str | TypeKind::Bool | TypeKind::Int | TypeKind::Float | TypeKind::Any => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Function type and schema type should also be included.

@Peefy Peefy added feat semantic Issues or PRs related to kcl semantic and checker lsp labels Jul 3, 2024
@Peefy Peefy added this to the v0.9.0 Release milestone Jul 3, 2024
@Peefy
Copy link
Contributor

Peefy commented Jul 3, 2024

Hello @shruti2522 I've leaved some comments and You can open other PRs to deal this and I will merge this PR to test.

@Peefy Peefy merged commit 203a576 into kcl-lang:main Jul 3, 2024
9 of 12 checks passed
@shruti2522
Copy link
Contributor Author

okay @Peefy, I will open another PR to resolve above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat lsp semantic Issues or PRs related to kcl semantic and checker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LFX] [Track] LSP inlayhint and hover content content optimization
4 participants