Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Avoid undeclared path when raising conflicting declarations #14958

Merged
merged 3 commits into from
Dec 17, 2024
Merged

[red-knot] Avoid undeclared path when raising conflicting declarations #14958

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa06c14
[red-knot] Avoid undeclared path when raising conflicting declarations
dhruvmanila Dec 13, 2024
cfdab33
Fix pre-commit, update benchmark diagnostic list
dhruvmanila Dec 13, 2024
9967298
Move logic to `declarations_ty`
dhruvmanila Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions crates/red_knot_python_semantic/resources/mdtest/declaration/error.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,44 @@ def _(flag: bool):
x = 1 # error: [conflicting-declarations] "Conflicting declared types for `x`: str, int"
```

## Partial declarations
## Incompatible declarations with bad assignment

```py
def _(flag: bool):
if flag:
x: str
else:
x: int

x = 1 # error: [conflicting-declarations] "Conflicting declared types for `x`: Unknown, int"
# error: [conflicting-declarations]
# error: [invalid-assignment]
x = b"foo"
```

## Incompatible declarations with bad assignment
## No errors

Currently, we avoid raising the conflicting-declarations for the following cases:

### Partial declarations

```py
def _(flag: bool):
if flag:
x: str
else:
x: int

# error: [conflicting-declarations]
# error: [invalid-assignment]
x = b"foo"
x = 1
```

### Partial declarations in try-except

Refer to <https://github.com/astral-sh/ruff/issues/13966>

```py
def _():
try:
x: int = 1
except:
x = 2

x = 3
```
4 changes: 4 additions & 0 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ pub enum Type<'db> {
}

impl<'db> Type<'db> {
pub const fn is_unknown(&self) -> bool {
matches!(self, Type::Unknown)
}

pub const fn is_never(&self) -> bool {
matches!(self, Type::Never)
}
Expand Down
5 changes: 5 additions & 0 deletions crates/red_knot_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,11 @@ impl<'db> TypeInferenceBuilder<'db> {
let mut bound_ty = ty;
let declared_ty = declarations_ty(self.db, declarations, undeclared_ty).unwrap_or_else(
|(ty, conflicting)| {
if let Some(undeclared_ty) = undeclared_ty {
if conflicting.iter().any(|ty| ty == &undeclared_ty) {
return ty;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has an issue: there could be three or more conflicting types, e.g. int, str, and Unknown; this should still emit a diagnostic for the conflict between int and str.

It's also a little weird that we just search for Unknown, because there could be an actual annotation x: Foo in one path, where Foo resolves to Unknown (e.g. because it is imported from a module we can't find), and we would still treat this Unknown the same as the undeclared-path Unknown.

I think it would be easier to make a more correct fix if we do it inside declarations_ty instead, so that in the cases where we don't want to emit the diagnostic, we don't even return an Err result at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, thanks for pointing that out. I've pushed a commit that moves the logic to declarations_ty which doesn't consider the undeclared_ty when looking for conflicting types. It does add the type to the final declared type.

// TODO point out the conflicting declarations in the diagnostic?
let symbol_table = self.index.symbol_table(binding.file_scope(self.db));
let symbol_name = symbol_table.symbol(binding.symbol(self.db)).name();
Expand Down
6 changes: 0 additions & 6 deletions crates/ruff_benchmark/benches/red_knot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,23 +31,17 @@ static EXPECTED_DIAGNOSTICS: &[&str] = &[
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:98:12 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:101:12 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:104:14 Name `char` used when possibly not defined",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:108:17 Conflicting declared types for `second_char`: Unknown, str | None",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:115:14 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:126:12 Name `char` used when possibly not defined",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:267:9 Conflicting declared types for `char`: Unknown, str | None",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:348:20 Name `nest` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:353:5 Name `nest` used when possibly not defined",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:364:9 Conflicting declared types for `char`: Unknown, str | None",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:381:13 Conflicting declared types for `char`: Unknown, str | None",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:395:9 Conflicting declared types for `char`: Unknown, str | None",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:453:24 Name `nest` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:455:9 Name `nest` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:482:16 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:566:12 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:573:12 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:579:12 Name `char` used when possibly not defined",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:580:63 Name `char` used when possibly not defined",
"error[lint:conflicting-declarations] /src/tomllib/_parser.py:590:9 Conflicting declared types for `char`: Unknown, str | None",
Comment on lines -34 to -50
Copy link
Contributor

Choose a reason for hiding this comment

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

Love to see these go away!

"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:629:38 Name `datetime_obj` used when possibly not defined",
];

Expand Down
Loading