diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..d3bba4f6 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,67 @@ +# CLAUDE.md + +Guidance for [Claude Code](https://claude.com/claude-code) when working in this repository. + +## What this repository is + +`pyls` is an implementation of the Language Server Protocol for Python. An editor speaks JSON-RPC to it over stdio or TCP; the server parses each open document and answers completion, definition, hover, reference, symbol and formatting requests, mostly by delegating to Jedi and to linters loaded as pluggy plugins. + +Two things shape almost every decision here: + +- **It still targets Python 2.7.** `setup.py` carries `python_version<"3"` markers and CI runs a 2.7 job. That rules out f-strings, and it is why source literals in tests carry `u''` prefixes. +- **Jedi is pinned below 0.18** (`jedi>=0.17.2,<0.18.0`). The 0.18 release changed the `Script` API, so the pin is load-bearing, not incidental. + +Development here has been quiet since December 2020 — the most recent commit is an automated config change — and the actively maintained community fork is `python-lsp-server`. Worth knowing before promising a change will ship, but this repository is not archived and still accepts issues and PRs. + +## Setting up an environment that works + +**A modern interpreter will not work.** CI runs 2.7, 3.6, 3.7 and 3.8, and the pinned Jedi does not install on recent Pythons. Use 3.8: + +``` +uv python install 3.8 +uv venv --python 3.8 /tmp/pls +uv pip install --python /tmp/pls/bin/python -e ".[all,test]" +uv pip install --python /tmp/pls/bin/python "setuptools<70" "pylint<3" +``` + +The last line is not optional: + +- `pyls/config/config.py` imports `pkg_resources`, which setuptools 70+ no longer ships. +- `test/plugins/test_pylint_lint.py` imports `pylint.epylint`, removed in pylint 3. + +## Commands + +These are the four steps CI runs, in order: + +``` +/tmp/pls/bin/python -m pytest test/ -q +/tmp/pls/bin/python -m pylint pyls test +/tmp/pls/bin/python -m pycodestyle pyls test +/tmp/pls/bin/python -m pyflakes pyls test +``` + +**A clean checkout does not produce a clean run.** Record a baseline before changing anything and compare against it, rather than assuming a failure is yours. On Python 3.8 with current dependency versions a clean checkout gives roughly **12 failed, 99 passed, 8 skipped**, concentrated in the flake8, pydocstyle, pylint and numpy-hover tests — these track linter versions that have moved on since 2020. `pylint pyls test` reports around 84 messages and `pyflakes` reports an undefined `unicode` in `pyls/_utils.py`, both pre-existing. + +## URIs are not always files + +A document is identified by a URI, and **not every URI has a file behind it**. Editors such as Monaco send `inmemory://dummy.py`, and VS Code sends `untitled:` and `vscode-notebook-cell:`; the contents of those documents only ever arrive over `textDocument/didOpen` and `didChange`. + +- `uris.to_fs_path()` returns a path-shaped **identifier**. For a non-file URI it is not a location on disk and must not be opened, walked, or used to derive a directory. +- `uris.is_file_uri()` is the check to use before doing anything filesystem-flavoured with `Document.path`, and `Document.is_file_backed` caches it. +- Note that `inmemory://dummy.py` puts `dummy.py` in the URI *authority*, not the path, so `urlparse` yields an empty path for it. That is a normal shape, not a malformed URI. + +Anything that computes `os.path.dirname(document.path)` and feeds it to Jedi, rope or a linter needs to be guarded by `is_file_backed`. An unguarded `dirname` of a non-file document used to normalize to `'.'`, which silently put the server's working directory on the module search path. + +## Conventions + +- **Every source file starts with `# Copyright 2017 Palantir Technologies, Inc.`** Copy it from a neighbouring file. +- **Write Python 2/3 compatible code.** No f-strings; use `.format()`. `pylint` will suggest `consider-using-f-string` anyway — that suggestion does not apply here. +- **Plugins are pluggy hooks** registered as entry points in `setup.py`. Adding a plugin means adding both the module and its `pyls` entry point. +- **Tests use plain pytest with fixtures in `test/fixtures.py`** (`workspace`, `doc`, `config`, `pyls`), re-exported through `test/conftest.py`. `from test.fixtures import ...` is deliberately the first import in a test module: `test` shadows a stdlib name, so pylint sorts it as a standard import and reports `wrong-import-order` if anything precedes it. + +## Things to be careful about + +- **Do not "modernize" the codebase.** Removing `u''` prefixes, `object` base classes or `.format()` calls breaks the Python 2.7 job. +- **Do not bump Jedi past 0.18** without reworking `Document.jedi_script`; the `Script` constructor signature changed. +- **`pyls/uris.py` mirrors VS Code's `vscode-uri`** and says so. If you diverge from it, say why in a comment, because the next reader will check the two against each other. +- **Changing `to_fs_path` affects everything.** It feeds `Document.path`, the workspace root, config discovery and most plugins. Verify plugin behaviour, not just the URI unit tests. diff --git a/pyls/uris.py b/pyls/uris.py index 7299ac9f..9cc5c2cf 100644 --- a/pyls/uris.py +++ b/pyls/uris.py @@ -43,12 +43,26 @@ def urlunparse(parts): )) +def is_file_uri(uri): + """Return whether the URI refers to a path on the local filesystem. + + A URI with no scheme is treated as a file URI, matching how editors send bare + paths. Everything else (inmemory:, untitled:, vscode-notebook-cell:, http:, ...) + has no file behind it, so its contents only exist in memory. + """ + return urlparse(uri)[0] in ('file', '') + + def to_fs_path(uri): """Returns the filesystem path of the given URI. Will handle UNC paths and normalize windows drive letters to lower-case. Also uses the platform specific path separator. Will *not* validate the path for invalid characters and semantics. Will *not* look at the scheme of this URI. + + For a non-file URI the result is a path-shaped identifier rather than a real + location on disk, and callers must not assume it can be opened. Use is_file_uri + to tell the two apart. """ # scheme://netloc/path;parameters?query#fragment scheme, netloc, path, _params, _query, _fragment = urlparse(uri) @@ -57,6 +71,14 @@ def to_fs_path(uri): # unc path: file://shares/c$/far/boo value = "//{}{}".format(netloc, path) + elif netloc and scheme not in ('file', ''): + # Non-file URI with an authority, such as Monaco's inmemory://dummy.py or + # vscode-notebook-cell://notebook/cell.py. vscode-uri drops the authority here + # and returns just the path, which is empty for inmemory://dummy.py. That left + # the document with no name at all, so the authority is kept instead: the + # document still needs a stable identity for module naming and diagnostics. + value = "/{}{}".format(netloc, path) + elif RE_DRIVE_LETTER_PATH.match(path): # windows drive letter: file:///C:/far/boo value = path[1].lower() + path[2:] diff --git a/pyls/workspace.py b/pyls/workspace.py index 78674d31..75608c5d 100644 --- a/pyls/workspace.py +++ b/pyls/workspace.py @@ -135,6 +135,10 @@ def __init__(self, uri, workspace, source=None, version=None, local=True, extra_ self.path = uris.to_fs_path(uri) self.dot_path = _utils.path_to_dot_name(self.path) self.filename = os.path.basename(self.path) + # Documents opened under a scheme such as inmemory: or untitled: have no file + # behind them. self.path is a stable identifier for those, not a location that + # can be opened or walked. + self.is_file_backed = uris.is_file_uri(uri) self._config = workspace._config self._workspace = workspace @@ -160,6 +164,15 @@ def lines(self): @lock def source(self): if self._source is None: + if not self.is_file_backed: + # Nothing to fall back to: the contents only ever arrive over + # textDocument/didOpen and didChange. Opening self.path would either + # raise a confusing error about a path the client never mentioned, or + # read an unrelated file that happens to share the name. + raise ValueError( + 'no source available for {}: a document with a non-file URI must be ' + 'opened before it can be read'.format(self.uri) + ) with io.open(self.path, 'r', encoding='utf-8') as f: return f.read() return self._source @@ -262,12 +275,15 @@ def jedi_script(self, position=None, use_document_path=False): project_path = self._workspace.root_path # Extend sys_path with document's path if requested - if use_document_path: + if use_document_path and self.is_file_backed: + # Skipped for non-file documents: os.path.dirname of their identifier is not + # a real directory. It used to normalize to '.', which silently put the + # server's working directory on the module search path. sys_path += [os.path.normpath(os.path.dirname(self.path))] kwargs = { 'code': self.source, - 'path': self.path, + 'path': self.path or None, 'environment': environment, 'project': jedi.Project(path=project_path, sys_path=sys_path), } diff --git a/test/test_document.py b/test/test_document.py index dc54613a..87e88d86 100644 --- a/test/test_document.py +++ b/test/test_document.py @@ -1,5 +1,16 @@ # Copyright 2017 Palantir Technologies, Inc. from test.fixtures import DOC_URI, DOC +import os + +try: + from unittest import mock +except ImportError: + import mock + +import jedi +import pytest + +from pyls import uris from pyls.workspace import Document @@ -97,3 +108,69 @@ def test_document_end_of_file_edit(workspace): "print 'b'\n", "o", ] + + +INMEMORY_URI = 'inmemory://dummy.py' + + +def test_non_file_document_props(workspace): + """A document under a non-file scheme still needs a usable name. + + Monaco sends inmemory://dummy.py, which puts the name in the URI authority. That used + to resolve to an empty path, leaving the document with no filename or module name. + """ + document = Document(INMEMORY_URI, workspace, u'import sys') + assert document.uri == INMEMORY_URI + assert document.path == '/dummy.py' + assert document.filename == 'dummy.py' + assert document.dot_path == 'dummy' + assert document.is_file_backed is False + + +def test_non_file_document_source(workspace): + document = Document(INMEMORY_URI, workspace, u'import sys') + assert document.source == u'import sys' + + +def test_non_file_document_without_source_raises(workspace): + """There is no file to fall back to, so say so rather than opening something else.""" + document = Document(INMEMORY_URI, workspace) + with pytest.raises(ValueError) as excinfo: + document.source # pylint: disable=pointless-statement + assert INMEMORY_URI in str(excinfo.value) + + +def test_non_file_document_does_not_extend_sys_path(workspace): + """os.path.dirname of a non-file document is not a directory. + + It previously normalized to '.', which put the server's working directory on the + module search path for any in-memory document. + """ + document = Document(INMEMORY_URI, workspace, u'import sys') + with mock.patch('jedi.Project', wraps=jedi.Project) as project: + document.jedi_script(use_document_path=True) + sys_path = project.call_args[1]['sys_path'] + assert os.getcwd() not in sys_path + assert '/' not in sys_path + + +def test_file_document_still_extends_sys_path(tmpdir, workspace): + """The file-backed behaviour is unchanged.""" + subdir = tmpdir.mkdir('sub') + source_file = subdir.join('real.py') + source_file.write('x = 1') + + document = Document(uris.from_fs_path(str(source_file)), workspace, u'x = 1') + assert document.is_file_backed is True + with mock.patch('jedi.Project', wraps=jedi.Project) as project: + document.jedi_script(use_document_path=True) + sys_path = project.call_args[1]['sys_path'] + assert str(subdir) in sys_path + + +def test_non_file_document_completions(workspace): + """The case from the issue: completions inside an in-memory document.""" + document = Document(INMEMORY_URI, workspace, u'import sys\nsys.') + script = document.jedi_script(use_document_path=True) + completions = [completion.name for completion in script.complete(2, 4)] + assert 'argv' in completions diff --git a/test/test_uris.py b/test/test_uris.py index d4e177e6..6829158d 100644 --- a/test/test_uris.py +++ b/test/test_uris.py @@ -14,6 +14,31 @@ def test_to_fs_path(uri, path): assert uris.to_fs_path(uri) == path +@unix_only +@pytest.mark.parametrize('uri,path', [ + # Monaco sends inmemory://name.py, where the name lands in the authority rather than + # the path. Dropping the authority left these documents with no name at all. + ('inmemory://dummy.py', '/dummy.py'), + ('inmemory:///dummy.py', '/dummy.py'), + ('vscode-notebook-cell://notebook/cell.py', '/notebook/cell.py'), + ('untitled:Untitled-1', 'Untitled-1'), +]) +def test_non_file_uri_to_fs_path(uri, path): + assert uris.to_fs_path(uri) == path + + +@pytest.mark.parametrize('uri,is_file', [ + ('file:///foo/bar', True), + ('/foo/bar', True), + ('inmemory://dummy.py', False), + ('untitled:Untitled-1', False), + ('vscode-notebook-cell://notebook/cell.py', False), + ('http://example.com/foo.py', False), +]) +def test_is_file_uri(uri, is_file): + assert uris.is_file_uri(uri) is is_file + + @windows_only @pytest.mark.parametrize('uri,path', [ ('file:///c:/far/boo', 'c:\\far\\boo'),