Regardless of whether #16408 and #16311 pan out, this part is worth
pulling out as a separate PR.
Before, you had to define a new `IndexVec` index type for each type of
association list you wanted to create. Now there's a single index type
that's internal to the alist implementation, and you use `List<K, V>` to
store a handle to a particular list.
This also adds some property tests for the alist implementation.
## Summary
This PR adds support for a pragma-style header for inline parser tests
containing JSON-serialized `ParseOptions`. For example,
```python
# parse_options: { "target-version": "3.9" }
match 2:
case 1:
pass
```
The line must start with `# parse_options: ` and then the rest of the
(trimmed) line is deserialized into `ParseOptions` used for parsing the
the test.
## Test Plan
Existing inline tests, plus two new inline tests for
`match-before-py310`.
---------
Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
## Summary
As mentioned in
https://github.com/astral-sh/ruff/pull/16296#discussion_r1967047387
This PR updates the client settings resolver to notify the user if there
are any errors in the config using a very basic approach. In addition,
each error related to specific settings are logged.
This isn't the best approach because it can log the same message
multiple times when both workspace and global settings are provided and
they both are the same. This is the case for a single workspace VS Code
instance.
I do have some ideas on how to improve this and will explore them during
my free time (low priority):
* Avoid resolving the global settings multiple times as they're static
* Include the source of the setting (workspace or global?)
* Maybe use a struct (`ResolvedClientSettings` +
`Vec<ClientSettingsResolverError>`) instead to make unit testing easier
## Test Plan
Using:
```jsonc
{
"ruff.logLevel": "debug",
// Invalid settings
"ruff.configuration": "$RANDOM",
"ruff.lint.select": ["RUF000", "I001"],
"ruff.lint.extendSelect": ["B001", "B002"],
"ruff.lint.ignore": ["I999", "F401"]
}
```
The error logs:
```
2025-02-27 12:30:04.318736000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found
2025-02-27 12:30:04.319196000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found
2025-02-27 12:30:04.320549000 ERROR Unknown rule selectors found in `lint.select`: ["RUF000"]
2025-02-27 12:30:04.320669000 ERROR Unknown rule selectors found in `lint.extendSelect`: ["B001"]
2025-02-27 12:30:04.320764000 ERROR Unknown rule selectors found in `lint.ignore`: ["I999"]
```
Notification preview:
<img width="470" alt="Screenshot 2025-02-27 at 12 29 06 PM"
src="https://github.com/user-attachments/assets/61f41d5c-2558-46b3-a1ed-82114fd8ec22"
/>
## Summary
Closes: https://github.com/astral-sh/ruff/issues/16267
This change skips building the `index` in RuffSettingsIndex when the
configuration preference, in the editor settings, is set to
`editorOnly`. This is appropriate due to the fact that the indexes will
go unused as long as the configuration preference persists.
## Test Plan
I have tested this in VSCode and can confirm that we skip indexing when
`editorOnly` is set. Upon switching back to `editorFirst` or
`filesystemFirst` we index the settings as normal.
I don't seen any unit tests for setting indexing at the moment, but I am
happy to give it a shot if that is something we want.
We currently keep two separate pieces of state regarding the current
loop on `SemanticIndexBuilder`. One is an enum simply reflecting whether
we are currently inside a loop, and the other is the saved flow states
for `break` statements found in the current loop.
For adding loopy control flow, I'll need to add some additional loop
state (`continue` states, for example). Prepare for this by
consolidating our existing loop state into a single struct and
simplifying the API for pushing and popping a loop.
This is purely a refactor, so tests are not changed.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Resolves#16374.
`PLW0177` now also reports the pattern of a case branch if it is an
attribute access whose qualified name is that of either `np.nan` or
`math.nan`.
As the rule is in preview, the changes are not preview-gated.
## Test Plan
`cargo nextest run` and `cargo insta test`.
Minor follow-up to https://github.com/astral-sh/ruff/pull/16161
This `not_callable` flag wasn't functional, because it could never be
`false`. It was initialized to `true` and then only ever updated with
`|=`, which can never make it `false`.
Add a test that exercises the case where it _should_ be `false` (all of
the union elements are callable) but `bindings` is also empty (all union
elements have binding errors). Before this PR, the added test wrongly
emits a diagnostic that the union `Literal[f1] | Literal[f2]` is not
callable.
And add a test where a union call results in one binding error and one
not-callable error, where we currently give the wrong result (we show
only the binding error), with a TODO.
Also add TODO comments in a couple other tests where ideally we'd report
more than just one error out of a union call.
Also update the flag name to `all_errors_not_callable` to more clearly
indicate the semantics of the flag.
## 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.
## Summary
Extend AIR302 with
* `airflow.operators.bash.BashOperator →
airflow.providers.standard.operators.bash.BashOperator`
* change existing rules `airflow.operators.bash_operator.BashOperator →
airflow.operators.bash.BashOperator` to
`airflow.operators.bash_operator.BashOperator →
airflow.providers.standard.operators.bash.BashOperator`
## Test Plan
a test fixture has been updated
## Summary
Resolves#15695, rework of #15704.
This change modifies the Mdtests framework so that:
* Paths must now be specified in a separate preceding line:
`````markdown
`a.py`:
```py
x = 1
```
`````
If the path of a file conflicts with its `lang`, an error will be
thrown.
* Configs are no longer accepted. The pattern still take them into
account, however, to avoid "Unterminated code block" errors.
* Unnamed files are now assigned unique, `lang`-respecting paths
automatically.
Additionally, all legacy usages have been updated.
## Test Plan
Unit tests and Markdown tests.
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
This extracts some pure refactoring noise from
https://github.com/astral-sh/ruff/pull/15861. This changes the API for
creating and evaluating visibility constraints, but does not change how
they are respresented internally. There should be no behavioral or
performance changes in this PR.
Changes:
- Hide the internal representation isn't changed, so that we can make
changes to it in #15861.
- Add a separate builder type for visibility constraints. (With TDDs, we
will have some additional builder state that we can throw away once
we're done constructing.)
- Remove a layer of helper methods from `UseDefMapBuilder`, making
`SemanticIndexBuilder` responsible for constructing whatever visibility
constraints it needs.
## Summary
Add support for implicitly-defined instance attributes, i.e. support
type inference for cases like this:
```py
class C:
def __init__(self) -> None:
self.x: int = 1
self.y = None
reveal_type(C().x) # int
reveal_type(C().y) # Unknown | None
```
## Benchmarks
Codspeed reports no change in a cold-cache benchmark, and a -1%
regression in the incremental benchmark. On `black`'s `src` folder, I
don't see a statistically significant difference between the branches:
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `./red_knot_main check --project /home/shark/black/src` | 133.7 ± 9.5 | 126.7 | 164.7 | 1.01 ± 0.08 |
| `./red_knot_feature check --project /home/shark/black/src` | 132.2 ± 5.1 | 118.1 | 140.9 | 1.00 |
## Test Plan
Updated and new Markdown tests
## Summary
Given the following code:
```python
set(([x for x in range(5)]))
```
the current implementation of C403 results in
```python
{(x for x in range(5))}
```
which is a set containing a generator rather than the result of the
generator.
This change removes the extraneous parentheses so that the resulting
code is:
```python
{x for x in range(5)}
```
## Test Plan
`cargo nextest run` and `cargo insta test`
## Summary
Related to #15848, this PR adds the imports explicitly as we'll now flag
these symbols as undefined.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This change does a simple swap of the existing renderer for one that
uses our vendored copy of `annotate-snippets`. We don't change anything
about the diagnostic data model, but this alone already makes
diagnostics look a lot nicer!
This mimics a simplification we have on the OR side, where we simplify
`A ∨ !A` to true. This requires changes to how we add `while` statements
to the semantic index, since we now need distinct
`VisibilityConstraint`s if we need to model evaluating a `Constraint`
multiple times at different points in the execution of the program.
Something Alex and I threw together during our 1:1 this morning. Allows
us to collect statistics on the prevalence of various types in a file,
most usefully TODO types or other dynamic types.
## Summary
This is a follow-up to #15565, tracked in #15642, to reuse the string
replacement logic from the other PEP 695 rules instead of the
`Generator`, which has the benefit of preserving more comments. However,
comments in some places are still dropped, so I added a check for this
and update the fix safety accordingly. I also added a `## Fix safety`
section to the docs to reflect this and the existing `isinstance`
caveat.
## Test Plan
Existing UP040 tests, plus some new cases.
## Summary
Resolves#10063 and follow-up to #15521.
The fix is now marked as unsafe if there are any comments within its
range. Tests are adapted from that of #15521.
## Test Plan
`cargo nextest run` and `cargo insta test`.
Both `list` and `dict` expect only a single positional argument. Giving
more positional arguments, or a keyword argument, is a `TypeError` and
neither the lint rule nor its fix make sense in that context.
Closes#15810
## Summary
Fixes https://github.com/astral-sh/ruff/issues/15812 by visiting the
second argument as a type definition.
## Test Plan
New F401 tests based on the report.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Builtin bindings are given a range of `0..0`, which causes strange
behavior when range checks are made at the top of the file. In this
case, the logic of the rule demands that the value of the dict
comprehension is not self-referential (i.e. it does not contain
definitions for any of the variables used within it). This logic was
confused by builtins which looked like they were defined "in the
comprehension", if the comprehension appeared at the top of the file.
Closes#15830
## Summary
This PR removes a trailing semicolon after an interface definition in
the custom TypeScript section of `ruff_wasm`. Currently, this semicolon
triggers the error "TS1036: Statements are not allowed in ambient
contexts" when including the file and compiling with e.g `tsc`.
## Test Plan
I made the change, ran `wasm-pack` and copied the generated directory
manually to my `node_modules` folder. I then compiled a file importing
`@astral-sh/ruff-wasm-web` again and confirmed that the compilation
error was gone.
If there is any `ParenthesizedWhitespace` (in the sense of LibCST) after
the function name `sorted` and before the arguments, then we must wrap
`sorted` with parentheses after removing the surrounding function.
Closes#15789
This PR uses the tokens of the parsed annotation available in the
`Checker`, instead of re-lexing (using `SimpleTokenizer`) the
annotation. This avoids some limitations of the `SimpleTokenizer`, such
as not being able to handle number and string literals.
Closes#15816 .
## Summary
Permits suspicious imports (the `S4` namespaced diagnostics) from stub
files.
Closes#15207.
## Test Plan
Added tests and ran `cargo nextest run`. The test files are copied from
the `.py` variants.
`FlowSnapshot` now tracks a `reachable` bool, which indicates whether we
have encountered a terminal statement on that control flow path. When
merging flow states together, we skip any that have been marked
unreachable. This ensures that bindings that can only be reached through
unreachable paths are not considered visible.
## Test Plan
The new mdtests failed (with incorrect `reveal_type` results, and
spurious `possibly-unresolved-reference` errors) before adding the new
visibility constraints.
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
## Summary
This is another follow-up to #15726 and #15778, extending the
quote-preserving behavior to f-strings and deleting the now-unused
`Generator::quote` field.
## Details
I also made one unrelated change to `rules/flynt/helpers.rs` to remove a
`to_string` call for making a `Box<str>` and tweaked some arguments to
some of the `Generator::unparse_f_string` methods to make the code
easier to follow, in my opinion. Happy to revert especially the latter
of these if needed.
Unfortunately this still does not fix the issue in #9660, which appears
to be more of an escaping issue than a quote-preservation issue. After
#15726, the result is now `a = f'# {"".join([])}' if 1 else ""` instead
of `a = f"# {''.join([])}" if 1 else ""` (single quotes on the outside
now), but we still don't have the desired behavior of double quotes
everywhere on Python 3.12+. I added a test for this but split it off
into another branch since it ended up being unaddressed here, but my
`dbg!` statements showed the correct preferred quotes going into
[`UnicodeEscape::with_preferred_quote`](https://github.com/astral-sh/ruff/blob/main/crates/ruff_python_literal/src/escape.rs#L54).
## Test Plan
Existing rule and `Generator` tests.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Implements some of #14738, by adding support for 6 new patterns:
```py
re.search("abc", s) is None # ⇒ "abc" not in s
re.search("abc", s) is not None # ⇒ "abc" in s
re.match("abc", s) is None # ⇒ not s.startswith("abc")
re.match("abc", s) is not None # ⇒ s.startswith("abc")
re.fullmatch("abc", s) is None # ⇒ s != "abc"
re.fullmatch("abc", s) is not None # ⇒ s == "abc"
```
## Test Plan
```shell
cargo nextest run
cargo insta review
```
And ran the fix on my startup's repo.
## Note
One minor limitation here:
```py
if not re.match('abc', s) is None:
pass
```
will get fixed to this (technically correct, just not nice):
```py
if not not s.startswith('abc'):
pass
```
This seems fine given that Ruff has this covered: the initial code
should be caught by
[E714](https://docs.astral.sh/ruff/rules/not-is-test/) and the fixed
code should be caught by
[SIM208](https://docs.astral.sh/ruff/rules/double-negation/).
## Summary
When we discussed the plan on how to proceed with instance attributes,
we said that we should first extend our research into the behavior of
existing type checkers. The result of this research is summarized in the
newly added / modified tests in this PR. The TODO comments align with
existing behavior of other type checkers. If we deviate from the
behavior, it is described in a comment.
## Summary
Resolves#12717.
This change incorporates the logic added in #15588.
## Test Plan
`cargo nextest run` and `cargo insta test`.
---------
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
## Summary
As promised in #15603 - the **highly** sophisticated change - adding
missing config docstrings that are used in command completions.
## Test Plan
I actually made a local change to emit all empty items and verified
there are none now, before opening the PR.
## Summary
This is a very closely related follow-up to #15726, adding the same
quote-preserving behavior to bytestrings. Only one rule (UP018) was
affected this time, and it was easy to mirror the plain string changes.
## Test Plan
Existing tests
## Summary
This is a first step toward fixing #7799 by using the quoting style
stored in the `flags` field on `ast::StringLiteral`s to select a quoting
style. This PR does not include support for f-strings or byte strings.
Several rules also needed small updates to pass along existing quoting
styles instead of using `StringLiteralFlags::default()`. The remaining
snapshot changes are intentional and should preserve the quotes from the
input strings.
## Test Plan
Existing tests with some accepted updates, plus a few new RUF055 tests
for raw strings.
---------
Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
* feat
* add is_execute_method_inherits_from_airflow_operator for checking the
removed context key in the execute method
* refactor: rename
* is_airflow_task as is_airflow_task_function_def
* in_airflow_task as in_airflow_task_function_def
* removed_in_3 as airflow_3_removal_expr
* removed_in_3_function_def as airflow_3_removal_function_def
* test:
* reorganize test cases
## Test Plan
a test fixture has been updated
---------
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
## Summary
Not the most important feature, but hey... was marked as the good first
issue ;-) fixes#4551
Unfortunately, looks like clap only generates proper completions for
zsh, so this would not make any difference for bash/fish.
## Test Plan
- cargo nextest run
- manual test by sourcing completions and then triggering autocomplete:
```shell
misha@PandaBook ruff % source <(target/debug/ruff generate-shell-completion zsh)
misha@PandaBook ruff % target/debug/ruff config lin
line-length -- The line length to use when enforcing long-lines violations
lint -- Configures how Ruff checks your code.
lint.allowed-confusables -- A list of allowed 'confusable' Unicode characters to ignore
lint.dummy-variable-rgx -- A regular expression used to identify 'dummy' variables, or
lint.exclude -- A list of file patterns to exclude from linting in addition
lint.explicit-preview-rules -- Whether to require exact codes to select preview rules. Whe
lint.extend-fixable -- A list of rule codes or prefixes to consider fixable, in ad
lint.extend-ignore -- A list of rule codes or prefixes to ignore, in addition to
lint.extend-per-file-ignores -- A list of mappings from file pattern to rule codes or prefi
lint.extend-safe-fixes -- A list of rule codes or prefixes for which unsafe fixes sho
lint.extend-select -- A list of rule codes or prefixes to enable, in addition to
lint.extend-unsafe-fixes -- A list of rule codes or prefixes for which safe fixes shoul
lint.external -- A list of rule codes or prefixes that are unsupported by Ru
lint.fixable -- A list of rule codes or prefixes to consider fixable. By de
lint.flake8-annotations -- Print a list of available options
lint.flake8-annotations.allow-star-arg-any -- Whether to suppress `ANN401` for dynamically typed `*args`
...
```
- check command help
```shell
❯ target/debug/ruff config -h
List or describe the available configuration options
Usage: ruff config [OPTIONS] [OPTION]
Arguments:
[OPTION] Config key to show
Options:
--output-format <OUTPUT_FORMAT> Output format [default: text] [possible values: text, json]
-h, --help Print help
Log levels:
-v, --verbose Enable verbose logging
-q, --quiet Print diagnostics, but nothing else
-s, --silent Disable all logging (but still exit with status code "1" upon detecting diagnostics)
Global options:
--config <CONFIG_OPTION> Either a path to a TOML configuration file (`pyproject.toml` or `ruff.toml`), or a TOML `<KEY> =
<VALUE>` pair (such as you might find in a `ruff.toml` configuration file) overriding a specific
configuration option. Overrides of individual settings using this option always take precedence over
all configuration files, including configuration files that were also specified using `--config`
--isolated Ignore all configuration files
```
- running original command
```shell
❯ target/debug/ruff config
cache-dir
extend
output-format
fix
unsafe-fixes
fix-only
show-fixes
required-version
preview
exclude
extend-exclude
extend-include
force-exclude
include
respect-gitignore
builtins
namespace-packages
target-version
src
line-length
indent-width
lint
format
analyze
```
## Summary
Adds a slightly more comprehensive documentation of our behavior
regarding type inference for public uses of symbols. In particular:
- What public type do we infer for `x: int = any()`?
- What public type do we infer for `x: Unknown = 1`?
## Summary
Found a comment that looks to be intended as docstring but accidentally
is just a normal comment.
Didn't create an issue as the readme said it's not neccessary for
trivial changes.
## Test Plan
<!-- How was it tested? -->
Can be tested by regenerating the docs.
Co-authored-by: Marcus Näslund <vidaochmarcus@gmail.com>
## Summary
On `main`, red-knot:
- Considers `P | Q` equivalent to `Q | P`
- Considered `tuple[P | Q]` equivalent to `tuple[Q | P]`
- Considers `tuple[P | tuple[P | Q]]` equivalent to `tuple[tuple[Q | P]
| P]`
- ‼️ Does _not_ consider `tuple[tuple[P | Q]]` equivalent to
`tuple[tuple[Q | P]]`
The key difference for the last one of these is that the union appears
inside a tuple that is directly nested inside another tuple.
This PR fixes this so that differently ordered unions are considered
equivalent even when they appear inside arbitrarily nested tuple types.
## Test Plan
- Added mdtests that fails on `main`
- Checked that all property tests continue to pass with this PR
This is a follow-up to #15702 that hopefully claws back the 1%
performance regression. Assuming it works, the trick is to iterate over
the constraints vectors via mut reference (aka a single pointer), so
that we're not copying `BitSet`s into and out of the zip tuples as we
iterate. We use `std::mem::take` as a poor-man's move constructor only
at the very end, when we're ready to emplace it into the result. (C++
idioms intended! 😄)
With local testing via hyperfine, I'm seeing this be 1-3% faster than
`main` most of the time — though a small number of runs (1 in 10,
maybe?) are a wash or have `main` faster. Codspeed reports a 2%
gain.
## Summary
Use `Unknown | T_inferred` as the type for *undeclared* public symbols.
## Test Plan
- Updated existing tests
- New test for external `__slots__` modifications.
- New tests for external modifications of public symbols.
**Summary**
Airflow 3.0 removes a set of deprecated context variables that were
phased out in 2.x. This PR introduces lint rules to detect usage of
these removed variables in various patterns, helping identify
incompatibilities. The removed context variables include:
```
conf
execution_date
next_ds
next_ds_nodash
next_execution_date
prev_ds
prev_ds_nodash
prev_execution_date
prev_execution_date_success
tomorrow_ds
yesterday_ds
yesterday_ds_nodash
```
**Detected Patterns and Examples**
The linter now flags the use of removed context variables in the
following scenarios:
1. **Direct Subscript Access**
```python
execution_date = context["execution_date"] # Flagged
```
2. **`.get("key")` Method Calls**
```python
print(context.get("execution_date")) # Flagged
```
3. **Variables Assigned from `get_current_context()`**
If a variable is assigned from `get_current_context()` and then used to
access a removed key:
```python
c = get_current_context()
print(c.get("execution_date")) # Flagged
```
4. **Function Parameters in `@task`-Decorated Functions**
Parameters named after removed context variables in functions decorated
with `@task` are flagged:
```python
from airflow.decorators import task
@task
def my_task(execution_date, **kwargs): # Parameter 'execution_date'
flagged
pass
```
5. **Removed Keys in Task Decorator `kwargs` and Other Scenarios**
Other similar patterns where removed context variables appear (e.g., as
part of `kwargs` in a `@task` function) are also detected.
```
from airflow.decorators import task
@task
def process_with_execution_date(**context):
execution_date = lambda: context["execution_date"] # flagged
print(execution_date)
@task(kwargs={"execution_date": "2021-01-01"}) # flagged
def task_with_kwargs(**context):
pass
```
**Test Plan**
Test fixtures covering various patterns of deprecated context usage are
included in this PR. For example:
```python
from airflow.decorators import task, dag, get_current_context
from airflow.models import DAG
from airflow.operators.dummy import DummyOperator
import pendulum
from datetime import datetime
@task
def access_invalid_key_task(**context):
print(context.get("conf")) # 'conf' flagged
@task
def print_config(**context):
execution_date = context["execution_date"] # Flagged
prev_ds = context["prev_ds"] # Flagged
@task
def from_current_context():
context = get_current_context()
print(context["execution_date"]) # Flagged
# Usage outside of a task decorated function
c = get_current_context()
print(c.get("execution_date")) # Flagged
@task
def some_task(execution_date, **kwargs):
print("execution date", execution_date) # Parameter flagged
@dag(
start_date=pendulum.datetime(2021, 1, 1, tz="UTC")
)
def my_dag():
task1 = DummyOperator(
task_id="task1",
params={
"execution_date": "{{ execution_date }}", # Flagged in template context
},
)
access_invalid_key_task()
print_config()
from_current_context()
dag = my_dag()
class CustomOperator(BaseOperator):
def execute(self, context):
execution_date = context.get("execution_date") # Flagged
next_ds = context.get("next_ds") # Flagged
next_execution_date = context["next_execution_date"] # Flagged
```
Ruff will emit `AIR302` diagnostics for each deprecated usage, with
suggestions when applicable, aiding in code migration to Airflow 3.0.
related: https://github.com/apache/airflow/issues/44409,
https://github.com/apache/airflow/issues/41641
---------
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
## Summary
Fixes#9663 and also improves the fixes for
[RUF055](https://docs.astral.sh/ruff/rules/unnecessary-regular-expression/)
since regular expressions are often written as raw strings.
This doesn't include raw f-strings.
## Test Plan
Existing snapshots for RUF055 and PT009, plus a new `Generator` test and
a regression test for the reported `PIE810` issue.
## Summary
Another small PR to focus #15674 solely on the relevant changes. This
makes our Markdown tests less dependent on precise types of public
symbols, without actually changing anything semantically in these tests.
Best reviewed using ignore-whitespace-mode.
## Test Plan
Tested these changes on `main` and on the branch from #15674.
## Summary
Make the remaining `infer.rs` unit tests independent from public symbol
type inference decisions (see upcoming change in #15674).
## Test Plan
- Made sure that the unit tests actually fail if one of the
`assert_type` assertions is changed.
## Summary
Port comprehension tests from Rust to Markdown
I don' think the remaining tests in `infer.rs` should be ported to
Markdown, maybe except for the incremental-checking tests when (if ever)
we have support for that in the MD tests.
closes#13696
## Summary
- Port "deferred annotations" unit tests to Markdown
- Port `implicit_global_in_function` unit test to Markdown
- Removed `resolve_method` and `local_inference` unit tests. These seem
like relics from a time where type inference was in it's early stages.
There is no way that these tests would fail today without lots of other
things going wrong as well.
part of #13696
based on #15683
## Test Plan
New MD tests for existing Rust unit tests.
## Summary
- Add feature to specify a custom typeshed from within Markdown-based
tests
- Port "builtins" unit tests from `infer.rs` to Markdown tests, part of
#13696
## Test Plan
- Tests for the custom typeshed feature
- New Markdown tests for deleted Rust unit tests
## Summary
Addresses the second follow up to #15565 in #15642. This was easier than
expected by using this cool destructuring syntax I hadn't used before,
and by assuming
[PYI059](https://docs.astral.sh/ruff/rules/generic-not-last-base-class/)
(`generic-not-last-base-class`).
## Test Plan
Using an existing test, plus two new tests combining multiple base
classes and multiple generics. It looks like I deleted a relevant test,
which I did, but I meant to rename this in #15565. It looks like instead
I copied it and renamed the copy.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
This PR extends our [PEP 695](https://peps.python.org/pep-0695) handling
from the type aliases handled by `UP040` to generic function and class
parameters, as suggested in the latter two examples from #4617:
```python
# Input
T = TypeVar("T", bound=float)
class A(Generic[T]):
...
def f(t: T):
...
# Output
class A[T: float]:
...
def f[T: float](t: T):
...
```
I first implemented this as part of `UP040`, but based on a brief
discussion during a very helpful pairing session with @AlexWaygood, I
opted to split them into rules separate from `UP040` and then also
separate from each other. From a quick look, and based on [this
issue](https://github.com/asottile/pyupgrade/issues/836), I'm pretty
sure neither of these rules is currently in pyupgrade, so I just took
the next available codes, `UP046` and `UP047`.
The last main TODO, noted in the rule file and in the fixture, is to
handle generic method parameters not included in the class itself, `S`
in this case:
```python
T = TypeVar("T")
S = TypeVar("S")
class Foo(Generic[T]):
def bar(self, x: T, y: S) -> S: ...
```
but Alex mentioned that that might be okay to leave for a follow-up PR.
I also left a TODO about handling multiple subclasses instead of bailing
out when more than one is present. I'm not sure how common that would
be, but I can still handle it here, or follow up on that too.
I think this is unrelated to the PR, but when I ran `cargo dev
generate-all`, it removed the rule code `PLW0101` from
`ruff.schema.json`. It seemed unrelated, so I left that out, but I
wanted to mention it just in case.
## Test Plan
New test fixture, `cargo nextest run`
Closes#4617, closes#12542
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Somehow, I managed to crash the `mdtest` runner today. I struggled to
reproduce this again to see if it's actually fixed (even with an
artificial `sleep` between the two `cargo test` invocations), but the
original backtrace clearly showed that this is where the problem
originated from. And it seems like a clear TOCTOU problem.
## Summary
Raise "invalid-assignment" diagnostics for incorrect assignments to
attributes, for example:
```py
class C:
var: str = "a"
C.var = 1 # error: "Object of type `Literal[1]` is not assignable to `str`"
```
closes#15456
## Test Plan
- Updated test assertions
- New test for assignments to module-attributes
## Summary
This PR generalizes some of the logic we have in `Type::is_subtype_of`
and `Type::is_disjoint_from` so that we fallback to the instance type of
the metaclass more often in `Type::ClassLiteral` and `Type::SubclassOf`
branches. This simplifies the code (we end up with one less branch in
`is_subtype_of`, and we can remove a helper method that's no longer
used), makes the code more robust (any fixes made to subtyping or
disjointness of instance types will automatically improve our
understanding of subtyping/disjointness for class-literal types and
`type[]` types) and more elegantly expresses the type-system invariants
encoded in these branches.
## Test Plan
No new tests added (it's a pure refactor, adding no new functionality).
All existing tests pass, however, including the property tests.
The AST generator creates a reference enum for each syntax group — an
enum where each variant contains a reference to the relevant syntax
node. Previously you could customize the name of the reference enum for
a group — primarily because there was an existing `ExpressionRef` type
that wouldn't have lined up with the auto-derived name `ExprRef`. This
follow-up PR is a simple search/replace to switch over to the
auto-derived name, so that we can remove this customization point.
This is a minor cleanup to the AST generation script to make a clearer
separation between nodes that do appear in a group enum, and those that
don't. There are some types and methods that we create for every syntax
node, and others that refer to the group that the syntax node belongs
to, and which therefore don't make sense for ungrouped nodes. This new
separation makes it clearer which category each definition is in, since
you're either inside of a `for group in ast.groups` loop, or a `for node
in ast.all_nodes` loop.
## Summary
Test executables usually write failure messages (including panics) to
stdout, but I just managed to make a mdtest crash with
```
thread 'mdtest__unary_not' has overflowed its stack
fatal runtime error: stack overflow
```
which is printed to stderr. This test simply appends stderr to stdout
(`stderr=subprocess.STDOUT` can not be used with `capture_output`)
## Test Plan
Make sure that the error message is now visible in the output of `uv -q
run crates/red_knot_python_semantic/mdtest.py`
## Summary
The `Options` struct is intended to capture the user's configuration
options but
`EnvironmentOptions::venv_path` supports both a `SitePackages::Known`
and `SitePackages::Derived`.
Users should only be able to provide `SitePackages::Derived`—they
specify a path to a venv, and Red Knot derives the path to the
site-packages directory. We'll only use the `Known` variant once we
automatically discover the Python installation.
That's why this PR changes `EnvironmentOptions::venv_path` from
`Option<SitePackages>` to `Option<SystemPathBuf>`.
This requires making some changes to the file watcher test, and I
decided to use `extra_paths` over venv path
because our venv validation is annoyingly correct -- making mocking a
venv rather involved.
## Test Plan
`cargo test`
## Summary
We were mistakenly using `CommentRanges::has_comments` to determine
whether our edits
were safe, which sometimes expands the checked range to the end of a
line. But in order to
determine safety we need to check exactly the range we're replacing.
This bug affected the rules `runtime-cast-value` (`TC006`) and
`quoted-type-alias` (`TC008`)
although it was very unlikely to be hit for `TC006` and for `TC008` we
never hit it because we
were checking the wrong expression.
## Test Plan
`cargo nextest run`
This commit fixes RUF055 rule to format `re.fullmatch(pattern, var)` to
`var == pattern` instead of the current `pattern == var` behaviour. This
is more idiomatic and easy to understand.
## Summary
This changes the current formatting behaviour of `re.fullmatch(pattern,
var)` to format it to `var == pattern` instead of `pattern == var`.
## Test Plan
I used a code file locally to see the updated formatting behaviour.
Fixes https://github.com/astral-sh/ruff/issues/14733
## Summary
As more and more tests move to Markdown, running the mdtest suite
becomes one of the most common tasks for developers working on Red Knot.
There are a few pain points when doing so, however:
- The `build.rs` script enforces recompilation (~five seconds) whenever
something changes in the `resource/mdtest` folder. This is strictly
necessary, because whenever files are added or removed, the test harness
needs to be updated. But this is very rarely the case! The most common
scenario is that a Markdown file has *changed*, and in this case, no
recompilation is necessary. It is currently not possible to distinguish
these two cases using `cargo::rerun-if-changed`. One can work around
this by running the test executable manually, but it requires finding
the path to the correct `mdtest-<random-hash>` executable.
- All Markdown tests are run by default. This is needed whenever Rust
code changes, but while working on the tests themselves, it is often
much more convenient to only run the tests for a single file. This can
be done by using a `mdtest__path_to_file` filter, but this needs to be
manually spelled out or copied from the test output.
- `cargo`s test output for a failing Markdown test is often
unnecessarily verbose. Unless there is an *actual* panic somewhere in
the code, mdtests usually fail with the explicit *"Some tests failed"*
panic in the mdtest suite. But in those cases, we are not interested in
the pointer to the source of this panic, but only in the mdtest suite
output.
This PR adds a Markdown test runner tool that attempts to make the
developer experience better.
Once it is started using
```bash
uv run -q crates/red_knot_python_semantic/mdtest.py
```
it will first recompile the tests once (if cargo requires it), find the
path to the `mdtest` executable, and then enter into a mode where it
watches for changes in the `red_knot_python_semantic` crate. Whenever …
* … a Markdown file changes, it will rerun the mdtest for this specific
file automatically (no recompilation!).
* … a Markdown file is added, it will recompile the tests and then run
the mdtest for the new file
* … Rust code is changed, it will recompile the tests and run all of
them
The tool also trims down `cargo test` output and only shows the actual
mdtest errors.
The tool will certainly require a few more iterations before it becomes
mature, but I'm curious to hear if there is any interest for something
like this.
## Test Plan
- Tested the new runner under various scenarios.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Rename two functions with outdated names (they used to return `Type`s):
* `bindings_ty` => `symbol_from_bindings` (returns `Symbol`)
* `declarations_ty` => `symbol_from_declarations` (returns a
`SymbolAndQualifiers` result)
I chose `symbol_from_*` instead of `*_symbol` as I found the previous
name quite confusing. Especially since `binding_ty` and `declaration_ty`
also exist (singular).
## Test Plan
—
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
## Summary
In preperation for https://github.com/astral-sh/ruff/pull/15558
Isolate the `show_settings` test instead of reading Ruff's
`pyproject.toml` for better test isolation.
## Test Plan
`cargo test`
## Summary
I noticed this while reviewing
https://github.com/astral-sh/ruff/pull/15541 that the code inside the
large closure cannot be formatted by the Rust formatter. This PR
extracts the qualified name and inlines the match expression.
## Test Plan
`cargo clippy` and `cargo insta`
## Summary
Right now, these are being applied in random order, since if we have two
`RedefinitionWhileUnused`, it just takes the first-generated (whereas
the next comparator in the sort here orders by location)... Which means
we frequently have to re-run!
## Summary
The fix range for sorting imports accounts for trailing whitespace, but
we should only show the trimmed range to the user when displaying the
diagnostic. So this PR changes the diagnostic range.
Closes#15504
## Test Plan
Reviewed snapshot changes
## Summary
Added some extra notes on why you should have focused try...except
blocks to
[TRY300](https://docs.astral.sh/ruff/rules/try-consider-else/).
When fixing a violation of this rule, a co-worker of mine (very
understandably) asked why this was better. The current docs just say
putting the return in the else is "more explicit", but if you look at
the [linked reference in the python
documentation](https://docs.python.org/3/tutorial/errors.html) they are
more clear on why violations like this is bad:
> The use of the else clause is better than adding additional code to
the [try](https://docs.python.org/3/reference/compound_stmts.html#try)
clause because it avoids accidentally catching an exception that wasn’t
raised by the code being protected by the try … except statement.
This is my attempt at adding more context to the docs on this. Open to
suggestions for wording!
---------
Co-authored-by: dylwil3 <dylwil3@gmail.com>
In the following situation:
```python
class Grandparent:
__slots__ = "a"
class Parent(Grandparent): ...
class Child(Parent):
__slots__ = "a"
```
the message for `W0244` now specifies that `a` is overwriting a slot
from `Grandparent`.
To implement this, we introduce a helper function `iter_super_classes`
which does a breadth-first traversal of the superclasses of a given
class (as long as they are defined in the same file, due to the usual
limitations of the semantic model).
Note: Python does not allow conflicting slots definitions under multiple
inheritance. Unless I'm misunderstanding something, I believe It follows
that the subposet of superclasses of a given class that redefine a given
slot is in fact totally ordered. There is therefore a unique _nearest_
superclass whose slot is being overwritten. So, you know, in case anyone
was super worried about that... you can just chill.
This is a followup to #9640 .
## Summary
Add support for `typing.ClassVar`, i.e. emit a diagnostic in this
scenario:
```py
from typing import ClassVar
class C:
x: ClassVar[int] = 1
c = C()
c.x = 3 # error: "Cannot assign to pure class variable `x` from an instance of type `C`"
```
## Test Plan
- New tests for the `typing.ClassVar` qualifier
- Fixed one TODO in `attributes.md`
While looking into potential AST optimizations, I noticed the `AstNode`
trait and `AnyNode` type aren't used anywhere in Ruff or Red Knot. It
looks like they might be historical artifacts of previous ways of
consuming AST nodes?
- `AstNode::cast`, `AstNode::cast_ref`, and `AstNode::can_cast` are not
used anywhere.
- Since `cast_ref` isn't needed anymore, the `Ref` associated type isn't
either.
This is a pure refactoring, with no intended behavior changes.
This PR replaces most of the hard-coded AST definitions with a
generation script, similar to what happens in `rust_python_formatter`.
I've replaced every "rote" definition that I could find, where the
content is entirely boilerplate and only depends on what syntax nodes
there are and which groups they belong to.
This is a pretty massive diff, but it's entirely a refactoring. It
should make absolutely no changes to the API or implementation. In
particular, this required adding some configuration knobs that let us
override default auto-generated names where they don't line up with
types that we created previously by hand.
## Test plan
There should be no changes outside of the `rust_python_ast` crate, which
verifies that there were no API changes as a result of the
auto-generation. Aggressive `cargo clippy` and `uvx pre-commit` runs
after each commit in the branch.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->
## Summary
Fixes parentheses not being stripped in C401. Pretty much the same as
#11607 which fixed it for C400.
## Test Plan
`cargo nextest run`
## Summary
This is a small, tentative step towards the bigger goal of understanding
instance attributes.
- Adds partial support for pure instance variables declared in the class
body, i.e. this case:
```py
class C:
variable1: str = "a"
variable2 = "b"
reveal_type(C().variable1) # str
reveal_type(C().variable2) # Unknown | Literal["b"]
```
- Adds `property` as a known class to query for `@property` decorators
- Splits up various `@Todo(instance attributes)` cases into
sub-categories.
## Test Plan
Modified existing MD tests.
## Summary
This PR adds support for configuring Red Knot in the `tool.knot` section
of the project's
`pyproject.toml` section. Options specified on the CLI precede the
options in the configuration file.
This PR only supports the `environment` and the `src.root` options for
now.
Other options will be added as separate PRs.
There are also a few concerns that I intentionally ignored as part of
this PR:
* Handling of relative paths: We need to anchor paths relative to the
current working directory (CLI), or the project (`pyproject.toml` or
`knot.toml`)
* Tracking the source of a value. Diagnostics would benefit from knowing
from which configuration a value comes so that we can point the user to
the right configuration file (or CLI) if the configuration is invalid.
* Schema generation and there's a lot more; see
https://github.com/astral-sh/ruff/issues/15491
This PR changes the default for first party codes: Our existing default
was to only add the project root. Now, Red Knot adds the project root
and `src` (if such a directory exists).
Theoretically, we'd have to add a file watcher event that changes the
first-party search paths if a user later creates a `src` directory. I
think this is pretty uncommon, which is why I ignored the complexity for
now but I can be persuaded to handle it if it's considered important.
Part of https://github.com/astral-sh/ruff/issues/15491
## Test Plan
Existing tests, new file watching test demonstrating that changing the
python version and platform is correctly reflected.
## Summary
Closes https://github.com/astral-sh/ruff/issues/15508
For any two instance types `T` and `S`, we know they are disjoint if
either `T` is final and `T` is not a subclass of `S` or `S` is final and
`S` is not a subclass of `T`.
Correspondingly, for any two types `type[T]` and `S` where `S` is an
instance type, `type[T]` can be said to be disjoint from `S` if `S` is
disjoint from `U`, where `U` is the type that represents all instances
of `T`'s metaclass.
And a heterogeneous tuple type can be said to be disjoint from an
instance type if the instance type is disjoint from `tuple` (a type
representing all instances of the `tuple` class at runtime).
## Test Plan
- A new mdtest added. Most of our `is_disjoint_from()` tests are not
written as mdtests just yet, but it's pretty hard to test some of these
edge cases from a Rust unit test!
- Ran `QUICKCHECK_TESTS=1000000 cargo test --release -p
red_knot_python_semantic -- --ignored types::property_tests::stable`
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
## Summary
Add a setting to allow ignoring one line docstrings for the pydoclint
rules.
Resolves#13086
Part of #12434
## Test Plan
Run tests with setting enabled.
---------
Co-authored-by: dylwil3 <dylwil3@gmail.com>
## Summary
This fixes the infinite loop reported in #14389 by raising an error to
the user about conflicting ICN001 (`unconventional-import-alias`) and
I002 (`missing-required-import`) configuration options.
## Test Plan
Added a CLI integration test reproducing the old behavior and then
confirming the fix.
Closes#14389
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
This fixes the infinite loop reported in #12897, where an
`unused-import` that is undefined at the scope of `__all__` is "fixed"
by adding it to `__all__` repeatedly. These changes make it so that only
imports in the global scope will be suggested to add to `__all__` and
the unused local import is simply removed.
## Test Plan
Added a CLI integration test that sets up the same module structure as
the original report
Closes#12897
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
Ref: https://github.com/astral-sh/ruff/pull/15387#discussion_r1917796907
This PR updates `F722` to show syntax error message instead of the
string content.
I think it's more useful to show the syntax error message than the
string content. In the future, when the diagnostics renderer is more
capable, we could even highlight the exact location of the syntax error
along with the annotation string.
This is also in line with how we show the diagnostic in red knot.
## Test Plan
Update existing test snapshots.
## Summary
Resolves#9467
Parse quoted annotations as if the string content is inside parenthesis.
With this logic `x` and `y` in this example are equal:
```python
y: """
int |
str
"""
z: """(
int |
str
)
"""
```
Also this rule only applies to triple
quotes([link](https://github.com/python/typing-council/issues/9#issuecomment-1890808610)).
This PR is based on the
[comments](https://github.com/astral-sh/ruff/issues/9467#issuecomment-2579180991)
on the issue.
I did one extra change, since we don't want any indentation tokens I am
setting the `State::Other` as the initial state of the Lexer.
Remaining work:
- [x] Add a test case for red-knot.
- [x] Add more tests.
## Test Plan
Added a test which previously failed because quoted annotation contained
indentation.
Added an mdtest for red-knot.
Updated previous test.
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
Allow links to issues that appear on the same line as the TODO
directive, if they conform to the format that VSCode's GitHub PR
extension produces.
Revival of #9627 (the branch was stale enough that rebasing was a lot
harder than just making the changes anew). Credit should go to the
author of that PR though.
Closes#8061
Co-authored-by: Martin Bernstorff <martinbernstorff@gmail.com>
Instead of doing this on a lint-by-lint basis, we now just do it right
before rendering. This is more broadly applicable.
Note that this doesn't fix the diagnostic rendering for the Python
parser. But that's using a different path anyway (`annotate-snippets` is
only used in tests).
Previously, these were pointing to the right place, but were missing the
`^`. With the `annotate-snippets` upgrade, the `^` was added, but they
started pointing to the end of the previous line instead of the
beginning of the following line. In this case, we really want it to
point to the beginning of the following line since we're calling out
indentation issues.
As in a prior commit, we fix this by tweaking the offsets emitted by the
lint itself. Instead of an empty range at the beginning of the line, we
point to the first character in the line. This "forces" the renderer to
point to the beginning of the line instead of the end of the preceding
line.
The end effect here is that the rendering is fixed by adding `^` in the
proper location.
This update includes some missing `^` in the diagnostic annotations.
This update also includes some shifting of "syntax error" annotations to
the end of the preceding line. I believe this is technically a
regression, but fixing them has proven quite difficult. I *think* the
best way to do that might be to tweak the spans generated by the Python
parser errors, but I didn't want to dig into that. (Another approach
would be to change the `annotate-snippets` rendering, but when I tried
that and managed to fix these regressions, I ended up causing a bunch of
other regressions.)
Ref 77d454525e (r1915458616)
This change also requires some shuffling to the offsets we generate for
the diagnostic. Previously, we were generating an empty range
immediately *after* the line terminator and immediate before the first
byte of the subsequent line. How this is rendered is somewhat open to
interpretation, but the new version of `annotate-snippets` chooses to
render this at the end of the preceding line instead of the beginning of
the following line.
In this case, we want the diagnostic to point to the beginning of the
following line. So we either need to change `annotate-snippets` to
render such spans at the beginning of the following line, or we need to
change our span to point to the first full character in the following
line. The latter will force `annotate-snippets` to move the caret to the
proper location.
I ended up deciding to change our spans instead of changing how
`annotate-snippets` renders empty spans after a line terminator. While I
didn't investigate it, my guess is that they probably had good reason
for doing so, and it doesn't necessarily strike me as _wrong_.
Furthermore, fixing up our spans seems like a good idea regardless, and
was pretty easy to do.
This looks like a bug fix since the caret is now pointing right at the
position of the unprintable character. I'm not sure if this is a result
of an improvement via the `annotate-snippets` upgrade, or because of
more accurate tracking of annotation ranges even after unprintable
characters are replaced. I'm tempted to say the former since in theory
the offsets were never wrong before because they were codepoint offsets.
Regardless, this looks like an improvement.
This updates snapshots where long lines now get trimmed with
`annotate-snippets`. And an ellipsis is inserted to indicate trimming.
This is a little hokey to test since in tests we don't do any styling.
And I believe this just uses the default "max term width" for rendering.
But in real life, it seems like a big improvement to have long lines
trimmed if they would otherwise wrap in the terminal. So this seems like
an improvement to me.
There are some other fixes here that overlap with previous categories.
We do this because `...` is valid Python, which makes it pretty likely
that some line trimming will lead to ambiguous output. So we add support
for overriding the cut indicator. This also requires changing some of
the alignment math, which was previously tightly coupled to `...`.
For Ruff, we go with `…` (`U+2026 HORIZONTAL ELLIPSIS`) for our cut
indicator.
For more details, see the patch sent to upstream:
https://github.com/rust-lang/annotate-snippets-rs/pull/172
This fix was sent upstream and the PR description includes more details:
https://github.com/rust-lang/annotate-snippets-rs/pull/170
Without this fix, there was an errant snapshot diff that looked like
this:
|
1 | version = "0.1.0"
2 | # Ensure that the spans from toml handle utf-8 correctly
3 | authors = [
| ___________^
4 | | { name = "Z͑ͫ̓ͪ̂ͫ̽͏̴̙...A̴̵̜̰͔ͫ͗͢L̠ͨͧͩ͘G̴̻͈͍̑͗̎̅͛́Ǫ̵̹̻̝̳͂̌̌͘", email = 1 }
5 | | ]
| |_^ RUF200
|
That ellipsis should _not_ be inserted since the line is not actually
truncated. The handling of line length (in bytes versus actual rendered
length) wasn't quite being handled correctly in all cases.
With this fix, there's (correctly) no snapshot diff.
The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
https://github.com/rust-lang/annotate-snippets-rs/pull/169
Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
This one almost looks like it fits into the other failure categories,
but without identifying root causes, it's hard to say for sure. The span
here does end after a line terminator, so it feels like it's like the
rest.
I also isolated this change since I found the snapshot diff pretty hard
to read and wanted to look at it more closely. In this case, the before
is:
E204.py:31:2: E204 [*] Whitespace after decorator
|
30 | # E204
31 | @ \
| __^
32 | | foo
| |_^ E204
33 | def baz():
34 | print('baz')
|
= help: Remove whitespace
And the after is:
E204.py:31:2: E204 [*] Whitespace after decorator
|
30 | # E204
31 | @ \
| ^^ E204
32 | foo
33 | def baz():
34 | print('baz')
|
= help: Remove whitespace
The updated rendering is clearly an improvement, since `foo` itself is
not really the subject of the diagnostic. The whitespace is.
Also, the new rendering matches the span fed to `annotate-snippets`,
where as the old rendering does not.
I separated out this snapshot update since the string of `^` including
whitespace looked a little odd. I investigated this one specifically,
and indeed, our span in this case is telling `annotate-snippets` to
point at the whitespace. So this is `annotate-snippets` doing what it's
told with a mildly sub-optimal span.
For clarity, the before rendering is:
skip.py:34:1: I001 [*] Import block is un-sorted or un-formatted
|
32 | import sys; import os # isort:skip
33 | import sys; import os # isort:skip # isort:skip
34 | / import sys; import os
|
= help: Organize imports
And now after is:
skip.py:34:1: I001 [*] Import block is un-sorted or un-formatted
|
32 | import sys; import os # isort:skip
33 | import sys; import os # isort:skip # isort:skip
34 | import sys; import os
| ^^^^^^^^^^^^^^^^^^^^^^^^^ I001
|
= help: Organize imports
This is a clear bug fix since it adds in the `I001` annotation, even
though the carets look a little funny by including the whitespace
preceding `import sys; import os`.
This group of updates is similar to the last one, but they call out the
fact that while the change is an improvement, it does still seem to be a
little buggy.
As one example, previously we would have this:
|
1 | / from __future__ import annotations
2 | |
3 | | from typing import Any
4 | |
5 | | from requests import Session
6 | |
7 | | from my_first_party import my_first_party_object
8 | |
9 | | from . import my_local_folder_object
10 | |
11 | |
12 | |
13 | | class Thing(object):
| |_^ I001
14 | name: str
15 | def __init__(self, name: str):
|
= help: Organize imports
And now here's what it looks like after:
|
1 | / from __future__ import annotations
2 | |
3 | | from typing import Any
4 | |
5 | | from requests import Session
6 | |
7 | | from my_first_party import my_first_party_object
8 | |
9 | | from . import my_local_folder_object
10 | |
11 | |
12 | |
| |__^ Organize imports
13 | class Thing(object):
14 | name: str
15 | def __init__(self, name: str):
|
= help: Organize imports
So at least now, the diagnostic is not pointing to a completely
unrelated thing (`class Thing`), but it's still not quite pointing to
the imports directly. And the `^` is a bit offset. After looking at
some examples more closely, I think this is probably more of a bug
with how we're generating offsets, since we are actually pointing to
a location that is a few empty lines _below_ the last import. And
`annotate-snippets` is rendering that part correctly. However, the
offset from the left (the `^` is pointing at `r` instead of `f` or even
at the end of `from . import my_local_folder_object`) appears to be a
problem with `annotate-snippets` itself.
We accept this under the reasoning that it's an improvement, albeit not
perfect.
I believe this case is different from the last in that it happens when
the end of a *multi-line* annotation occurs after a line terminator.
Previously, the diagnostic would render on the next line, which is
definitely a bit weird. This new update renders it at the end of the
line the annotation ends on.
In some cases, the annotation was previously rendered to point at source
lines below where the error occurred, which is probably pretty
confusing.
This looks like a bug fix that occurs when the annotation is a
zero-width span immediately following a line terminator. Previously, the
caret seems to be rendered on the next line, but it should be rendered
at the end of the line the span corresponds to.
I admit that this one is kinda weird. I would somewhat expect that our
spans here are actually incorrect, and that to obtain this sort of
rendering, we should identify a span just immediately _before_ the line
terminator and not after it. But I don't want to dive into that rabbit
hole for now (and given how `annotate-snippets` now renders these
spans, perhaps there is more to it than I see), and this does seem like
a clear improvement given the spans we feed to `annotate-snippets`.
The previous rendering just seems wrong in that a `^` is omitted. The
new version of `annotate-snippets` seems to get this right. I checked a
pseudo random sample of these, and it seems to only happen when the
position pointed at a line terminator.
It's hard to grok the change from the snapshot diffs alone, so here's
one example. Before:
PYI021.pyi:15:5: PYI021 [*] Docstrings should not be included in stubs
|
14 | class Baz:
15 | """Multiline docstring
| _____^
16 | |
17 | | Lorem ipsum dolor sit amet
18 | | """
| |_______^ PYI021
19 |
20 | def __init__(self) -> None: ...
|
= help: Remove docstring
And now after:
PYI021.pyi:15:5: PYI021 [*] Docstrings should not be included in stubs
|
14 | class Baz:
15 | / """Multiline docstring
16 | |
17 | | Lorem ipsum dolor sit amet
18 | | """
| |_______^ PYI021
19 |
20 | def __init__(self) -> None: ...
|
= help: Remove docstring
I personally think both of these are fine. If we felt strongly, I could
investigate reverting to the old style, but the new style seems okay to
me.
In other words, these updates I believe are just cosmetic and not a bug
fix.
These updates center around the addition of annotations in the
diagnostic rendering. Previously, the annotation was just not rendered
at all. With the `annotate-snippets` upgrade, it is now rendered. I
examined a pseudo random sample of these, and they all look correct.
As will be true in future batches, some of these snapshots also have
changes to whitespace in them as well.
These snapshot changes should *all* only be a result of changes to
trailing whitespace in the output. I checked a psuedo random sample of
these, and the whitespace found in the previous snapshots seems to be an
artifact of the rendering and _not_ of the source data. So this seems
like a strict bug fix to me.
There are other snapshots with whitespace changes, but they also have
other changes that we split out into separate commits. Basically, we're
going to do approximately one commit per category of change.
This represents, by far, the biggest chunk of changes to snapshots as a
result of the `annotate-snippets` upgrade.
Previously, we were replacing unprintable ASCII characters with a
printable representation of them via fancier Unicode characters. Since
`annotate-snippets` used to use codepoint offsets, this didn't make our
ranges incorrect: we swapped one codepoint for another.
But now, with the `annotate-snippets` upgrade, we use byte offsets
(which is IMO the correct choice). However, this means our ranges can be
thrown off since an ASCII codepoint is always one byte and a non-ASCII
codepoint is always more than one byte.
Instead of tweaking the `ShowNonprinting` trait and making it more
complicated (which is used in places other than this diagnostic
rendering it seems), we instead change `replace_whitespace` to handle
non-printable characters. This works out because `replace_whitespace`
was already updating the annotation range to account for the tab
replacement. We copy that approach for unprintable characters.
This is pretty much just moving to the new API and taking care to use
byte offsets. This is *almost* enough. The next commit will fix a bug
involving the handling of unprintable characters as a result of
switching to byte offsets.
This is a tiny change that, perhaps slightly shady, permits us to use
the `annotate-snippets` renderer without its mandatory header (which
wasn't there in `annotate-snippets 0.9`). Specifically, we can now do
this:
Level::None.title("")
The combination of a "none" level and an empty label results in the
`annotate-snippets` header being skipped entirely. (Not even an empty
line is written.)
This is maybe not the right API for upstream `annotate-snippets`, but
it's very easy for us to do and unblocks the upgrade (albeit relying on
a vendored copy).
Ref https://github.com/rust-lang/annotate-snippets-rs/issues/167
This merely adds the crate to our repository. Some cosmetic changes are
made to make it work in our repo and follow our conventions, such as
changing the name to `ruff_annotate_snippets`. We retain the original
license information. We do drop some things, such as benchmarks, but
keep tests and examples.
## Summary
The initial purpose was to fix#15043, where code like this:
```python
from fastapi import FastAPI, Query
app = FastAPI()
@app.get("/test")
def handler(echo: str = Query("")):
return echo
```
was being fixed to the invalid code below:
```python
from typing import Annotated
from fastapi import FastAPI, Query
app = FastAPI()
@app.get("/test")
def handler(echo: Annotated[str, Query("")]): # changed
return echo
```
As @MichaReiser pointed out, the correct fix is:
```python
from typing import Annotated
from fastapi import FastAPI, Query
app = FastAPI()
@app.get("/test")
def handler(echo: Annotated[str, Query()] = ""): # changed
return echo
```
After fixing the issue for `Query`, I realized that other classes like
`Path`, `Body`, `Cookie`, `Header`, `File`, and `Form` also looked
susceptible to this issue. The last few commits should handle these too,
which I think means this will also close#12913.
I had to reorder the arguments to the `do_stuff` test case because the
new fix removes some default argument values (eg for `Path`:
`some_path_param: str = Path()` becomes `some_path_param: Annotated[str,
Path()]`).
There's also #14484 related to this rule. I'm happy to take a stab at
that here or in a follow up PR too.
## Test Plan
`cargo test`
I also checked the fixed output with `uv run --with fastapi
FAST002_0.py`, but it required making a bunch of additional changes to
the test file that I wasn't sure we wanted in this PR.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
If `S <: T`, then `~T <: ~S`. This test currently fails with example
like:
```
S = tuple[()]
T = ~Literal[True] & ~Literal[False]
```
`T` is equivalent to `~(Literal[True] | Literal[False])` and therefore
equivalent to `~bool`, but the minimal example for a failure is what is
stated above. We correctly recognize that `S <: T`, but fail to see that
`~T <: ~S`, i.e. `bool <: ~tuple[()]`.
This is why the tests goes into the "flaky" section as well.
## Test Plan
```
export QUICKCHECK_TESTS=100000
while cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::flaky::negation_reverses_subtype_order; do :; done
```
## Summary
Adds some initial tests for class and instance attributes, mostly to
document (and discuss) what we want to support eventually. These
tests are not exhaustive yet. The idea is to specify the coarse-grained
behavior first.
Things that we'll eventually want to test:
- Interplay with inheritance
- Support `Final` in addition to `ClassVar`
- Specific tests for `ClassVar`, like making sure that we support things
like `x: Annotated[ClassVar[int], "metadata"]`
- … or making sure that we raise an error here:
```py
class Foo:
def __init__(self):
self.x: ClassVar[str] = "x"
```
- Add tests for `__new__` in addition to the tests for `__init__`
- Add tests that show that we use the union of types if multiple methods
define the symbol with different types
- Make sure that diagnostics are raised if, e.g., the inferred type of
an assignment within a method does not match the declared type in the
class body.
- https://github.com/astral-sh/ruff/pull/15474#discussion_r1916556284
- Method calls are completely left out for now.
- Same for `@property`
- … and the descriptor protocol
## Test Plan
New Markdown tests
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
The next sync of typeshed would have failed without manual changes
anyway, so I'm doing one manual sync + the required changes in our
`sys.platform` tests (which are necessary because of my tiny typeshed PR
here: https://github.com/python/typeshed/pull/13378).
closes#15485 (the next run of the pipeline in two weeks should be fine
as the bug has been fixed upstream)
## Summary
Adds two additional tests for `is_equivalent_to` so that we cover all
properties of an [equivalence relation].
## Test Plan
```
while cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stable; do :; done
```
[equivalence relation]:
https://en.wikipedia.org/wiki/Equivalence_relation
## Summary
This PR fixes the `show_*_msg` macros to pass all the tokens instead of
just a single token. This allows for using various expressions right in
the macro similar to how it would be in `format_args!`.
## Test Plan
`cargo clippy`
## Summary
This PR creates separate functions to check whether the document path is
excluded for linting or formatting. The main motivation is to avoid the
double `Option` for the call sites and makes passing the correct
settings simpler.
## Summary
This changeset adds new tests for public uses of symbols,
considering all possible declaredness and boundness states.
Note that this is a mere documentation of the current behavior. There is
still an [open ticket] questioning some of these choices (or unintential
behaviors).
## Test plan
Made sure that the respective test fails if I add the questionable case
again in `symbol_by_id`:
```rs
Symbol::Type(inferred_ty, Boundness::Bound) => {
Symbol::Type(inferred_ty, Boundness::Bound)
}
```
[open ticket]: https://github.com/astral-sh/ruff/issues/14297
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
Replace typo "security_managr" in AIR303 as "security_manager"
## Test Plan
<!-- How was it tested? -->
a test fixture has been updated
## Summary
Simplification follow-up to #15413.
There's no need to have a dedicated `CallOutcome` variant for every
known function, it's only necessary if the special-cased behavior of the
known function includes emitting extra diagnostics. For `typing.cast`,
there's no such need; we can use the regular `Callable` outcome variant,
and update the return type according to the cast. (This is the same way
we already handle `len`.)
One reason to avoid proliferating unnecessary `CallOutcome` variants is
that currently we have to explicitly add emitting call-binding
diagnostics, for each outcome variant. So we were previously wrongly
silencing any binding diagnostics on calls to `typing.cast`. Fixing this
revealed a separate bug, that we were emitting a bogus error anytime
more than one keyword argument mapped to a `**kwargs` parameter. So this
PR also adds test and fix for that bug.
## Test Plan
Existing `cast` tests pass unchanged, added new test for `**kwargs` bug.
## Summary
I noticed this while trying out
https://github.com/astral-sh/ruff-vscode/issues/665 that we use the
`Display` implementation to show the error which hides the context. This
PR changes it to use the `Debug` implementation and adds the message as
a context.
## Test Plan
**Before:**
```
0.001228084s ERROR main ruff_server::session::index::ruff_settings: Unable to find editor-specified configuration file: Failed to parse /private/tmp/hatch-test/ruff.toml
```
**After:**
```
0.002348750s ERROR main ruff_server::session::index::ruff_settings: Unable to load editor-specified configuration file
Caused by:
0: Failed to parse /private/tmp/hatch-test/ruff.toml
1: TOML parse error at line 2, column 18
|
2 | extend-select = ["ASYNC101"]
| ^^^^^^^^^^
Unknown rule selector: `ASYNC101`
```
## Summary
In `SymbolState` merging, use `BitSet::union` instead of inserting
declarations one by one. This used to be the case but was changed in
https://github.com/astral-sh/ruff/pull/15019 because we had to iterate
over declarations anyway.
This is an alternative to https://github.com/astral-sh/ruff/pull/15419
by @MichaReiser. It's similar in performance, but a bit more
declarative and less imperative.
## Summary
Follow-up PR from https://github.com/astral-sh/ruff/pull/15415🥲
The exact same property test already exists:
`intersection_assignable_to_both` and
`all_type_pairs_can_be_assigned_from_their_intersection`
## Test Plan
`cargo test -p red_knot_python_semantic -- --ignored
types::property_tests::flaky`
## Summary
Implements upstream diagnostics `PT029`, `PT030`, `PT031` that function
as pytest.warns corollaries of `PT010`, `PT011`, `PT012` respectively.
Most of the implementation and documentation is designed to mirror those
existing diagnostics.
Closes#14239
## Test Plan
Tests for `PT029`, `PT030`, `PT031` largely copied from `PT010`,
`PT011`, `PT012` respectively.
`cargo nextest run`
---------
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
A small PR to reduce some of the code duplication between the various
branches, make it a little more readable and move the API closer to what
we already have for `KnownClass`
## Summary
The cause of this bug is from
https://github.com/astral-sh/ruff/pull/12575 which was itself a bug fix
but the fix wasn't completely correct.
fixes: #14768
fixes: https://github.com/astral-sh/ruff-vscode/issues/644
## Test Plan
Consider the following three cells:
1.
```python
class Foo:
def __init__(self):
self.x = 1
def __str__(self):
return f"Foo({self.x})"
```
2.
```python
def hello():
print("hello world")
```
3.
```python
y = 1
```
The test case is moving cell 2 to the top i.e., cell 2 goes to position
1 and cell 1 goes to position 2.
Before this fix, it can be seen that the cells were pushed at the end of
the vector:
```
12.643269917s INFO ruff:main ruff_server::edit:📓 Before update: [
NotebookCell {
document: TextDocument {
contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"",
},
},
NotebookCell {
document: TextDocument {
contents: "def hello():\n print(\"hello world\")",
},
},
NotebookCell {
document: TextDocument {
contents: "y = 1",
},
},
]
12.643777667s INFO ruff:main ruff_server::edit:📓 After update: [
NotebookCell {
document: TextDocument {
contents: "y = 1",
},
},
NotebookCell {
document: TextDocument {
contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"",
},
},
NotebookCell {
document: TextDocument {
contents: "def hello():\n print(\"hello world\")",
},
},
]
```
After the fix in this PR, it can be seen that the cells are being pushed
at the correct `start` index:
```
6.520570917s INFO ruff:main ruff_server::edit:📓 Before update: [
NotebookCell {
document: TextDocument {
contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"",
},
},
NotebookCell {
document: TextDocument {
contents: "def hello():\n print(\"hello world\")",
},
},
NotebookCell {
document: TextDocument {
contents: "y = 1",
},
},
]
6.521084792s INFO ruff:main ruff_server::edit:📓 After update: [
NotebookCell {
document: TextDocument {
contents: "def hello():\n print(\"hello world\")",
},
},
NotebookCell {
document: TextDocument {
contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"",
},
},
NotebookCell {
document: TextDocument {
contents: "y = 1",
},
},
]
```
## Summary
[**Rendered version of the new test
suite**](https://github.com/astral-sh/ruff/blob/david/intersection-type-tests/crates/red_knot_python_semantic/resources/mdtest/intersection_types.md)
Moves most of our existing intersection-types tests to a dedicated
Markdown test suite, extends the test coverage, unifies the notation for
these tests, groups tests into a proper structure, and adds some
explanations for various simplification strategies.
This changeset also:
- Adds a new simplification where `~Never` is removed from
intersections.
- Adds a new simplification where adding `~object` simplifies the whole
intersection to `Never`
- Avoids unnecessary assignment-checks between inferred and declared
type. This was added to this changeset to avoid many false positive
errors in this test suite.
Resolves the task described in this old comment
[here](e01da82a5a..e7e432bca2 (r1819924085)).
## Test Plan
Running the new Markdown tests
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Prompted by
> One nit: I think we need to consider `Any` and `Unknown` and `Todo` as
all (gradually) equivalent to each other, and thus `type & Any` and
`type & Unknown` and `type & Todo` as also equivalent. The distinction
between `Any` vs `Unknown` vs `Todo` is entirely about
provenance/debugging, there is no type level distinction. (And I've been
wondering if the `Any` vs `Unknown` distinction is really worth it.)
The thought here is that _most_ places want to treat `Any`, `Unknown`,
and `Todo` identically. So this PR simplifies things by having a single
`Type::Any` variant, and moves the provenance part into a new `AnyType`
type. If you need to treat e.g. `Todo` differently, you still can by
pattern-matching into the `AnyType`. But if you don't, you can just use
`Type::Any(_)`.
(This would also allow us to (more easily) distinguish "unknown via an
unannotated value" from "unknown because of a typing error" should we
want to do that in the future)
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Carl Meyer <carl@astral.sh>
## Summary
This moves almost all of our existing `UnionBuilder` tests to a
Markdown-based test suite.
I see how this could be a more controversial change, since these tests
where written specifically for `UnionBuilder`, and by creating the union
types using Python type expressions, we add an additional layer on top
(parsing and inference of these expressions) that moves these tests away
from clean unit tests more in the direction of integration tests. Also,
there are probably a few implementation details of `UnionBuilder` hidden
in the test assertions (e.g. order of union elements after
simplifications).
That said, I think we would like to see all those properties that are
being tested here from *any* implementation of union types. And the
Markdown tests come with the usual advantages:
- More consice
- Better readability
- No re-compiliation when working on tests
- Easier to add additional explanations and structure to the test suite
This changeset adds a few additional tests, but keeps the logic of the
existing tests except for a few minor modifications for consistency.
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: T-256 <132141463+T-256@users.noreply.github.com>
## Summary
The symlink-approach in the typeshed-sync workflow caused some problems
on Windows, even though it seemed to work fine in CI:
https://github.com/astral-sh/ruff/pull/15138#issuecomment-2578642129
Here, we rely on `build.rs` to patch typeshed instead, which allows us
to get rid of the modifications in the workflow (thank you
@MichaReiser for the idea).
## Test Plan
- Made sure that changes to `knot_extensions.pyi` result in a recompile
of `red_knot_vendored`.
Stabilise [`slice-to-remove-prefix-or-suffix`](https://docs.astral.sh/ruff/rules/slice-to-remove-prefix-or-suffix/) (`FURB188`) for the Ruff 0.9 release.
This is a stylistic rule, but I think it's a pretty uncontroversial one. There are no open issues or PRs regarding it and it's been in preview for a while now.
Douglas Creager
Mike Perlov
Adam Johnson
Alex Waygood
InSync
Micha Reiser
Brent Westbrook
Dhruv Manilawala
Darius Carrier
Carl Meyer
Vasco Schiavo
David Peter
Muspi Merol
Ari Pollak
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
Alexander Nordin
Justin Bramley
renovate[bot]
Tom Kuson
Leo Gaskin
Garrett Reynolds
Marek Hanuš
Marcus Näslund
Charlie Marsh
Jelle Zijlstra
Zanie Blue
Ankit Chaurasia
Chandra Kiran G
Calum Young
wooly18
Will Lachance
Akira Noda
guillaumeLepape
Auguste Lalande
Aleksei Latyshev
Josiah Outram Halstead