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: async compile in lsp #922

Merged
merged 3 commits into from
Nov 28, 2023
Merged

feat: async compile in lsp #922

merged 3 commits into from
Nov 28, 2023

Conversation

He1pa
Copy link
Contributor

@He1pa He1pa commented Nov 28, 2023

feat: asymc compile in lsp
1 support async compile when vfs changes(open file and ChangeTextDocument). 2. remove scope in lsp db

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

lsp

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

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

@He1pa He1pa self-assigned this Nov 28, 2023
@He1pa He1pa added the lsp label Nov 28, 2023
@He1pa He1pa added this to the v0.7.0 Release milestone Nov 28, 2023
kclvm/tools/src/LSP/src/state.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/state.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/state.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented Nov 28, 2023

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Nov 28, 2023

PR Analysis

(review updated until commit 09063f2)

  • 🎯 Main theme: Asynchronous compilation in Language Server Protocol (LSP)
  • 📝 PR summary: This PR introduces asynchronous compilation in the Language Server Protocol (LSP) when Virtual File System (VFS) changes occur. It also removes the 'scope' from the LSP database. The changes mainly affect the 'completion.rs' and 'analysis.rs' files.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR involves changes in the core functionality of the LSP, which requires a deep understanding of the system. Additionally, the PR does not include any new tests for the introduced changes.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR seems to be well-structured and the changes are logically consistent. However, it would be beneficial to include tests for the new functionality to ensure it works as expected and does not introduce any regressions. Also, it would be helpful to provide more context or explanation about why the 'scope' was removed from the LSP database.

  • 🤖 Code feedback:
    • relevant file: kclvm/tools/src/LSP/src/completion.rs
      suggestion: It seems like the 'prog_scope' parameter was removed from several functions. If this parameter is no longer needed, make sure to remove any remaining references or unused imports related to it. [important]
      relevant line: -use kclvm_sema::resolver::scope::ProgramScope;

    • relevant file: kclvm/tools/src/LSP/src/analysis.rs
      suggestion: The Analysis struct now uses an Arc<RwLock<>> for the 'db' field. Make sure to handle potential lock contention and errors properly throughout the code where this field is accessed. [important]
      relevant line: pub db: Arc<RwLock<HashMap<FileId, AnalysisDatabase>>>,

How to use

Instructions

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@coveralls
Copy link
Collaborator

coveralls commented Nov 28, 2023

Pull Request Test Coverage Report for Build 7017050254

  • 122 of 164 (74.39%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 72.602%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/tools/src/LSP/src/state.rs 47 54 87.04%
kclvm/tools/src/LSP/src/completion.rs 29 38 76.32%
kclvm/tools/src/LSP/src/request.rs 13 39 33.33%
Files with Coverage Reduction New Missed Lines %
kclvm/sema/src/resolver/scope.rs 1 56.55%
Totals Coverage Status
Change from base Build 7016387853: -0.04%
Covered Lines: 42248
Relevant Lines: 58191

💛 - Coveralls

@Peefy Peefy changed the title feat: asymc compile in lsp feat: async compile in lsp Nov 28, 2023
kclvm/tools/src/LSP/src/request.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/request.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented Nov 28, 2023

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 09063f2

@He1pa He1pa force-pushed the async_compile_in_lsp branch from da580b9 to 4cab98e Compare November 28, 2023 08:53
@He1pa He1pa force-pushed the async_compile_in_lsp branch from 4cab98e to a04502b Compare November 28, 2023 09:04
…open file and ChangeTextDocument). 2. remove scope in lsp db

Signed-off-by: he1pa <[email protected]>
@He1pa He1pa force-pushed the async_compile_in_lsp branch from a04502b to 1bf75e5 Compare November 28, 2023 09:30
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

Signed-off-by: he1pa <[email protected]>
@Peefy Peefy merged commit 5ab79bb into kcl-lang:main Nov 28, 2023
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Status: v0.7.0 Release Done
Development

Successfully merging this pull request may close these issues.

4 participants