Skip to content

fix(lsp): context is singleton in vscode #4722

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.

Sign up for GitHub

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

Merged
benfdking merged 1 commit into from
Jun 11, 2025
Merged

fix(lsp): context is singleton in vscode #4722

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 17 additions & 7 deletions sqlmesh/lsp/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,13 @@ def _create_lsp_context(self, paths: t.List[Path]) -> t.Optional[LSPContext]:
A new LSPContext instance wrapping the created context, or None if creation fails
"""
try:
context = self.context_class(paths=paths)
lsp_context = LSPContext(context)
self.lsp_context = lsp_context
return lsp_context
if self.lsp_context is None:
context = self.context_class(paths=paths)
else:
self.lsp_context.context.load()
context = self.lsp_context.context
self.lsp_context = LSPContext(context)
return self.lsp_context
Comment on lines +109 to +115
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

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

The current logic re-wraps the context in a new LSPContext even when one exists. To fully honor the singleton pattern, consider reusing the existing LSPContext instance when no state updates are required.

Copilot uses AI. Check for mistakes.
except Exception as e:
self.server.log_trace(f"Error creating context: {e}")
return None
Expand Down Expand Up @@ -293,8 +296,16 @@ def did_open(ls: LanguageServer, params: types.DidOpenTextDocumentParams) -> Non

@self.server.feature(types.TEXT_DOCUMENT_DID_CHANGE)
def did_change(ls: LanguageServer, params: types.DidChangeTextDocumentParams) -> None:
if self.lsp_context is None:
current_path = Path.cwd()
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

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

Using Path.cwd() might be fragile if the server's working directory does not reflect the intended project folder. Consider deriving the path from the document URI or a configuration parameter to ensure the correct context is used.

Suggested change
current_path = Path.cwd()
current_path = self._get_workspace_folder_or_default()

Copilot uses AI. Check for mistakes.
self._ensure_context_in_folder(current_path)
Comment thread
benfdking marked this conversation as resolved.
if self.lsp_context is None:
ls.log_trace("No context found after did_change")
return

uri = URI(params.text_document.uri)
context = self._context_get_or_load(uri)

models = context.map[uri.to_path()]
if models is None or not isinstance(models, ModelTarget):
return
Expand Down Expand Up @@ -731,9 +742,8 @@ def _ensure_context_for_document(
for a config.py or config.yml file in the parent directories.
"""
if self.lsp_context is not None:
context = self.lsp_context
context.context.load() # Reload or refresh context
self.lsp_context = LSPContext(context.context)
self.lsp_context.context.load()
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The context reinitialization logic is similar to what is done in _create_lsp_context. Consider refactoring this duplication into a helper method to ensure consistent behavior and easier maintenance.

Copilot uses AI. Check for mistakes.
self.lsp_context = LSPContext(self.lsp_context.context)
return

# No context yet: try to find config and load it
Expand Down