Skip to content

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jun 24, 2025

Summary

Ref: #14820 (comment)

This PR fixes a bug where virtual paths or any paths that doesn't exists on the file system weren't being considered for checking inclusion / exclusion. This was because the logic used file_path which returns None for those path. This PR fixes that by using the virtual_file_path method that returns a Path corresponding to the actual file on disk or any kind of virtual path.

This should ideally just fix the above linked issue by way of excluding the documents representing the interactive window because they aren't in the inclusion set. It failed only on Windows previously because the file path construction would fail and then Ruff would default to including all the files.

Test Plan

On my machine, the .interactive paths are always excluded so I'm using the inclusion set instead:

{
  "ruff.nativeServer": "on",
  "ruff.path": ["/Users/dhruv/work/astral/ruff/target/debug/ruff"],
  "ruff.configuration": {
    "extend-include": ["*.interactive"]
  }
}

The diagnostics are shown for both the file paths and the interactive window:

Screenshot 2025-06-24 at 14 56 40

And, the logs:

2025-06-24 14:56:26.478275000 DEBUG notification{method="notebookDocument/didChange"}: Included path via `extend-include`: /Interactive-1.interactive

And, when using ruff.exclude via:

{
	"ruff.exclude": ["*.interactive"]
}

With logs:

2025-06-24 14:58:41.117743000 DEBUG notification{method="notebookDocument/didChange"}: Ignored path via `exclude`: /Interactive-1.interactive

@dhruvmanila dhruvmanila added the server Related to the LSP server label Jun 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Jun 24, 2025

The diagnostics remain even after I close the interactive window:

2025-06-24 14:59:51.053545000 DEBUG notification{method="textDocument/didClose"}: Unable to close document with key vscode-notebook-cell:/Interactive-1.interactive#W1sdW50aXRsZWQ%3D - the snapshot was unavailable

TODO: Currently debugging

@dhruvmanila dhruvmanila marked this pull request as ready for review June 24, 2025 10:23
@dhruvmanila
Copy link
Member Author

TODO: Currently debugging

Ugh, it seems that VS Code is not sending the request in the order that we would get for other notebook documents.

When I close the interactive window, I get the following two requests:

[Trace - 3:56:51 PM] Sending notification 'notebookDocument/didClose'.
Params: {
    "notebookDocument": {
        "uri": "untitled:/Interactive-1.interactive"
    },
    "cellTextDocuments": [
        {
            "uri": "vscode-notebook-cell:/Interactive-1.interactive#W1sdW50aXRsZWQ%3D"
        }
    ]
}


[Trace - 3:56:51 PM] Sending notification 'textDocument/didClose'.
Params: {
    "textDocument": {
        "uri": "vscode-notebook-cell:/Interactive-1.interactive#W1sdW50aXRsZWQ%3D"
    }
}

Here, VS Code first sends the request to close the notebook document, so we remove it from the internal index. And, then when it sends us the request to close the cell document, we fail because the notebook document doesn't exists in the index:

let controller = self.documents.get(&url)?;

But, when I close a regular Jupyter Notebook (.ipynb file), it sends the request in reverse order:

[Trace - 4:13:58 PM] Sending notification 'textDocument/didClose'.
Params: {
    "textDocument": {
        "uri": "vscode-notebook-cell:/private/tmp/ruff-interactive-test/play.ipynb#W0sZmlsZQ%3D%3D"
    }
}


[Trace - 4:13:58 PM] Sending notification 'notebookDocument/didClose'.
Params: {
    "notebookDocument": {
        "uri": "file:///private/tmp/ruff-interactive-test/play.ipynb"
    },
    "cellTextDocuments": [
        {
            "uri": "vscode-notebook-cell:/private/tmp/ruff-interactive-test/play.ipynb#W0sZmlsZQ%3D%3D"
        }
    ]
}

@dhruvmanila dhruvmanila requested a review from MichaReiser June 24, 2025 11:26
@dhruvmanila
Copy link
Member Author

Discussing this internally, I think the above mentioned inconsistency can be fixed in a separate PR as it isn't related to the issue.

@MichaReiser
Copy link
Member

Thanks for looking into this issue

@dhruvmanila dhruvmanila enabled auto-merge (squash) June 24, 2025 12:21
@dhruvmanila dhruvmanila merged commit 66fc7c8 into main Jun 24, 2025
33 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/server-virtual-path branch June 24, 2025 12:24
dcreager added a commit that referenced this pull request Jun 24, 2025
* main:
  [ty] Fix false positives when subscripting an object inferred as having an `Intersection` type (#18920)
  [`flake8-use-pathlib`] Add autofix for `PTH202` (#18763)
  [ty] Add relative import completion tests
  [ty] Clarify what "cursor" means
  [ty] Add a cursor test builder
  [ty] Enforce sort order of completions (#18917)
  [formatter] Fix missing blank lines before decorated classes in .pyi files (#18888)
  Apply fix availability and applicability when adding to `DiagnosticGuard` and remove `NoqaCode::rule` (#18834)
  py-fuzzer: allow relative executable paths (#18915)
  [ty] Change `environment.root` to accept multiple paths (#18913)
  [ty] Rename `src.root` setting to `environment.root` (#18760)
  Use file path for detecting package root (#18914)
  Consider virtual path for various server actions (#18910)
  [ty] Introduce `UnionType::try_from_elements` and `UnionType::try_map` (#18911)
  [ty] Support narrowing on `isinstance()`/`issubclass()` if the second argument is a dynamic, intersection, union or typevar type (#18900)
  [ty] Add decorator check for implicit attribute assignments (#18587)
  [`ruff`] Trigger `RUF037` for empty string and byte strings (#18862)
  [ty] Avoid duplicate diagnostic in unpacking (#18897)
  [`pyupgrade`] Extend version detection to include `sys.version_info.major` (`UP036`) (#18633)
  [`ruff`] Frozen Dataclass default should be valid (`RUF009`) (#18735)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants