Skip to content

Commit

Permalink
[red-knot] Avoid undeclared path when raising conflicting declarations (
Browse files Browse the repository at this point in the history
astral-sh#14958)

## Summary

This PR updates the logic when raising conflicting declarations
diagnostic to avoid the undeclared path if present.

The conflicting declaration diagnostics is added when there are two or
more declarations in the control flow path of a definition whose type
isn't equivalent to each other. This can be seen in the following
example:

```py
if flag:
	x: int
x = 1  # conflicting-declarations: Unknown, int
```

After this PR, we'd avoid considering "Unknown" as part of the
conflicting declarations. This means we'd still flag it for the
following case:

```py
if flag:
	x: int
else:
	x: str
x = 1  # conflicting-declarations: int, str
```

A solution that's local to the exception control flow was also explored
which required updating the logic for merging the flow snapshot to avoid
considering declarations using a flag. This is preserved here:
https://github.com/astral-sh/ruff/compare/dhruv/control-flow-no-declarations?expand=1.

The main motivation to avoid that is we don't really understand what the
user experience is w.r.t. the Unknown type and the
conflicting-declaration diagnostics. This makes us unsure on what the
right semantics are as to whether that diagnostics should be raised or
not and when to raise them. For now, we've decided to move forward with
this PR and could decide to adopt another solution or remove the
conflicting-declaration diagnostics in the future.

Closes: astral-sh#13966 

## Test Plan

Update the existing mdtest case. Add an additional case specific to
exception control flow to verify that the diagnostic is not being raised
now.
  • Loading branch information
dhruvmanila authored Dec 17, 2024
1 parent 4ddf922 commit dcdc6e7
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,6 @@ def _(flag: bool):
def __call__(self) -> int: ...

a = NonCallable()
# error: "Object of type `Literal[1] | Literal[__call__]` is not callable (due to union element `Literal[1]`)"
reveal_type(a()) # revealed: Unknown | int
# error: "Object of type `Literal[__call__] | Literal[1]` is not callable (due to union element `Literal[1]`)"
reveal_type(a()) # revealed: int | Unknown
```
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,17 @@ def _(flag: bool):
x = 1 # error: [conflicting-declarations] "Conflicting declared types for `x`: str, int"
```

## Partial declarations
## Incompatible declarations for 2 (out of 3) types

```py
def _(flag: bool):
if flag:
def _(flag1: bool, flag2: bool):
if flag1:
x: str
elif flag2:
x: int

x = 1 # error: [conflicting-declarations] "Conflicting declared types for `x`: Unknown, int"
# Here, the declared type for `x` is `int | str | Unknown`.
x = 1 # error: [conflicting-declarations] "Conflicting declared types for `x`: str, int"
```

## Incompatible declarations with bad assignment
Expand All @@ -42,3 +45,31 @@ def _(flag: bool):
# error: [invalid-assignment]
x = b"foo"
```

## No errors

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

### Partial declarations

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

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
```
46 changes: 27 additions & 19 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@ type DeclaredTypeResult<'db> = Result<Type<'db>, (Type<'db>, Box<[Type<'db>]>)>;
/// `Ok(declared_type)`. If there are conflicting declarations, returns
/// `Err((union_of_declared_types, conflicting_declared_types))`.
///
/// If undeclared is a possibility, `undeclared_ty` type will be part of the return type (and may
/// conflict with other declarations.)
/// If undeclared is a possibility, `undeclared_ty` type will be part of the return type but it
/// will not be considered to be conflicting with any other types.
///
/// # Panics
/// Will panic if there are no declarations and no `undeclared_ty` is provided. This is a logic
Expand All @@ -304,27 +304,31 @@ fn declarations_ty<'db>(
declarations: DeclarationsIterator<'_, 'db>,
undeclared_ty: Option<Type<'db>>,
) -> DeclaredTypeResult<'db> {
let decl_types = declarations.map(|declaration| declaration_ty(db, declaration));
let mut declaration_types = declarations.map(|declaration| declaration_ty(db, declaration));

let mut all_types = undeclared_ty.into_iter().chain(decl_types);

let first = all_types.next().expect(
"declarations_ty must not be called with zero declarations and no may-be-undeclared",
);
let Some(first) = declaration_types.next() else {
if let Some(undeclared_ty) = undeclared_ty {
// Short-circuit to return the undeclared type if there are no declarations.
return Ok(undeclared_ty);
}
panic!("declarations_ty must not be called with zero declarations and no undeclared_ty");
};

let mut conflicting: Vec<Type<'db>> = vec![];
let declared_ty = if let Some(second) = all_types.next() {
let mut builder = UnionBuilder::new(db).add(first);
for other in [second].into_iter().chain(all_types) {
if !first.is_equivalent_to(db, other) {
conflicting.push(other);
}
builder = builder.add(other);
let mut builder = UnionBuilder::new(db).add(first);
for other in declaration_types {
if !first.is_equivalent_to(db, other) {
conflicting.push(other);
}
builder.build()
} else {
first
};
builder = builder.add(other);
}
// Avoid considering the undeclared type for the conflicting declaration diagnostics. It
// should still be part of the declared type.
if let Some(undeclared_ty) = undeclared_ty {
builder = builder.add(undeclared_ty);
}
let declared_ty = builder.build();

if conflicting.is_empty() {
Ok(declared_ty)
} else {
Expand Down Expand Up @@ -447,6 +451,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
6 changes: 0 additions & 6 deletions crates/ruff_benchmark/benches/red_knot.rs
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",
"warning[lint:possibly-unresolved-reference] /src/tomllib/_parser.py:629:38 Name `datetime_obj` used when possibly not defined",
];

Expand Down

0 comments on commit dcdc6e7

Please sign in to comment.