mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-03 10:22:24 +00:00
[red-knot] Avoid undeclared path when raising conflicting declarations (#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: #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.
This commit is contained in:
parent
4ddf9228f6
commit
dcdc6e7c64
4 changed files with 64 additions and 31 deletions
|
@ -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
|
||||
```
|
||||
|
|
|
@ -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
|
||||
|
@ -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
|
||||
```
|
||||
|
|
|
@ -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
|
||||
|
@ -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 {
|
||||
|
@ -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)
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue