mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-09-30 13:51:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[ty] simplify return type of place_from_declarations (#19884)

Browse source 
## Summary

A [passing
comment](https://github.com/astral-sh/ruff/pull/19711#issuecomment-3169312014)
led me to explore why we didn't report a class attribute as possibly
unbound if it was a method and defined in two different conditional
branches.

I found that the reason was because of our handling of "conflicting
declarations" in `place_from_declarations`. It returned a `Result` which
would be `Err` in case of conflicting declarations.

But we only actually care about conflicting declarations when we are
actually doing type inference on that scope and might emit a diagnostic
about it. And in all cases (including that one), we want to otherwise
proceed with the union of the declared types, as if there was no
conflict.

In several cases we were failing to handle the union of declared types
in the same way as a normal declared type if there was a declared-types
conflict. The `Result` return type made this mistake really easy to
make, as we'd match on e.g. `Ok(Place::Type(...))` and do one thing,
then match on `Err(...)` and do another, even though really both of
those cases should be handled the same.

This PR refactors `place_from_declarations` to instead return a struct
which always represents the declared type we should use in the same way,
as well as carrying the conflicting declared types, if any. This struct
has a method to allow us to explicitly ignore the declared-types
conflict (which is what we want in most cases), as well as a method to
get the declared type and the conflict information, in the case where we
want to emit a diagnostic on the conflict.

## Test Plan

Existing CI; added a test showing that we now understand a
multiply-conditionally-defined method as possibly-unbound.

This does trigger issues on a couple new fuzzer seeds, but the issues
are just new instances of an already-known (and rarely occurring)
problem which I already plan to address in a future PR, so I think it's
OK to land as-is.

I happened to build this initially on top of
https://github.com/astral-sh/ruff/pull/19711, which adds invalid-await
diagnostics, so I also updated some invalid-syntax tests to not await on
an invalid type, since the purpose of those tests is to check the
syntactic location of the `await`, not the validity of the awaited type.
This commit is contained in:
Carl Meyer 2025-08-13 07:17:08 -07:00 • committed by GitHub
parent 5725c4b17f
commit e12747a903
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 239 additions and 198 deletions
Download patch file Download diff file Expand all files Collapse all files

159
crates/ty_python_semantic/src/place.rs
View file

@ -472,12 +472,56 @@ pub(crate) fn place_from_declarations<'db>(
place_from_declarations_impl(db, declarations, RequiresExplicitReExport::No)
}
pub(crate) type DeclaredTypeAndConflictingTypes<'db> =
(TypeAndQualifiers<'db>, Box<indexmap::set::Slice<Type<'db>>>);
type DeclaredTypeAndConflictingTypes<'db> = (
TypeAndQualifiers<'db>,
Option<Box<indexmap::set::Slice<Type<'db>>>>,
);
/// The result of looking up a declared type from declarations; see [`place_from_declarations`].
pub(crate) type PlaceFromDeclarationsResult<'db> =
Result<PlaceAndQualifiers<'db>, DeclaredTypeAndConflictingTypes<'db>>;
pub(crate) struct PlaceFromDeclarationsResult<'db> {
place_and_quals: PlaceAndQualifiers<'db>,
conflicting_types: Option<Box<indexmap::set::Slice<Type<'db>>>>,
}
impl<'db> PlaceFromDeclarationsResult<'db> {
fn conflict(
place_and_quals: PlaceAndQualifiers<'db>,
conflicting_types: Box<indexmap::set::Slice<Type<'db>>>,
) -> Self {
PlaceFromDeclarationsResult {
place_and_quals,
conflicting_types: Some(conflicting_types),
}
}
pub(crate) fn ignore_conflicting_declarations(self) -> PlaceAndQualifiers<'db> {
self.place_and_quals
}
pub(crate) fn into_place_and_conflicting_declarations(
self,
) -> (
PlaceAndQualifiers<'db>,
Option<Box<indexmap::set::Slice<Type<'db>>>>,
) {
(self.place_and_quals, self.conflicting_types)
}
}
impl<'db> From<PlaceAndQualifiers<'db>> for PlaceFromDeclarationsResult<'db> {
fn from(place_and_quals: PlaceAndQualifiers<'db>) -> Self {
PlaceFromDeclarationsResult {
place_and_quals,
conflicting_types: None,
}
}
}
impl<'db> From<Place<'db>> for PlaceFromDeclarationsResult<'db> {
fn from(place: Place<'db>) -> Self {
PlaceFromDeclarationsResult::from(PlaceAndQualifiers::from(place))
}
}
/// A type with declaredness information, and a set of type qualifiers.
///
@ -659,7 +703,8 @@ fn place_by_id<'db>(
ConsideredDefinitions::AllReachable => use_def.all_reachable_declarations(place_id),
};
let declared = place_from_declarations_impl(db, declarations, requires_explicit_reexport);
let declared = place_from_declarations_impl(db, declarations, requires_explicit_reexport)
.ignore_conflicting_declarations();
let all_considered_bindings = || match considered_definitions {
ConsideredDefinitions::EndOfScope => use_def.end_of_scope_bindings(place_id),
@ -668,53 +713,41 @@ fn place_by_id<'db>(
// If a symbol is undeclared, but qualified with `typing.Final`, we use the right-hand side
// inferred type, without unioning with `Unknown`, because it can not be modified.
if let Some(qualifiers) = declared
.as_ref()
.ok()
.and_then(PlaceAndQualifiers::is_bare_final)
{
if let Some(qualifiers) = declared.is_bare_final() {
let bindings = all_considered_bindings();
return place_from_bindings_impl(db, bindings, requires_explicit_reexport)
.with_qualifiers(qualifiers);
}
// Handle bare `ClassVar` annotations by falling back to the union of `Unknown` and the
// inferred type.
match declared {
Ok(PlaceAndQualifiers {
// Handle bare `ClassVar` annotations by falling back to the union of `Unknown` and the
// inferred type.
PlaceAndQualifiers {
place: Place::Type(Type::Dynamic(DynamicType::Unknown), declaredness),
qualifiers,
}) if qualifiers.contains(TypeQualifiers::CLASS_VAR) => {
} if qualifiers.contains(TypeQualifiers::CLASS_VAR) => {
let bindings = all_considered_bindings();
match place_from_bindings_impl(db, bindings, requires_explicit_reexport) {
Place::Type(inferred, boundness) => {
return Place::Type(
UnionType::from_elements(db, [Type::unknown(), inferred]),
boundness,
)
.with_qualifiers(qualifiers);
}
Place::Type(inferred, boundness) => Place::Type(
UnionType::from_elements(db, [Type::unknown(), inferred]),
boundness,
)
.with_qualifiers(qualifiers),
Place::Unbound => {
return Place::Type(Type::unknown(), declaredness).with_qualifiers(qualifiers);
Place::Type(Type::unknown(), declaredness).with_qualifiers(qualifiers)
}
}
}
_ => {}
}
match declared {
// Place is declared, trust the declared type
Ok(
place_and_quals @ PlaceAndQualifiers {
place: Place::Type(_, Boundness::Bound),
qualifiers: _,
},
) => place_and_quals,
place_and_quals @ PlaceAndQualifiers {
place: Place::Type(_, Boundness::Bound),
qualifiers: _,
} => place_and_quals,
// Place is possibly declared
Ok(PlaceAndQualifiers {
PlaceAndQualifiers {
place: Place::Type(declared_ty, Boundness::PossiblyUnbound),
qualifiers,
}) => {
} => {
let bindings = all_considered_bindings();
let boundness_analysis = bindings.boundness_analysis;
let inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
@ -741,10 +774,10 @@ fn place_by_id<'db>(
PlaceAndQualifiers { place, qualifiers }
}
// Place is undeclared, return the union of `Unknown` with the inferred type
Ok(PlaceAndQualifiers {
PlaceAndQualifiers {
place: Place::Unbound,
qualifiers: _,
}) => {
} => {
let bindings = all_considered_bindings();
let boundness_analysis = bindings.boundness_analysis;
let mut inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
@ -786,12 +819,6 @@ fn place_by_id<'db>(
.into()
}
}
// Place has conflicting declared types
Err((declared, _)) => {
// Intentionally ignore conflicting declared types; that's not our problem,
// it's the problem of the module we are importing from.
Place::bound(declared.inner_type()).with_qualifiers(declared.qualifiers())
}
}
// TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
@ -1129,18 +1156,20 @@ impl<'db> DeclaredTypeBuilder<'db> {
}
fn build(mut self) -> DeclaredTypeAndConflictingTypes<'db> {
if !self.conflicting_types.is_empty() {
let type_and_quals = TypeAndQualifiers::new(self.inner.build(), self.qualifiers);
if self.conflicting_types.is_empty() {
(type_and_quals, None)
} else {
self.conflicting_types.insert_before(
0,
self.first_type
.expect("there must be a first type if there are conflicting types"),
);
(
type_and_quals,
Some(self.conflicting_types.into_boxed_slice()),
)
}
(
TypeAndQualifiers::new(self.inner.build(), self.qualifiers),
self.conflicting_types.into_boxed_slice(),
)
}
}
@ -1203,22 +1232,16 @@ fn place_from_declarations_impl<'db>(
);
if let Some(first) = types.next() {
let declared = if let Some(second) = types.next() {
let (declared, conflicting) = if let Some(second) = types.next() {
let mut builder = DeclaredTypeBuilder::new(db);
builder.add(first);
builder.add(second);
for element in types {
builder.add(element);
}
let (union, conflicting) = builder.build();
if !conflicting.is_empty() {
return Err((union, conflicting));
}
union
builder.build()
} else {
first
(first, None)
};
let boundness = match boundness_analysis {
@ -1244,9 +1267,16 @@ fn place_from_declarations_impl<'db>(
},
};
Ok(Place::Type(declared.inner_type(), boundness).with_qualifiers(declared.qualifiers()))
let place_and_quals =
Place::Type(declared.inner_type(), boundness).with_qualifiers(declared.qualifiers());
if let Some(conflicting) = conflicting {
PlaceFromDeclarationsResult::conflict(place_and_quals, conflicting)
} else {
place_and_quals.into()
}
} else {
Ok(Place::Unbound.into())
Place::Unbound.into()
}
}
@ -1281,31 +1311,32 @@ mod implicit_globals {
use crate::semantic_index::{place_table, use_def_map};
use crate::types::{KnownClass, Type};
use super::{Place, PlaceFromDeclarationsResult, place_from_declarations};
use super::{Place, place_from_declarations};
pub(crate) fn module_type_implicit_global_declaration<'db>(
db: &'db dyn Db,
name: &str,
) -> PlaceFromDeclarationsResult<'db> {
) -> PlaceAndQualifiers<'db> {
if !module_type_symbols(db)
.iter()
.any(|module_type_member| module_type_member == name)
{
return Ok(Place::Unbound.into());
return Place::Unbound.into();
}
let Type::ClassLiteral(module_type_class) = KnownClass::ModuleType.to_class_literal(db)
else {
return Ok(Place::Unbound.into());
return Place::Unbound.into();
};
let module_type_scope = module_type_class.body_scope(db);
let place_table = place_table(db, module_type_scope);
let Some(symbol_id) = place_table.symbol_id(name) else {
return Ok(Place::Unbound.into());
return Place::Unbound.into();
};
place_from_declarations(
db,
use_def_map(db, module_type_scope).end_of_scope_symbol_declarations(symbol_id),
)
.ignore_conflicting_declarations()
}
/// Looks up the type of an "implicit global symbol". Returns [`Place::Unbound`] if