## Summary
Currently, the log messages emitted by the server includes multiple
information which isn't really required most of the time.
Here's the current format:
```
0.000755625s DEBUG main ruff_server::session::index::ruff_settings: Indexing settings for workspace: /Users/dhruv/playground/ruff
0.016334666s DEBUG ThreadId(10) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
0.019954541s INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
0.020160416s TRACE ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
0.020209625s TRACE ruff:worker:0 request{id=1 method="textDocument/diagnostic"}: ruff_server::server::api: enter
0.020228166s DEBUG ruff:worker:0 request{id=1 method="textDocument/diagnostic"}: ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/test.py
0.020359833s INFO ruff:main ruff_server::server: Configuration file watcher successfully registered
```
This PR updates the following:
* Uses current timestamp (same as red-knot) for all log levels instead
of the uptime value
* Includes the target and thread names only at the trace level
What this means is that the message is reduced to only important
information at DEBUG level:
```
2025-02-26 11:35:02.198375000 DEBUG Indexing settings for workspace: /Users/dhruv/playground/ruff
2025-02-26 11:35:02.209933000 DEBUG Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
2025-02-26 11:35:02.217165000 INFO Registering workspace: /Users/dhruv/playground/ruff
2025-02-26 11:35:02.217631000 DEBUG Included path via `include`: /Users/dhruv/playground/ruff/lsp/test.py
2025-02-26 11:35:02.217684000 INFO Configuration file watcher successfully registered
```
while still showing the other information (thread names and target) at
trace level:
```
2025-02-26 11:35:27.819617000 DEBUG main ruff_server::session::index::ruff_settings: Indexing settings for workspace: /Users/dhruv/playground/ruff
2025-02-26 11:35:27.830500000 DEBUG ThreadId(11) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
2025-02-26 11:35:27.837212000 INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
2025-02-26 11:35:27.837714000 TRACE ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
2025-02-26 11:35:27.838019000 INFO ruff:main ruff_server::server: Configuration file watcher successfully registered
2025-02-26 11:35:27.838084000 TRACE ruff:worker:1 request{id=1 method="textDocument/diagnostic"}: ruff_server::server::api: enter
2025-02-26 11:35:27.838205000 DEBUG ruff:worker:1 request{id=1 method="textDocument/diagnostic"}: ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/test.py
```
## Summary
[Internal design
document](https://www.notion.so/astral-sh/In-editor-settings-19e48797e1ca807fa8c2c91b689d9070?pvs=4)
This PR expands `ruff.configuration` to allow inline configuration
directly in the editor. For example:
```json
{
"ruff.configuration": {
"line-length": 100,
"lint": {
"unfixable": ["F401"],
"flake8-tidy-imports": {
"banned-api": {
"typing.TypedDict": {
"msg": "Use `typing_extensions.TypedDict` instead"
}
}
}
},
"format": {
"quote-style": "single"
}
}
}
```
This means that now `ruff.configuration` accepts either a path to
configuration file or the raw config itself. It's _mostly_ similar to
`--config` with one difference that's highlighted in the following
section. So, it can be said that the format of `ruff.configuration` when
provided the config map is same as the one on the [playground] [^1].
## Limitations
<details><summary><b>Casing (<code>kebab-case</code> v/s/
<code>camelCase</code>)</b></summary>
<p>
The config keys needs to be in `kebab-case` instead of `camelCase` which
is being used for other settings in the editor.
This could be a bit confusing. For example, the `line-length` option can
be set directly via an editor setting or can be configured via
`ruff.configuration`:
```json
{
"ruff.configuration": {
"line-length": 100
},
"ruff.lineLength": 120
}
```
#### Possible solution
We could use feature flag with [conditional
compilation](https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg_attr-attribute)
to indicate that when used in `ruff_server`, we need the `Options`
fields to be renamed as `camelCase` while for other crates it needs to
be renamed as `kebab-case`. But, this might not work very easily because
it will require wrapping the `Options` struct and create two structs in
which we'll have to add `#[cfg_attr(...)]` because otherwise `serde`
will complain:
```
error: duplicate serde attribute `rename_all`
--> crates/ruff_workspace/src/options.rs:43:38
|
43 | #[cfg_attr(feature = "editor", serde(rename_all = "camelCase"))]
| ^^^^^^^^^^
```
</p>
</details>
<details><summary><b>Nesting (flat v/s nested keys)</b></summary>
<p>
This is the major difference between `--config` flag on the command-line
v/s `ruff.configuration` and it makes it such that `ruff.configuration`
has same value format as [playground] [^1].
The config keys needs to be split up into keys which can result in
nested structure instead of flat structure:
So, the following **won't work**:
```json
{
"ruff.configuration": {
"format.quote-style": "single",
"lint.flake8-tidy-imports.banned-api.\"typing.TypedDict\".msg": "Use `typing_extensions.TypedDict` instead"
}
}
```
But, instead it would need to be split up like the following:
```json
{
"ruff.configuration": {
"format": {
"quote-style": "single"
},
"lint": {
"flake8-tidy-imports": {
"banned-api": {
"typing.TypedDict": {
"msg": "Use `typing_extensions.TypedDict` instead"
}
}
}
}
}
}
```
#### Possible solution (1)
The way we could solve this and make it same as `--config` would be to
add a manual logic of converting the JSON map into an equivalent TOML
string which would be then parsed into `Options`.
So, the following JSON map:
```json
{ "lint.flake8-tidy-imports": { "banned-api": {"\"typing.TypedDict\".msg": "Use typing_extensions.TypedDict instead"}}}
```
would need to be converted into the following TOML string:
```toml
lint.flake8-tidy-imports = { banned-api = { "typing.TypedDict".msg = "Use typing_extensions.TypedDict instead" } }
```
by recursively convering `"key": value` into `key = value` which is to
remove the quotes from key and replacing `:` with `=`.
#### Possible solution (2)
Another would be to just accept `Map<String, String>` strictly and
convert it into `key = value` and then parse it as a TOML string. This
would also match `--config` but quotes might become a nuisance because
JSON only allows double quotes and so it'll require escaping any inner
quotes or use single quotes.
</p>
</details>
## Test Plan
### VS Code
**Requires https://github.com/astral-sh/ruff-vscode/pull/702**
**`settings.json`**:
```json
{
"ruff.lint.extendSelect": ["TID"],
"ruff.configuration": {
"line-length": 50,
"format": {
"quote-style": "single"
},
"lint": {
"unfixable": ["F401"],
"flake8-tidy-imports": {
"banned-api": {
"typing.TypedDict": {
"msg": "Use `typing_extensions.TypedDict` instead"
}
}
}
}
}
}
```
Following video showcases me doing the following:
1. Check diagnostics that it includes `TID`
2. Run `Ruff: Fix all auto-fixable problems` to test `unfixable`
3. Run `Format: Document` to test `line-length` and `quote-style`
https://github.com/user-attachments/assets/0a38176f-3fb0-4960-a213-73b2ea5b1180
### Neovim
**`init.lua`**:
```lua
require('lspconfig').ruff.setup {
init_options = {
settings = {
lint = {
extendSelect = { 'TID' },
},
configuration = {
['line-length'] = 50,
format = {
['quote-style'] = 'single',
},
lint = {
unfixable = { 'F401' },
['flake8-tidy-imports'] = {
['banned-api'] = {
['typing.TypedDict'] = {
msg = 'Use typing_extensions.TypedDict instead',
},
},
},
},
},
},
},
}
```
Same steps as in the VS Code test:
https://github.com/user-attachments/assets/cfe49a9b-9a89-43d7-94f2-7f565d6e3c9d
## Documentation Preview
https://github.com/user-attachments/assets/e0062f58-6ec8-4e01-889d-fac76fd8b3c7
[playground]: https://play.ruff.rs
[^1]: This has one advantage that the value can be copy-pasted directly
into the playground
## Summary
This PR builds on the changes in #16220 to pass a target Python version
to the parser. It also adds the `Parser::unsupported_syntax_errors` field, which
collects version-related syntax errors while parsing. These syntax
errors are then turned into `Message`s in ruff (in preview mode).
This PR only detects one syntax error (`match` statement before Python
3.10), but it has been pretty quick to extend to several other simple
errors (see #16308 for example).
## Test Plan
The current tests are CLI tests in the linter crate, but these could be
supplemented with inline parser tests after #16357.
I also tested the display of these syntax errors in VS Code:
---------
Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
In https://github.com/astral-sh/ruff/pull/16306#discussion_r1966290700,
@carljm pointed out that #16306 introduced a terminology problem, with
too many things called a "constraint". This is a follow-up PR that
renames `Constraint` to `Predicate` to hopefully clear things up a bit.
So now we have that:
- a _predicate_ is a Python expression that might influence type
inference
- a _narrowing constraint_ is a list of predicates that constraint the
type of a binding that is visible at a use
- a _visibility constraint_ is a ternary formula of predicates that
define whether a binding is visible or a statement is reachable
This is a pure renaming, with no behavioral changes.
## Summary
Model dunder-calls correctly (and in one single place), by implementing
this behavior (using `__getitem__` as an example).
```py
def getitem_desugared(obj: object, key: object) -> object:
getitem_callable = find_in_mro(type(obj), "__getitem__")
if hasattr(getitem_callable, "__get__"):
getitem_callable = getitem_callable.__get__(obj, type(obj))
return getitem_callable(key)
```
See the new `calls/dunder.md` test suite for more information. The new
behavior also needs much fewer lines of code (the diff is positive due
to new tests).
## Test Plan
New tests; fix TODOs in existing tests.
This PR adds an implementation of [association
lists](https://en.wikipedia.org/wiki/Association_list), and uses them to
replace the previous `BitSet`/`SmallVec` representation for narrowing
constraints.
An association list is a linked list of key/value pairs. We additionally
guarantee that the elements of an association list are sorted (by their
keys), and that they do not contain any entries with duplicate keys.
Association lists have fallen out of favor in recent decades, since you
often need operations that are inefficient on them. In particular,
looking up a random element by index is O(n), just like a linked list;
and looking up an element by key is also O(n), since you must do a
linear scan of the list to find the matching element. Luckily we don't
need either of those operations for narrowing constraints!
The typical implementation also suffers from poor cache locality and
high memory allocation overhead, since individual list cells are
typically allocated separately from the heap. We solve that last problem
by storing the cells of an association list in an `IndexVec` arena.
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
I am working on a project that uses ruff linters' docs to generate a
fine-tuning dataset for LLMs.
To achieve this, I first ran the command `ruff rule --all
--output-format json` to retrieve all the rules. Then, I parsed the
explanation field to get these 3 consistent sections:
- `Why is this bad?`
- `What it does`
- `Example`
However, during the initial processing, I noticed that the markdown
headings are not that consistent. For instance:
- In most cases, `Use instead` appears as a normal paragraph within the
`Example` section, but in the file
`crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs` it is
a level-2 heading
- The heading "What it does**?**" is used in some places, while others
consistently use "What it does"
- There are 831 `Example` headings and 65 `Examples`. But all of them
only have one example case
This PR normalized these across all rules.
## Test Plan
CI are passed.
## Summary
Add a diagnostic if a pure instance variable is accessed on a class object. For example
```py
class C:
instance_only: str
def __init__(self):
self.instance_only = "a"
# error: Attribute `instance_only` can only be accessed on instances, not on the class object `Literal[C]` itself.
C.instance_only
```
---------
Co-authored-by: David Peter <mail@david-peter.de>
## Summary
This PR is another step in preparing to detect syntax errors in the
parser. It introduces the new `per-file-target-version` top-level
configuration option, which holds a mapping of compiled glob patterns to
Python versions. I intend to use the
`LinterSettings::resolve_target_version` method here to pass to the
parser:
f50849aeef/crates/ruff_linter/src/linter.rs (L491-L493)
## Test Plan
I added two new CLI tests to show that the `per-file-target-version` is
respected in both the formatter and the linter.
## Summary
Add support for `@classmethod`s.
```py
class C:
@classmethod
def f(cls, x: int) -> str:
return "a"
reveal_type(C.f(1)) # revealed: str
```
## Test Plan
New Markdown tests
## Summary
* Existing example did not include RawSQL() call like it should
* Also clarify the example a bit to make it clearer that the code is not
secure
## Test Plan
N/A, only documentation updated
## Summary
I spotted a minor mistake in my descriptor protocol implementation where
`C.descriptor` would pass the meta type (`type`) of the type of `C`
(`Literal[C]`) as the owner argument to `__get__`, instead of passing
`Literal[C]` directly.
## Test Plan
New test.
Two related changes. For context:
1. We were maintaining two separate arenas of `Constraint`s in each
use-def map. One was used for narrowing constraints, and the other for
visibility constraints. The visibility constraint arena was interned,
ensuring that we always used the same ID for any particular
`Constraint`. The narrowing constraint arena was not interned.
2. The TDD code relies on _all_ TDD nodes being interned and reduced.
This is an important requirement for TDDs to be a canonical form, which
allows us to use a single int comparison to test for "always true/false"
and to compare two TDDs for equivalence. But we also need to support an
individual `Constraint` having multiple values in a TDD evaluation (e.g.
to handle a `while` condition having different values the first time
it's evaluated vs later times). Previously, we handled that by
introducing a "copy" number, which was only there as a disambiguator, to
allow an interned, deduplicated constraint ID to appear in the TDD
formula multiple times.
A better way to handle (2) is to not intern the constraints in the
visibility constraint arena! The caller now gets to decide: if they add
a `Constraint` to the arena more than once, they get distinct
`ScopedConstraintId`s — which the TDD code will treat as distinct
variables, allowing them to take on different values in the ternary
function.
With that in place, we can then consolidate on a single (non-interned)
arena, which is shared for both narrowing and visibility constraints.
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
## Summary
Resolves 3/4 requests in #16217:
- ✅ Remove not special methods: `__cmp__`, `__div__`, `__nonzero__`, and
`__unicode__`.
- ✅ Add special methods: `__next__`, `__buffer__`, `__class_getitem__`,
`__mro_entries__`, `__release_buffer__`, and `__subclasshook__`.
- ✅ Support positional-only arguments.
- ❌ Add support for module functions `__dir__` and `__getattr__`. As
mentioned in the issue the check is scoped for methods rather than
module functions. I am hesitant to expand the scope of this check
without a discussion.
## Test Plan
- Manually confirmed each example file from the issue functioned as
expected.
- Ran cargo nextest to ensure `unexpected_special_method_signature` test
still passed.
Fixes#16217.
## Summary
This is just a small refactor to move workspace related structs and impl
out from `server.rs` where `Server` is defined and into a new
`workspace.rs`.
## Summary
This PR achieves the following:
* Add support for checking method calls, and inferring return types from
method calls. For example:
```py
reveal_type("abcde".find("abc")) # revealed: int
reveal_type("foo".encode(encoding="utf-8")) # revealed: bytes
"abcde".find(123) # error: [invalid-argument-type]
class C:
def f(self) -> int:
pass
reveal_type(C.f) # revealed: <function `f`>
reveal_type(C().f) # revealed: <bound method: `f` of `C`>
C.f() # error: [missing-argument]
reveal_type(C().f()) # revealed: int
```
* Implement the descriptor protocol, i.e. properly call the `__get__`
method when a descriptor object is accessed through a class object or an
instance of a class. For example:
```py
from typing import Literal
class Ten:
def __get__(self, instance: object, owner: type | None = None) ->
Literal[10]:
return 10
class C:
ten: Ten = Ten()
reveal_type(C.ten) # revealed: Literal[10]
reveal_type(C().ten) # revealed: Literal[10]
```
* Add support for member lookup on intersection types.
* Support type inference for `inspect.getattr_static(obj, attr)` calls.
This was mostly used as a debugging tool during development, but seems
more generally useful. It can be used to bypass the descriptor protocol.
For the example above:
```py
from inspect import getattr_static
reveal_type(getattr_static(C, "ten")) # revealed: Ten
```
* Add a new `Type::Callable(…)` variant with the following sub-variants:
* `Type::Callable(CallableType::BoundMethod(…))` — represents bound
method objects, e.g. `C().f` above
* `Type::Callable(CallableType::MethodWrapperDunderGet(…))` — represents
`f.__get__` where `f` is a function
* `Type::Callable(WrapperDescriptorDunderGet)` — represents
`FunctionType.__get__`
* Add new known classes:
* `types.MethodType`
* `types.MethodWrapperType`
* `types.WrapperDescriptorType`
* `builtins.range`
## Performance analysis
On this branch, we do more work. We need to do more call checking, since
we now check all method calls. We also need to do ~twice as many member
lookups, because we need to check if a `__get__` attribute exists on
accessed members.
A brief analysis on `tomllib` shows that we now call `Type::call` 1780
times, compared to 612 calls before.
## Limitations
* Data descriptors are not yet supported, i.e. we do not infer correct
types for descriptor attribute accesses in `Store` context and do not
check writes to descriptor attributes. I felt like this was something
that could be split out as a follow-up without risking a major
architectural change.
* We currently distinguish between `Type::member` (with descriptor
protocol) and `Type::static_member` (without descriptor protocol). The
former corresponds to `obj.attr`, the latter corresponds to
`getattr_static(obj, "attr")`. However, to model some details correctly,
we would also need to distinguish between a static member lookup *with*
and *without* instance variables. The lookup without instance variables
corresponds to `find_name_in_mro`
[here](https://docs.python.org/3/howto/descriptor.html#invocation-from-an-instance).
We currently approximate both using `member_static`, which leads to two
open TODOs. Changing this would be a larger refactoring of
`Type::own_instance_member`, so I chose to leave it out of this PR.
## Test Plan
* New `call/methods.md` test suite for method calls
* New tests in `descriptor_protocol.md`
* New `call/getattr_static.md` test suite for `inspect.getattr_static`
* Various updated tests
## Summary
This avoids looking up `__bool__` on class `bool` for every
`Type::Instance(bool).bool()` call. 1% performance win on cold cache, 4%
win on incremental performance.
This updates the `SymbolBindings` and `SymbolDeclarations` types to use
a single smallvec of live bindings/declarations, instead of splitting
that out into separate containers for each field.
I'm seeing an 11-13% `cargo bench` performance improvement with this
locally (for both cold and incremental). I'm interested to see if
Codspeed agrees!
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
A minor cleanup that breaks up a `HashMap` of an enum into separate
`HashMap`s for each variant. (These separate fields were already how
this cache was being described in the big comment at the top of the
file!)
This is a small tweak to avoid adding the callable `Type` on the error
value itself. Namely, it's always available regardless of the error, and
it's easy to pass it down explicitly to the diagnostic generating code.
It's likely that the other `CallBindingError` variants will also want
the callable `Type` to improve diagnostics too. This way, we don't have
to duplicate the `Type` on each variant. It's just available to all of
them.
Ref https://github.com/astral-sh/ruff/pull/16239#discussion_r1962352646
## Summary
Follow up on the discussion
[here](https://github.com/astral-sh/ruff/pull/16121#discussion_r1962973298).
Replace builtin classes with custom placeholder names, which should
hopefully make the tests a bit easier to understand.
I carefully renamed things one after the other, to make sure that there
is no functional change in the tests.
## Summary
This PR should help in
https://github.com/astral-sh/ruff-vscode/issues/676.
There are two issues that this is trying to fix all related to the way
shutdown should happen as per the protocol:
1. After the server handled the [shutdown
request](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#shutdown)
and while waiting for the exit notification:
> If a server receives requests after a shutdown request those requests
should error with `InvalidRequest`.
But, we raised an error and exited. This PR fixes it by entering a loop
which responds to any request during this period with `InvalidRequest`
2. If the server received an [exit
notification](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit)
but the shutdown request was never received, the server handled that by
logging and exiting with success but as per the spec:
> The server should exit with success code 0 if the shutdown request has
been received before; otherwise with error code 1.
So, this PR fixes that as well by raising an error in this case.
## Test Plan
I'm not sure how to go about testing this without using a mock server.
## Summary
This is part of the preparation for detecting syntax errors in the
parser from https://github.com/astral-sh/ruff/pull/16090/. As suggested
in [this
comment](https://github.com/astral-sh/ruff/pull/16090/#discussion_r1953084509),
I started working on a `ParseOptions` struct that could be stored in the
parser. For this initial refactor, I only made it hold the existing
`Mode` option, but for syntax errors, we will also need it to have a
`PythonVersion`. For that use case, I'm picturing something like a
`ParseOptions::with_python_version` method, so you can extend the
current calls to something like
```rust
ParseOptions::from(mode).with_python_version(settings.target_version)
```
But I thought it was worth adding `ParseOptions` alone without changing
any other behavior first.
Most of the diff is just updating call sites taking `Mode` to take
`ParseOptions::from(Mode)` or those taking `PySourceType`s to take
`ParseOptions::from(PySourceType)`. The interesting changes are in the
new `parser/options.rs` file and smaller parts of `parser/mod.rs` and
`ruff_python_parser/src/lib.rs`.
## Test Plan
Existing tests, this should not change any behavior.
We now resolve references in "eager" scopes correctly — using the
bindings and declarations that are visible at the point where the eager
scope is created, not the "public" type of the symbol (typically the
bindings visible at the end of the scope).
---------
Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
This uses the refactoring and support for secondary diagnostic messages
to improve the diagnostic for "invalid argument type." The main
improvement here is that we show where the function being called is
defined, and annotate the span corresponding to the invalid parameter.
This is a small little hack to make the `Diagnostic` trait
capable of supporting attaching multiple spans.
This design should be considered transient. This was just the
quickest way that I could see to pass multiple spans through from
the type checker to the diagnostic renderer.
This commit has no behavioral changes.
This refactor moves the logic for turning a `D: Diagnostic` into
an `annotate_snippets::Message` into its own types. This would
ideally just be a function or something, but the `annotate-snippets`
types want borrowed data, and sometimes we need to produce owned
data. So we gather everything we need into our own types and then
spit it back out in the format that `annotate-snippets` wants.
This factor was motivated by wanting to render multiple snippets.
The logic for generating a code frame is complicated enough that
it's worth splitting out so that we can reuse it for other spans.
(Note that one should consider this prototype-level code. It is
unlikely to survive for long.)
It seems nothing is using it, and I'm not sure if it makes semantic
sense. Particularly if we want to support multiple ranges. One could
make an argument that this ought to correspond to the "primary"
range (which we should have), but I think such a concept is better
expressed as an explicit routine if possible.
## Summary
Resolves#15979.
The file explains what Red Knot is (a type checker), what state it is in
(not yet ready for user testing), what its goals ("extremely fast") and
non-goals (not a drop-in replacement for other type checkers) are as
well as what the crates contain.
## Test Plan
None.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Move class attribute (property, methods, variables) related cases in
AIR302_names to AIR302_class_attribute
## Test Plan
No functionality change. Test fixture is reogranized
Fixes false negative when slice bound uses length of string literal.
We were meant to check the following, for example. Given:
```python
text[:bound] if text.endswith(suffix) else text
```
We want to know whether:
- `suffix` is a string literal and `bound` is a number literal
- `suffix` is an expression and `bound` is
exactly `-len(suffix)` (as AST nodes, prior to evaluation.)
The issue is that negative number literals like `-10` are stored as
unary operators applied to a number literal in the AST. So when `suffix`
was a string literal but `bound` was `-len(suffix)` we were getting
caught in the match arm where `bound` needed to be a number. This is now
fixed with a guard.
Closes#16231
## Summary
This PR updates the formatter and linter to use the `PythonVersion`
struct from the `ruff_python_ast` crate internally. While this doesn't
remove the need for the `linter::PythonVersion` enum, it does remove the
`formatter::PythonVersion` enum and limits the use in the linter to
deserializing from CLI arguments and config files and moves most of the
remaining methods to the `ast::PythonVersion` struct.
## Test Plan
Existing tests, with some inputs and outputs updated to reflect the new
(de)serialization format. I think these are test-specific and shouldn't
affect any external (de)serialization.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
This PR updates the `ruff.printDebugInformation` command to return the
info as string in the response. Currently, we send a `window/logMessage`
request with the info but that has the disadvantage that it's not
visible to the user directly.
What `rust-analyzer` does with it's `rust-analyzer/status` request which
returns it as a string which then the client can just display it in a
separate window. This is what I'm thinking of doing as well.
Other editors can also benefit from it by directly opening a temporary
file with this information that the user can see directly.
There are couple of options here:
1. Keep using the command, keep the log request and return the string
2. Keep using the command, remove the log request and return the string
3. Create a new request similar to `rust-analyzer/status` which returns
a string
This PR implements (1) but I'd want to move towards (2) and remove the
log request completely. We haven't advertised it as such so this would
only require updating the VS Code extension to handle it by opening a
new document with the debug content.
## Test plan
For VS Code, refer to https://github.com/astral-sh/ruff-vscode/pull/694.
For Neovim, one could do:
```lua
local function execute_ruff_command(command)
local client = vim.lsp.get_clients({
bufnr = vim.api.nvim_get_current_buf(),
name = name,
method = 'workspace/executeCommand',
})[1]
if not client then
return
end
client.request('workspace/executeCommand', {
command = command,
arguments = {
{ uri = vim.uri_from_bufnr(0) }
},
function(err, result)
if err then
-- log error
return
end
vim.print(result)
-- Or, open a new window with the `result` content
end
}
```
## Summary
Separate ImportPathMoved and ProviderName to avoid misusing (AIR303)
## Test Plan
only code arrangement is updated. existing test fixture should be not be
changed
## Summary
Related to https://github.com/astral-sh/ruff-vscode/pull/686, this PR
ignores handling source code actions for notebooks which are not
prefixed with `notebook`.
The main motivation is that the native server does not actually handle
it well which results in gibberish code. There's some context about this
in
https://github.com/astral-sh/ruff-vscode/issues/680#issuecomment-2647490812
and the following comments.
closes: https://github.com/astral-sh/ruff-vscode/issues/680
## Test Plan
Running a notebook with the following does nothing except log the
message:
```json
"notebook.codeActionsOnSave": {
"source.organizeImports.ruff": "explicit",
},
```
while, including the `notebook` code actions does make the edit (as
usual):
```json
"notebook.codeActionsOnSave": {
"notebook.source.organizeImports.ruff": "explicit"
},
```
## Summary
This PR does the following:
* Moves the following from `types.rs` in `symbol.rs`:
* `symbol`
* `global_symbol`
* `imported_symbol`
* `symbol_from_bindings`
* `symbol_from_declarations`
* `SymbolAndQualifiers`
* `SymbolFromDeclarationsResult`
* Moves the following from `stdlib.rs` in `symbol.rs` and removes
`stdlib.rs`:
* `known_module_symbol`
* `builtins_symbol`
* `typing_symbol` (only for tests)
* `typing_extensions_symbol`
* `builtins_module_scope`
* `core_module_scope`
* Add `symbol_from_bindings_impl` and `symbol_from_declarations_impl` to
keep `RequiresExplicitReExport` an implementation detail
* Make `declaration_type` a `pub(crate)` as it's required in
`symbol_from_declarations` (`binding_type` is already `pub(crate)`
The main motivation is to keep the implementation details private and
only expose an ergonomic API which uses sane defaults for various
scenario to avoid any mistakes from the caller. Refer to
https://github.com/astral-sh/ruff/pull/16133#discussion_r1955262772,
https://github.com/astral-sh/ruff/pull/16133#issue-2850146612 for
details.
## Summary
This PR makes the following changes:
- It adjusts various callsites to use the new
`ast::StringLiteral::contents_range()` method that was introduced in
https://github.com/astral-sh/ruff/pull/16183. This is less verbose and
more type-safe than using the `ast::str::raw_contents()` helper
function.
- It adds a new `ast::ExprStringLiteral::as_unconcatenated_literal()`
helper method, and adjusts various callsites to use it. This addresses
@MichaReiser's review comment at
https://github.com/astral-sh/ruff/pull/16183#discussion_r1957334365.
There is no functional change here, but it helps readability to make it
clearer that we're differentiating between implicitly concatenated
strings and unconcatenated strings at various points.
- It renames the `StringLiteralValue::flags()` method to
`StringLiteralFlags::first_literal_flags()`. If you're dealing with an
implicitly concatenated string `string_node`,
`string_node.value.flags().closer_len()` could give an incorrect result;
this renaming makes it clearer that the `StringLiteralFlags` instance
returned by the method is only guaranteed to give accurate information
for the first `StringLiteral` contained in the `ExprStringLiteral` node.
- It deletes the unused `BytesLiteralValue::flags()` method. This seems
prone to misuse in the same way as `StringLiteralValue::flags()`: if
it's an implicitly concatenated bytestring, the `BytesLiteralFlags`
instance returned by the method would only give accurate information for
the first `BytesLiteral` in the bytestring.
## Test Plan
`cargo test`
On `main` we warn the user if there is an invalid noqa comment[^1] and
at least one of the following holds:
- There is at least one diagnostic
- A lint rule related to `noqa`s is enabled (e.g. `RUF100`)
This is probably strange behavior from the point of view of the user, so
we now show invalid `noqa`s even when there are no diagnostics.
Closes#12831
[^1]: For the current definition of "invalid noqa comment", which may be
expanded in #12811 . This PR is independent of loc. cit. in the sense
that the CLI warnings should be consistent, regardless of which `noqa`
comments are considered invalid.
## Summary
Fixes#16189.
Only `sys.breakpointhook` is flagged by the upstream linter:
007a745c86/pylint/checkers/stdlib.py (L38)
but I think it makes sense to flag
[`__breakpointhook__`](https://docs.python.org/3/library/sys.html#sys.__breakpointhook__)
too, as suggested in the issue because it
> contain[s] the original value of breakpointhook [...] in case [it
happens] to get replaced with broken or alternative objects.
## Test Plan
New T100 test cases
## Summary
Provides documentation about the FIPS compliant flag for Python hashlib
`usedforsecurity`
Fixes#16188
## Test Plan
* pre-commit hooks
---------
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
## Summary
Running `cargo test -p red_knot_python_semantic` failed because of a
missing serde feature. This PR enables the `ruff_python_ast`'`s `serde`
if the crate's `serde` feature is enabled
## Test Plan
`cargo test -p red_knot_python_semantic` compiles again
When adjusting the existing tests, I aimed to avoid dealing with the
special case in other tests if it's not necessary to do so (that is,
avoid using `float` and `complex` as examples where we just need "some
type"), and keep the tests for the special case mostly collected in the
mdtest dedicated to that purpose.
Fixes https://github.com/astral-sh/ruff/issues/14932
## Summary
Added checks for subscript expressions on builtin classes as in FURB189.
The object is changed to use the collections objects and the types from
the subscript are kept.
Resolves#16130
> Note: Added some comments in the code explaining why
## Test Plan
- Added a subscript dict and list class to the test file.
- Tested locally to check that the symbols are changed and the types are
kept.
- No modifications changed on optional `str` values.
## Summary
This PR moves the `PythonVersion` struct from the
`red_knot_python_semantic` crate to the `ruff_python_ast` crate so that
it can be used more easily in the syntax error detection work. Compared
to that [prototype](https://github.com/astral-sh/ruff/pull/16090/) these
changes reduce us from 2 `PythonVersion` structs to 1.
This does not unify any of the `PythonVersion` *enums*, but I hope to
make some progress on that in a follow-up.
## Test Plan
Existing tests, this should not change any external behavior.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
This change begins to resolve#16071 by moving the `OperatorPrecedence`
structs from the `ruff_python_linter` crate into `ruff_python_ast`. This
PR also implements `precedence()` methods on the `Expr` and `ExprRef`
enums.
## Test Plan
Since this change mainly shifts existing logic, I didn't add any
additional tests. Existing tests do pass.
## Summary
This PR refactors the symbol lookup APIs to better facilitate the
re-export implementation. Specifically,
* Add `module_type_symbol` which returns the `Symbol` that's a member of
`types.ModuleType`
* Rename `symbol` -> `symbol_impl`; add `symbol` which delegates to
`symbol_impl` with `RequireExplicitReExport::No`
* Update `global_symbol` to do `symbol_impl` -> fall back to
`module_type_symbol` and default to `RequireExplicitReExport::No`
* Add `imported_symbol` to do `symbol_impl` with
`RequireExplicitReExport` as `Yes` if the module is in a stub file else
`No`
* Update `known_module_symbol` to use `imported_symbol` with a fallback
to `module_type_symbol`
* Update `ModuleLiteralType::member` to use `imported_symbol` with a
custom fallback
We could potentially also update `symbol_from_declarations` and
`symbol_from_bindings` to avoid passing in the `RequireExplicitReExport`
as it would be always `No` if called directly. We could add
`symbol_from_declarations_impl` and `symbol_from_bindings_impl`.
Looking at the `_impl` functions, I think we should move all of these
symbol related logic into `symbol.rs` where `Symbol` is defined and the
`_impl` could be private while we expose the public APIs at the crate
level. This would also make the `RequireExplicitReExport` an
implementation detail and the caller doesn't need to worry about it.
This is an alternative implementation to #15848.
## Summary
This PR adds support for re-export conventions for imports for stub
files.
**How does this work?**
* Add a new flag on the `Import` and `ImportFrom` definitions to
indicate whether they're being exported or not
* Add a new enum to indicate whether the symbol lookup is happening
within the same file or is being queried from another file (e.g., an
import statement)
* When a `Symbol` is being queried, we'll skip the definitions that are
(a) coming from a stub file (b) external lookup and (c) check the
re-export flag on the definition
This implementation does not yet support `__all__` and `*` imports as
both are features that needs to be implemented independently.
closes: #14099closes: #15476
## Test Plan
Add test cases, update existing ones if required.
## Summary
Resolves#15859.
The rule now adds parentheses if the original call wraps an unary
expression and is:
* The left-hand side of a binary expression where the operator is `**`.
* The caller of a call expression.
* The subscripted of a subscript expression.
* The object of an attribute access.
The fix will also be marked as unsafe if there are any comments in its
range.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
Resolves#13294, follow-up to #13882.
At #13882, it was concluded that a fix should not be offered for raw
strings. This change implements that. The five rules in question are now
no longer always fixable.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Follow-up to https://github.com/astral-sh/ruff/pull/15951 to update
* the options links in A005 to reference
`lint.flake8-builtins.builtins-strict-checking`
* the description of the rule to explain strict vs non-strict checking
* the option documentation to point back to the rule
For now, the only thing one can configure is whether color is enabled or
not. This avoids needing to ask the `colored` crate whether colors have
been globally enabled or disabled. And, more crucially, avoids the need
to _set_ this global flag for testing diagnostic output. Doing so can
have unintended consequences, as outlined in #16115.
Fixes#16115
## Summary
Add support for the `project.requires-python` field in `pyproject.toml`
files.
Fall back to the resolved lower bound of `project.requires-python` if
the `environment.python-version` field is `None` (or more accurately,
initialize `environment.python-version with `requires-python`'s lower
bound if left unspecified).
## UX design
There are two options on how we can handle the fallback to
`requires-python`'s lower bound:
1. Store the resolved lower bound in `environment.python-version` if
that field is `None` (Implemented in this PR)
2. Store the `requires-python` constraint separately.
There's no observed difference unless a user-level configuration (or any
other inherited configuration is used). Let's discuss it on the given
example
**User configuration**
```toml
[environment]
python-version = "3.10"
```
**Project configuration (`pyproject.toml`)**
```toml
[project]
name = "test"
requires-python = ">= 3.12"
[tool.knot]
# No environment table
```
The resolved version for 1. is 3.12 because the `requires-python`
constraint precedence takes precedence over the `python-version` in the
user configuration. 2. resolves to 3.10 because all `python-version`
constraints take precedence before falling back to `requires-python`.
Ruff implements 1. It's also the easier to implement and it does seem
intuitive to me that the more local `requires-python` constraint takes
precedence.
## Test plan
Added CLI and unit tests.
The PR addresses the issue #16040 .
---
The logic used into the rule is the following:
Suppose to have an expression of the form
```python
if a cmp b:
c = d
```
where `a`,` b`, `c` and `d` are Python obj and `cmp` one of `<`, `>`,
`<=`, `>=`.
Then:
- `if a=c and b=d`
- if `<=` fix with `a = max(b, a)`
- if `>=` fix with `a = min(b, a)`
- if `>` fix with `a = min(a, b)`
- if `<` fix with `a = max(a, b)`
- `if a=d and b=c`
- if `<=` fix with `b = min(a, b)`
- if `>=` fix with `b = max(a, b)`
- if `>` fix with `b = max(b, a)`
- if `<` fix with `b = min(b, a)`
- do nothing, i.e., we cannot fix this case.
---
In total we have 8 different and possible cases.
```
| Case | Expression | Fix |
|-------|------------------|---------------|
| 1 | if a >= b: a = b | a = min(b, a) |
| 2 | if a <= b: a = b | a = max(b, a) |
| 3 | if a <= b: b = a | b = min(a, b) |
| 4 | if a >= b: b = a | b = max(a, b) |
| 5 | if a > b: a = b | a = min(a, b) |
| 6 | if a < b: a = b | a = max(a, b) |
| 7 | if a < b: b = a | b = min(b, a) |
| 8 | if a > b: b = a | b = max(b, a) |
```
I added them in the tests.
Please double-check that I didn't make any mistakes. It's quite easy to
mix up > and <.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
After I was asked twice within the same day, I thought it would be a
good idea to write some *user facing* documentation that explains our
reasoning behind inferring `Unknown | T_inferred` for public uses of
undeclared symbols. This is a major deviation from the behavior of other
type checkers and it seems like a good practice to defend our choice
like this.
## Summary
* fix ImportPathMoved / ProviderName misuse
* oncrete names, such as `["airflow", "config_templates",
"default_celery", "DEFAULT_CELERY_CONFIG"]`, should use `ProviderName`.
In contrast, module paths like `"airflow", "operators", "weekday", ...`
should use `ImportPathMoved`. Misuse may lead to incorrect detection.
## Test Plan
update test fixture
This essentially makes it impossible to construct a `Diagnostic`
that has a `TextRange` but no `File`.
This is meant to be a precursor to multi-span support.
(Note that I consider this more of a prototyping-change and not
necessarily what this is going to look like longer term.)
Reviewers can probably review this PR as one big diff instead of
commit-by-commit.
## Summary
This is a follow up to
https://github.com/astral-sh/ruff/pull/15763#discussion_r1949681336
It reverts the change to using ptr equality for `AstNodeRef`s, which in
turn removes the `Eq`, `PartialEq`, and `Hash` implementations for
`AstNodeRef`s parametrized with AST nodes.
Cheap comparisons shouldn't be needed because the node field is
generally marked as `[#tracked]` and `#[no_eq]` and removing the
implementations even enforces that those
attributes are set on all `AstNodeRef` fields (which is good).
The only downside this has is that we technically wouldn't have to mark
the `Unpack::target` as `#[tracked]` because
the `target` field is accessed in every query accepting `Unpack` as an
argument.
Overall, enforcing the use of `#[tracked]` seems like a good trade off,
espacially considering that it's very likely that
we'd probably forget to mark the `Unpack::target` field as tracked if we
add a new `Unpack` query that doesn't access the target.
## Test Plan
`cargo test`
## Summary
Fixes#16007. The logic from the last fix for this (#9427) was
sufficient, it just wasn't being applied because `Attributes` sections
aren't expected to have nested sections. I just deleted the outer
conditional, which should hopefully fix this for all section types.
## Test Plan
New regression test, plus the existing D417 tests.
## Summary
Transition to using coarse-grained tracked structs (depends on
https://github.com/salsa-rs/salsa/pull/657). For now, this PR doesn't
add any `#[tracked]` fields, meaning that any changes cause the entire
struct to be invalidated. It also changes `AstNodeRef` to be
compared/hashed by pointer address, instead of performing a deep AST
comparison.
## Test Plan
This yields a 10-15% improvement on my machine (though weirdly some runs
were 5-10% without being flagged as inconsistent by criterion, is there
some non-determinism involved?). It's possible that some of this is
unrelated, I'll try applying the patch to the current salsa version to
make sure.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
Resolves#16082.
`UP036` will now also take into consideration whether or not a micro
version number is set:
* If a third element doesn't exist, the existing logic is preserved.
* If it exists but is not an integer literal, the check will not be
reported.
* If it is an integer literal but doesn't fit into a `u8`, the check
will be reported as invalid.
* Otherwise, the compared version is determined to always be less than
the target version when:
* The target's minor version is smaller than that of the comparator, or
* The operator is `<`, the micro version is 0, and the two minor
versions compare equal.
As this is considered a bugfix, it is not preview-gated.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
- Simplify unions with `object` to `object`.
- Add a new `Type::object(db)` constructor to abbreviate
`KnownClass::Object.to_instance(db)` in some places.
- Add a `Type::is_object` and `Class::is_object` function to make some
tests for a bit easier to read.
closes#16084
## Test Plan
New Markdown tests.
The index in subscript access like `d[*y]` will not be linted or
autofixed with parentheses, even when
`lint.ruff.parenthesize-tuple-in-subscript = true`.
Closes#16077
## Summary
This PR adds support for user-level configurations
(`~/.config/knot/knot.toml`) to Red Knot.
Red Knot will watch the user-level configuration file for changes but
only if it exists
when the process start. It doesn't watch for new configurations,
mainly to simplify things for now (it would require watching the entire
`.config` directory because the `knot` subfolder might not exist
either).
The new `ConfigurationFile` struct seems a bit overkill for now but I
plan to use it for
hierarchical configurations as well.
Red Knot uses the same strategy as uv and Ruff by using the etcetera
crate.
## Test Plan
Added CLI and file watching test
## Summary
This PR adds a new `user_configuration_directory` method to `System`. We
need it to resolve where to lookup a user-level `knot.toml`
configuration file.
The method belongs to `System` because not all platforms have a
convention of where to store such configuration files (e.g. wasm).
I refactored `TestSystem` to be a simple wrapper around an `Arc<dyn
System...>` and use the `System.as_any` method instead to cast it down
to an `InMemory` system. I also removed some `System` specific methods
from `InMemoryFileSystem`, they don't belong there.
This PR removes the `os` feature as a default feature from `ruff_db`.
Most crates depending on `ruff_db` don't need it because they only
depend on `System` or only depend on `os` for testing. This was
necessary to fix a compile error with `red_knot_wasm`
## Test Plan
I'll make use of the method in my next PR. So I guess we won't know if
it works before then but I copied the code from Ruff/uv, so I have high
confidence that it is correct.
`cargo test`
## Summary
This PR generalize the idea that we may want to emit diagnostics for
invalid or incompatible configuration values similar to how we already
do it for `rules`.
This PR introduces a new `Settings` struct that is similar to `Options`
but, unlike
`Options`, are fields have their default values filled in and they use a
representation optimized for reads.
The diagnostics created during loading the `Settings` are stored on the
`Project` so that we can emit them when calling `check`.
The motivation for this work is that it simplifies adding new settings.
That's also why I went ahead and added the `terminal.error-on-warning`
setting to demonstrate how new settings are added.
## Test Plan
Existing tests, new CLI test.
This PR resolved#15772
Before PR:
```
def _(
this_is_fine: int = f(), # No error
this_is_not: list[int] = f() # B008: Do not perform function call `f` in argument defaults
): ...
@dataclass
class _:
this_is_not_fine: list[int] = f() # RUF009: Do not perform function call `f` in dataclass defaults
this_is_also_not: int = f() # RUF009: Do not perform function call `f` in dataclass defaults
```
After PR:
```
def _(
this_is_fine: int = f(), # No error
this_is_not: list[int] = f() # B008: Do not perform function call `f` in argument defaults
): ...
@dataclass
class _:
this_is_not_fine: list[int] = f() # RUF009: Do not perform function call `f` in dataclass defaults
this_is_fine: int = f()
```
## Summary
Follow-up to #15984.
Previously, `PLE1310` would only report when the object is a literal:
```python
'a'.strip('//') # error
foo = ''
foo.strip('//') # no error
```
After this change, objects whose type can be inferred to be either `str`
or `bytes` will also be reported in preview.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
fixes: #16041
## Test Plan
Using the [project](https://github.com/bwcc-clan/polebot) in the linked
issue:
Notice how the project "polebot" is in the "play" directory which is
included in the `exclude` setting as:
```toml
exclude = ["play"]
```
**Before this fix**
```
DEBUG ruff:worker:0 ruff_server::resolve: Ignored path via `exclude`: /private/tmp/ruff-test/play/polebot/src/utils/log_tools.py
```
**After this fix**
```
DEBUG ruff:worker:2 ruff_server::resolve: Included path via `include`: /private/tmp/ruff-test/play/polebot/src/utils/log_tools.py
```
I also updated the same project to remove the "play" directory from the
`exclude` setting and made sure that anything under the `polebot/play`
directory is included:
```
DEBUG ruff:worker:4 ruff_server::resolve: Included path via `include`: /private/tmp/ruff-test/play/polebot/play/test.py
```
And, excluded when I add the directory back:
```
DEBUG ruff:worker:2 ruff_server::resolve: Ignored path via `exclude`: /private/tmp/ruff-test/play/polebot/play/test.py
```
## Summary
This PR refactors the `RuffSettings` struct to directly include the
resolved `Settings` instead of including the specific fields from it.
The server utilizes a lot of it already, so it makes sense to just
include the entire struct for simplicity.
### `Deref`
I implemented `Deref` on `RuffSettings` to return the `Settings` because
`RuffSettings` is now basically a wrapper around it with the config path
as the other field. This path field is only used for debugging
("printDebugInformation" command).
## Summary
This PR adds the configuration option
`lint.flake8-builtins.builtins-strict-checking`, which is used in A005
to determine whether the fully-qualified module name (relative to the
project root or source directories) should be checked instead of just
the final component as is currently the case.
As discussed in
https://github.com/astral-sh/ruff/issues/15399#issuecomment-2587017147,
the default value of the new option is `false` on preview, so modules
like `utils.logging` from the initial report are no longer flagged by
default. For non-preview the default is still strict checking.
## Test Plan
New A005 test module with the structure reported in #15399.
Fixes#15399
## Summary
Resolves#12321.
The physical-line-based `RUF054` checks for form feed characters that
are preceded by only tabs and spaces, but not any other characters,
including form feeds.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
No functional change here; this is another simplification split out from
my outcome-refactor branch to reduce the diff there. This merges
`TypeInferenceBuilder::infer_name_load` and
`TypeInferenceBuilder::lookup_name`. This removes the need to have
extensive doc-comments about the purpose of
`TypeInferenceBuilder::lookup_name`, since the method only makes sense
when called from the specific context of
`TypeInferenceBuilder::infer_name_load`.
## Test Plan
`cargo test -p red_knot_python_semantic`
Fixes#16024
## Summary
This PR adds proper isolation for `UP049` fixes so that two type
parameters are not renamed to the same name, which would introduce
invalid syntax. E.g. for this:
```py
class Foo[_T, __T]: ...
```
we cannot apply two autofixes to the class, as that would produce
invalid syntax -- this:
```py
class Foo[T, T]: ...
```
The "isolation" here means that Ruff won't apply more than one fix to
the same type-parameter list in a single iteration of the loop it does
to apply all autofixes. This means that after the first autofix has been
done, the semantic model will have recalculated which variables are
available in the scope, meaning that the diagnostic for the second
parameter will be deemed unfixable since it collides with an existing
name in the same scope (the name we autofixed the first parameter to in
an earlier iteration of the autofix loop).
Cc. @ntBre, for interest!
## Test Plan
I added an integration test that reproduces the bug on `main`.
When suggesting a return type as a union in Python <=3.9, we now avoid a
`TypeError` by correctly suggesting syntax like `Union[int,str,None]`
instead of `Union[int | str | None]`.
## Summary
Follow-up to #16026.
Previously, the fix for this would be marked as unsafe, even though all
comments are preserved:
```python
# .pyi
T: TypeAlias = ( # Comment
int | str
)
```
Now it is safe: comments within the parenthesized range no longer affect
applicability.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Dylan <53534755+dylwil3@users.noreply.github.com>
## Summary
Resolves#15968.
Previously, these would be considered violations:
```python
b''.strip('//')
''.lstrip('//', foo = "bar")
```
...while these are not:
```python
b''.strip(b'//')
''.strip('\\b\\x08')
```
Ruff will now not report when the types of the object and that of the
argument mismatch, or when there are extra arguments.
## Test Plan
`cargo nextest run` and `cargo insta test`.
See #15951 for the original discussion and reviews. This is just the
first half of that PR (reaching parity with `flake8-builtins` without
adding any new configuration options) split out for nicer changelog
entries.
For posterity, here's a script for generating the module structure that
was useful for interactive testing and creating the table
[here](https://github.com/astral-sh/ruff/pull/15951#issuecomment-2640662041).
The results for this branch are the same as the `Strict` column there,
as expected.
```shell
mkdir abc collections foobar urlparse
for i in */
do
touch $i/__init__.py
done
cp -r abc foobar collections/.
cp -r abc collections foobar/.
touch ruff.toml
touch foobar/logging.py
```
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
This very large PR changes the field `.diagnostics` in the `Checker`
from a `Vec<Diagnostic>` to a `RefCell<Vec<Diagnostic>>`, adds methods
to push new diagnostics to this cell, and then removes unnecessary
mutability throughout all of our lint rule implementations.
Consequently, the compiler may now enforce what was, till now, the
_convention_ that the only changes to the `Checker` that can happen
during a lint are the addition of diagnostics[^1].
The PR is best reviewed commit-by-commit. I have tried to keep the large
commits limited to "bulk actions that you can easily see are performing
the same find/replace on a large number of files", and separate anything
ad-hoc or with larger diffs. Please let me know if there's anything else
I can do to make this easier to review!
Many thanks to [`ast-grep`](https://github.com/ast-grep/ast-grep),
[`helix`](https://github.com/helix-editor/helix), and good ol'
fashioned`git` magic, without which this PR would have taken the rest of
my natural life.
[^1]: And randomly also the seen variables violating `flake8-bugbear`?
## Summary
- Do not return `Option<Type<…>>` from `Unpacker::get`, but just `Type`.
Panic otherwise.
- Rename `Unpacker::get` to `Unpacker::expression_type`
## Summary
* Support assignments to attributes in more cases:
- assignments in `for` loops
- in unpacking assignments
* Add test for multi-target assignments
* Add tests for all other possible assignments to attributes that could
possibly occur (in decreasing order of likeliness):
- augmented attribute assignments
- attribute assignments in `with` statements
- attribute assignments in comprehensions
- Note: assignments to attributes in named expressions are not
syntactically allowed
closes#15962
## Test Plan
New Markdown tests
## Summary
This PR reverts the behavior changes from
https://github.com/astral-sh/ruff/pull/15990
But it isn't just a revert, it also:
* Adds a test covering this specific behavior
* Preserves the improvement to use `saturating_sub` in the package case
to avoid overflows in the case of invalid syntax
* Use `ancestors` instead of a `for` loop
## Test Plan
Added test
## Summary
Adds a JSON schema generation step for Red Knot. This PR doesn't yet add
a publishing step because it's still a bit early for that
## Test plan
I tested the schema in Zed, VS Code and PyCharm:
* PyCharm: You have to manually add a schema mapping (settings JSON
Schema Mappings)
* Zed and VS code support the inline schema specification
```toml
#:schema /Users/micha/astral/ruff/knot.schema.json
[environment]
extra-paths = []
[rules]
call-possibly-unbound-method = "error"
unknown-rule = "error"
# duplicate-base = "error"
```
```json
{
"$schema": "file:///Users/micha/astral/ruff/knot.schema.json",
"environment": {
"python-version": "3.13",
"python-platform": "linux2"
},
"rules": {
"unknown-rule": "error"
}
}
```
https://github.com/user-attachments/assets/a18fcd96-7cbe-4110-985b-9f1935584411
The Schema overall works but all editors have their own quirks:
* PyCharm: Hovering a name always shows the section description instead
of the description of the specific setting. But it's the same for other
settings in `pyproject.toml` files 🤷
* VS Code (JSON): Using the generated schema in a JSON file gives
exactly the experience I want
* VS Code (TOML):
* Properties with multiple possible values are repeated during
auto-completion without giving any hint how they're different. 
* The property description mushes together the description of the
property and the value, which looks sort of ridiculous. 
* Autocompletion and documentation hovering works (except the
limitations mentioned above)
* Zed:
* Very similar to VS Code with the exception that it uses the
description attribute to distinguish settings with multiple possible
values 
I don't think there's much we can do here other than hope (or help)
editors improve their auto completion. The same short comings also apply
to ruff, so this isn't something new. For now, I think this is good
enough
## Summary
Part of #15809 and #15876.
This change brings several bugfixes:
* The nested `map()` call in `list(map(lambda x: x, []))` where `list`
is overshadowed is now correctly reported.
* The call will no longer reported if:
* Any arguments given to `map()` are variadic.
* Any of the iterables contain a named expression.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
This change resolves#15814 to ensure that `SIM401` is only triggered on
known dictionary types. Before, the rule was getting triggered even on
types that _resemble_ a dictionary but are not actually a dictionary.
I did this using the `is_known_to_be_of_type_dict(...)` functionality.
The logic for this function was duplicated in a few spots, so I moved
the code to a central location, removed redundant definitions, and
updated existing calls to use the single definition of the function!
## Test Plan
Since this PR only modifies an existing rule, I made changes to the
existing test instead of adding new ones. I made sure that `SIM401` is
triggered on types that are clearly dictionaries and that it's not
triggered on a simple custom dictionary-like type (using a modified
version of [the code in the issue](#15814))
The additional changes to de-duplicate `is_known_to_be_of_type_dict`
don't break any existing tests -- I think this should be fine since the
logic remains the same (please let me know if you think otherwise, I'm
excited to get feedback and work towards a good fix 🙂).
---------
Co-authored-by: Junhson Jean-Baptiste <junhsonjb@naan.mynetworksettings.com>
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
Resolves#15997.
Ruff used to introduce syntax errors while fixing these cases, but no
longer will:
```python
{"a": [], **{},}
# ^^^^ Removed, leaving two contiguous commas
{"a": [], **({})}
# ^^^^^ Removed, leaving a stray closing parentheses
```
Previously, the function would take a shortcut if the unpacked
dictionary is empty; now, both cases are handled using the same logic
introduced in #15394. This change slightly modifies that logic to also
remove the first comma following the dictionary, if and only if it is
empty.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
Fixes https://github.com/astral-sh/ruff/issues/15978
Even Better TOML doesn't support `allOf` well. In fact, it just crashes.
This PR works around this limitation by avoid using `allOf` in the
automatically
derived schema for the docstring formatting setting.
### Alternatives
schemars introduces `allOf` whenver it sees a `$ref` alongside other
object properties
because this is no longer valid according to Draft 7. We could replace
the
visitor performing the rewrite but I prefer not to because replacing
`allOf` with `oneOf`
is only valid for objects that don't have any other `oneOf` or `anyOf`
schema.
## Test Plan
https://github.com/user-attachments/assets/25d73b2a-fee1-4ba6-9ffe-869b2c3bc64e
## Summary
I noticed that the diagnostic range in specific unpacking assignments is
wrong. For this example
```py
a, b = 1
```
we previously got (see first commit):
```
error: lint:not-iterable
--> /src/mdtest_snippet.py:1:1
|
1 | a, b = 1
| ^^^^ Object of type `Literal[1]` is not iterable
|
```
and with this change, we get:
```
error: lint:not-iterable
--> /src/mdtest_snippet.py:1:8
|
1 | a, b = 1
| ^ Object of type `Literal[1]` is not iterable
|
```
## Test Plan
New snapshot tests.
## Summary
Fixes https://github.com/astral-sh/ruff/issues/15989
Red Knot failed to resolve relative imports if the importing module is
located at a search path root.
The issue was that the module resolver returned an `Err(TooManyDots)` as
soon as the parent of the current module is `None` (which is the case
for a module at the search path root).
However, this is incorrect if a `tail` (a module name) exists.
Closes#15681
## Summary
This changes `analyze::typing::is_type_checking_block` to recognize all
symbols named "TYPE_CHECKING".
This matches the current behavior of mypy and pyright as well as
`flake8-type-checking`.
It also drops support for detecting `if False:` and `if 0:` as type
checking blocks. This used to be an option for
providing backwards compatibility with Python versions that did not have
a `typing` module, but has since
been removed from the typing spec and is no longer supported by any of
the mainstream type checkers.
## Test Plan
`cargo nextest run`
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
- Minor wording update
- Code improvement (thanks Alex)
- Removed all unnecessary filenames throughout our Markdown tests (two
new ones were added in the meantime)
- Minor rewording of the statically-known-branches introduction
This example from @sharkdp shows how terminal statements can appear in
statically known branches:
https://github.com/astral-sh/ruff/pull/15676#issuecomment-2618809716
```py
def _(cond: bool):
x = "a"
if cond:
x = "b"
if True:
return
reveal_type(x) # revealed: "a", "b"; should be "a"
```
We now use visibility constraints to track reachability, which allows us
to model this correctly. There are two related changes as a result:
- New bindings are not assumed to be visible; they inherit the current
"scope start" visibility, which effectively means that new bindings are
visible if/when the current flow is reachable
- When simplifying visibility constraints after branching control flow,
we only simplify if none of the intervening branches included a terminal
statement. That is, earlier unaffected bindings are only _actually_
unaffected if all branches make it to the merge point.
## Summary
Allow for literate style in Markdown tests and merge multiple (unnamed)
code blocks into a single embedded file.
closes#15941
## Test Plan
- Interactively made sure that error-lines were reported correctly in
multi-snippet sections.
This causes the diagnostic to highlight the actual unresovable import
instead of the entire `from ... import ...` statement.
While we're here, we expand the test coverage to cover all of the
possible ways that an `import` or a `from ... import` can fail.
Some considerations:
* The first commit in this PR adds a regression test for the current
behavior.
* This creates a new `mdtest/diagnostics` directory. Are folks cool
with this? I guess the idea is to put tests more devoted to diagnostics
than semantics in this directory. (Although I'm guessing there will
be some overlap.)
Fixes#15866
## Summary
This is a first step towards creating a test suite for
[descriptors](https://docs.python.org/3/howto/descriptor.html). It does
not (yet) aim to be exhaustive.
relevant ticket: #15966
## Test Plan
Compared desired behavior with the runtime behavior and the behavior of
existing type checkers.
---------
Co-authored-by: Mike Perlov <mishamsk@gmail.com>
This ties together everything from the previous commits.
Some interesting bits here are how the snapshot is generated
(where we include relevant info to make it easier to review
the snapshots) and also a tweak to how inline assertions are
processed.
This commit also includes some example snapshots just to get
a sense of what they look like. Follow-up work should add
more of these I think.
I split this out into a separate commit and put it here
so that reviewers can get a conceptual model of what the
code is doing before seeing the code. (Hopefully that helps.)
This makes it possible for callers to set where snapshots
should be stored. In general, I think we expect this to
always be set, since otherwise snapshots will end up in
`red_knot_test`, which is where the tests are actually run.
But that's overall counter-intuitive. This permits us to
store snapshots from mdtests alongside the mdtests themselves.
I found it useful to have the `&dyn Diagnostic` trait impl
specifically. I added `Arc<dyn Diagnostic>` for completeness.
(I do kind of wonder if we should be preferring `Arc<dyn ...>`
over something like `Box<dyn ...>` more generally, especially
for things with immutable APIs. It would make cloning cheap.)
This change was done to reduce snapshot churn. Previously,
if one added a new section to an Markdown test suite, then
the snapshots of all sections with unnamed files below it would
necessarily change because of the unnamed file count being
global to the test suite.
Instead, we track counts based on section. While adding new
unnamed files within a section will still change unnamed
files below it, I believe this will be less "churn" because
the snapshot will need to change anyway. Some churn is still
possible, e.g., if code blocks are re-ordered. But I think this
is an acceptable trade-off.
## Summary
Minor docs follow-up to #15862 to mention UP049 in the UP046 and UP047
`See also` sections. I wanted to mention it in UP040 too but realized it
didn't have a `See also` section, so I also added that, adapted from the
other two rules.
## Test Plan
cargo test
## Summary
The PR addresses the issue #15887
For two objects `a` and `b`, we ensure that the auto-fix and the
suggestion is of the form `a = min(a, b)` (or `a = max(a, b)`). This is
because we want to be consistent with the python implementation of the
methods: `min` and `max`. See the above issue for more details.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
Resolves#15936.
The fixes will now attempt to preserve the original iterable's format
and quote it if necessary. For `FURB142`, comments within the fix range
will make it unsafe as well.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
Resolves#15863.
In preview, diagnostic ranges will now be limited to that of the
argument. Rule documentation, variable names, error messages and fix
titles have all been modified to use "argument" consistently.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
Resolves#15925.
`N803` now checks for functions instead of parameters. In preview mode,
if a method is decorated with `@override` and the current scope is that
of a class, it will be ignored.
## Test Plan
`cargo nextest run` and `cargo insta test`.
## Summary
Follow-up to #15779.
Prior to this change, non-name expressions are not reported at all:
```python
type(a.b) is type(None) # no error
```
This change enhances the rule so that such cases are also reported in
preview. Additionally:
* The fix will now be marked as unsafe if there are any comments within
its range.
* Error messages are slightly modified.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Previously an error was emitted any time the configuration required both
an import of a module and an alias for that module. However, required
imports could themselves contain an alias, which may or may not agree
with the required alias.
To wit: requiring `import pandas as pd` does not conflict with the
`flake8-import-conventions.alias` config `{"pandas":"pd"}`.
This PR refines the check before throwing an error.
Closes#15911
## Summary
This PR adds `Type::call_bound` method for calls that should follow
descriptor protocol calling convention. The PR is intentionally shallow
in scope and only fixes#15672
Couple of obvious things that weren't done:
* Switch to `call_bound` everywhere it should be used
* Address the fact, that red_knot resolves `__bool__ = bool` as a Union,
which includes `Type::Dynamic` and hence fails to infer that the
truthiness is always false for such a class (I've added a todo comment
in mdtests)
* Doesn't try to invent a new type for descriptors, although I have a
gut feeling it may be more convenient in the end, instead of doing
method lookup each time like I did in `call_bound`
## Test Plan
* extended mdtests with 2 examples from the issue
* cargo neatest run
We now use ternary decision diagrams (TDDs) to represent visibility
constraints. A TDD is just like a BDD ([_binary_ decision
diagram](https://en.wikipedia.org/wiki/Binary_decision_diagram)), but
with "ambiguous" as an additional allowed value. Unlike the previous
representation, TDDs are strongly normalizing, so equivalent ternary
formulas are represented by exactly the same graph node, and can be
compared for equality in constant time.
We currently have a slight 1-3% performance regression with this in
place, according to local testing. However, we also have a _5× increase_
in performance for pathological cases, since we can now remove the
recursion limit when we evaluate visibility constraints.
As follow-on work, we are now closer to being able to remove the
`simplify_visibility_constraint` calls in the semantic index builder. In
the vast majority of cases, we now see (for instance) that the
visibility constraint after an `if` statement, for bindings of symbols
that weren't rebound in any branch, simplifies back to `true`. But there
are still some cases we generate constraints that are cyclic. With
fixed-point cycle support in salsa, or with some careful analysis of the
still-failing cases, we might be able to remove those.
## Summary
This is a new rule to implement the renaming of PEP 695 type parameters
with leading underscores after they have (presumably) been converted
from standalone type variables by either UP046 or UP047. Part of #15642.
I'm not 100% sure the fix is always safe, but I haven't come up with any
counterexamples yet. `Renamer` seems pretty precise, so I don't think
the usual issues with comments apply.
I initially tried writing this as a rule that receives a `Stmt` rather
than a `Binding`, but in that case the
`checker.semantic().current_scope()` was the global scope, rather than
the scope of the type parameters as I needed. Most of the other rules
using `Renamer` also used `Binding`s, but it does have the downside of
offering separate diagnostics for each parameter to rename.
## Test Plan
New snapshot tests for UP049 alone and the combination of UP046, UP049,
and PYI018.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
I experimented with [not trimming trailing newlines in code
snippets](https://github.com/astral-sh/ruff/pull/15926#discussion_r1940992090),
but since came to the conclusion that the current behavior is better
because otherwise, there is no way to write snippets without a trailing
newline at all. And when you copy the code from a Markdown snippet in
GitHub, you also don't get a trailing newline.
I was surprised to see some test failures when I played with this
though, and decided to make this test independent from this
implementation detail.
## Summary
Fix line number reporting in MDTest error messages.
## Test Plan
Introduced an error in a Markdown test and made sure that the line in
the error message matches.
## Summary
This is a follow-up to #15726, #15778, and #15794 to preserve the triple
quote and prefix flags in plain strings, bytestrings, and f-strings.
I also added a `StringLiteralFlags::without_triple_quotes` method to
avoid passing along triple quotes in rules like SIM905 where it might
not make sense, as discussed
[here](https://github.com/astral-sh/ruff/pull/15726#discussion_r1930532426).
## Test Plan
Existing tests, plus many new cases in the `generator::tests::quote`
test that should cover all combinations of quotes and prefixes, at least
for simple string bodies.
Closes#7799 when combined with #15694, #15726, #15778, and #15794.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Replaces our existing Markdown test parser with a fully hand-written
parser. I tried to fix this bug using the old approach and kept running
into problems. Eventually this seemed like the easier way. It's more
code (+50 lines, excluding the new test), but I hope it's relatively
straightforward to understand, compared to the complex interplay between
the byte-stream-manipulation and regex-parsing that we had before.
I did not really focus on performance, as the parsing time does not
dominate the test execution time, but this seems to be slightly faster
than what we had before (executing all MD tests; debug):
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| this branch | 2.775 ± 0.072 | 2.690 | 2.877 | 1.00 |
| `main` | 2.921 ± 0.034 | 2.865 | 2.967 | 1.05 ± 0.03 |
closes#15923
## Test Plan
One new regression test.
Carl Meyer
Dhruv Manilawala
Brent Westbrook
Douglas Creager
David Peter
Alex Waygood
Muspi Merol
Micha Reiser
Mike Perlov
Vasco Schiavo
InSync
Ari Pollak
Darius Carrier
Victorien
Andrew Gallant
Wei Lee
Dylan
sobolevn
purajit
cake-monotone
Dylan
Aaron Gokaslan
Ayush Baweja
github-actions[bot]
Vlad Nedelcu
Junhson Jean-Baptiste
Shaygan Hooshyari
Ibraheem Ahmed
ABDULRAHMAN ALRAHMA
David Salvisberg
Raymond Berger
David Salvisberg