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

red-knot: source_text, line_index, and parsed_module queries #11822

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 10, 2024

Summary

This PR adds the most basic file-based queries that are needed for semantic analysis, linting and formatting:

  • source_text: Reads the content of a file. Changing a file's revision markes the query result as stale. I excluded jupyter notebook support for now.
  • line_index: Computes the line index for a file's source text.
  • parsed_module: Parses the file's text to a python module, infering the file type from the file extension.

Test Plan

I added a few unit tests that demonstrate the basic functionality.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Jun 10, 2024
@MichaReiser MichaReiser changed the base branch from main to salsa-files-vfs June 10, 2024 11:19
@MichaReiser MichaReiser changed the title red-knot: source_text, line_index, and parsed_module queries red-knot[salsa part 3]: source_text, line_index, and parsed_module queries Jun 11, 2024
@MichaReiser MichaReiser requested review from carljm and AlexWaygood and removed request for dhruvmanila June 11, 2024 13:57
@MichaReiser MichaReiser changed the base branch from salsa-files-vfs to salsa-2-semantic-jar June 11, 2024 14:07
Copy link
Contributor

github-actions bot commented Jun 11, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

mlflow/mlflow (error)

Failed to clone mlflow/mlflow: error: RPC failed; curl 55 Failed sending HTTP2 data
error: 27308 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

crates/ruff_db/src/parsed.rs Outdated Show resolved Hide resolved
let ty = match path {
VfsPath::FileSystem(path) => path
.extension()
.map_or(PySourceType::Python, PySourceType::from_extension),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to assume a file is Python if it has no extension? Is that an existing Ruff behavior? That seems likely to lead to a lot of syntax/parsing errors. From a user perspective, I would expect to have to specially request via config if I want Ruff to treat an unknown-extension file as Python source.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to filter out non-python files (according to the user's configuration) when walking the workspace so that we never end up calling parse on these files.

crates/ruff_db/src/parsed.rs Outdated Show resolved Hide resolved
}

#[cfg(test)]
mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests also for stub file, vendored stub file, and unknown-extension file?

Copy link
Member Author

@MichaReiser MichaReiser Jun 13, 2024

Choose a reason for hiding this comment

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

I added a test for vendored paths. I prefer to not add a test for unknown extensions and stub files because I then end up re-testing PySourceType::from_extension. I rather want to have integration tests for different file types (We write a lot of integration tests in ruff and only few unit tests. What we have in these PRs is already way above the average number of unit tests)

crates/ruff_python_ast/src/lib.rs Show resolved Hide resolved
@carljm
Copy link
Contributor

carljm commented Jun 12, 2024

Oh, and let's make sure to fix the clippy errors before merging this.

crates/ruff_db/src/file_system.rs Show resolved Hide resolved
crates/ruff_db/src/parsed.rs Outdated Show resolved Hide resolved
Base automatically changed from salsa-2-semantic-jar to main June 13, 2024 07:00
@MichaReiser MichaReiser changed the title red-knot[salsa part 3]: source_text, line_index, and parsed_module queries red-knot: source_text, line_index, and parsed_module queries Jun 13, 2024
@MichaReiser MichaReiser enabled auto-merge (squash) June 13, 2024 07:19
@MichaReiser MichaReiser merged commit d4dd96d into main Jun 13, 2024
17 checks passed
@MichaReiser MichaReiser deleted the salsa-source branch June 13, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants