Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions pyrefly/lib/test/tsp/tsp_interaction/get_type_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,14 +233,15 @@ fn test_get_computed_type_string_is_class() {
}

#[test]
fn test_get_computed_type_none_is_builtin() {
fn test_get_computed_type_none_is_class() {
let (mut tsp, file_uri, snapshot) = setup_project("x = None\n");

let result = get_computed_type_ok(&mut tsp, &file_uri, 0, 0, snapshot);
assert_kind(&result, TypeKind::Builtin);
assert_kind(&result, TypeKind::Class);

let name = result.get("name").and_then(|v| v.as_str());
assert_eq!(name, Some("none"), "Expected builtin name 'none'");
let declaration = result.get("declaration").expect("Expected declaration");
let name = declaration.get("name").and_then(|v| v.as_str());
assert_eq!(name, Some("NoneType"), "Expected class name 'NoneType'");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This (and the unit tests) verify the wire shape — Class + name NoneType — but not the actual symptom from #4035: that Pylance now renders str | None instead of str | Unknown. The issue notes the consumer models this as builtins.NoneType; since we're emitting types.NoneType, it'd be reassuring to confirm end-to-end against a real Pylance client that the hover round-trips (i.e. that Pylance keys off the class name rather than the builtins module).


tsp.shutdown();
}
Expand Down
126 changes: 108 additions & 18 deletions pyrefly/lib/tsp/type_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@
//! - `Tensor`/`NNModule` → TSP `ClassType` from their base class.
//! - `TypeAlias` → unwraps to the aliased type.
//! - `SpecialForm` → TSP `BuiltInType` with the form name.
//! - `Any`, `Never`, `None`, `Ellipsis` → TSP `BuiltInType`.
//! - `Any`, `Never`, `Ellipsis` → TSP `BuiltInType`.
//! - Solver-internal types → TSP `BuiltInType` with a representative name.
//!
//! Note: `None` is emitted as a `types.NoneType` `ClassType`, not as a
//! `BuiltInType`, because `BuiltInType.name` is restricted to protocol
//! sentinel names.
//! All `Type` variants are explicitly handled; no types fall through to a
//! generic `SynthesizedType` stub.

Expand Down Expand Up @@ -145,7 +148,7 @@ impl TypeConverter<'_> {
// --- Built-in special types ---
PyreflyType::Any(_) => builtin("any"),
PyreflyType::Never(_) => builtin("never"),
PyreflyType::None => builtin("none"),
PyreflyType::None => self.types_class("NoneType", TypeFlags::INSTANCE),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The behavior change here is correct — builtin("none") was off-spec. My design concern is how the target is derived: types_class re-derives the NoneType location by name (types + "NoneType") instead of using pyrefly's authoritative stdlib.none_type(), which already resolves the version-correct location.

Could we instead convert the actual stdlib.none_type() ClassType through the existing convert_class_type(..., TypeFlags::INSTANCE) path? That makes None encode identically to an explicit types.NoneType annotation and inherits the correct module/range automatically, so there's no duplicated module/name/path logic. TypeConverter doesn't hold the stdlib today, but the server.rs caller does (it computes get_stdlib) and could pass the resolved class (or its qname) down. See find_definition_for_none in state/lsp.rs for the existing precedent that follows stdlib.none_type().

PyreflyType::Ellipsis => builtin("ellipsis"),

// --- Class instances (int, str, list[int], user-defined classes, etc.) ---
Expand Down Expand Up @@ -660,6 +663,19 @@ impl TypeConverter<'_> {
})
}

/// Build a TSP `ClassType` whose declaration points at `types.<name>`.
fn types_class(&self, name: &str, flags: TypeFlags) -> TspType {
TspType::Class(TspClassType {
declaration: Declaration::Regular(self.types_class_declaration(name)),
flags,
id: next_id(),
kind: TypeKind::Class,
literal_value: None,
type_alias_info: None,
type_args: None,
})
}

/// Build a class declaration for `typing.<name>`. Resolves the real source
/// range via the export-location resolver; falls back to a zero range
/// pointing at bundled `typing.pyi` when unavailable.
Expand All @@ -682,6 +698,28 @@ impl TypeConverter<'_> {
make_typing_class_declaration(name)
}

/// Build a class declaration for `types.<name>`. Resolves the real source
/// range via the export-location resolver; falls back to a zero range
/// pointing at bundled `types.pyi` when unavailable.
fn types_class_declaration(&self, name: &str) -> RegularDeclaration {
let symbol = Name::new(name);
if let Some((module_path, lsp_range)) = self
.resolve_export
.and_then(|resolve| resolve(ModuleName::types(), &symbol))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Concrete version-correctness issue with hardcoding ModuleName::types(): stdlib.none_type() resolves NoneType from types only on Python 3.10+, and from _typeshed on older versions (see none_location in crates/pyrefly_types/src/stdlib.rs). On a project targeting < 3.10 this lookup points at a different module than pyrefly's own None resolution / goto-definition, so the TSP declaration and the in-editor "go to definition" for None would disagree. Deriving the location from stdlib.none_type()'s qname avoids the divergence.

{
return RegularDeclaration {
kind: DeclarationKind::Regular,
category: DeclarationCategory::Class,
name: Some(name.to_owned()),
node: Node {
range: lsp_range_to_tsp(lsp_range),
uri: path_to_uri(&module_path),
},
};
}
make_types_class_declaration(name)
}

/// Convert a pyrefly `Overload` to a TSP `OverloadedType`.
fn convert_overload_to_tsp(
&self,
Expand Down Expand Up @@ -818,6 +856,21 @@ fn make_typing_class_declaration(name: &str) -> RegularDeclaration {
}
}

/// Build a declaration for a class in `types.pyi`.
fn make_types_class_declaration(name: &str) -> RegularDeclaration {
let module_path =
pyrefly_python::module_path::ModulePath::bundled_typeshed(PathBuf::from("types.pyi"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor, same caveat as the resolver path: this fallback hardcodes types.pyi, which is only correct for 3.10+. It only fires when no export resolver is available (unit tests), so impact is low — but if the location is derived from stdlib.none_type() this hardcoded file (and the whole helper) goes away.

RegularDeclaration {
kind: DeclarationKind::Regular,
category: DeclarationCategory::Class,
name: Some(name.to_owned()),
node: Node {
range: zero_range(),
uri: path_to_uri(&module_path),
},
}
}

/// Build a TSP `TypeVar` with a synthesized declaration. Used for
/// solver-internal TypeVar-like placeholders (Quantified, Args, Kwargs,
/// ElementOfTypeVarTuple) where there is no real source location.
Expand Down Expand Up @@ -1043,8 +1096,20 @@ mod tests {
fn test_convert_none() {
let tsp = convert_type(&PyreflyType::None);
match tsp {
TspType::BuiltInType(b) => assert_eq!(b.name, "none"),
other => panic!("expected BuiltInType, got {other:?}"),
TspType::Class(c) => {
assert!(c.flags.contains(TypeFlags::INSTANCE));
let Declaration::Regular(decl) = c.declaration else {
panic!("expected RegularDeclaration");
};
assert_eq!(decl.name.as_deref(), Some("NoneType"));
assert_eq!(decl.category, DeclarationCategory::Class);
assert!(
decl.node.uri.contains("types.pyi"),
"expected types URI, got {}",
decl.node.uri
);
}
other => panic!("expected Class, got {other:?}"),
}
}

Expand All @@ -1062,8 +1127,8 @@ mod tests {
let a = convert_type(&PyreflyType::None);
let b = convert_type(&PyreflyType::Ellipsis);
let id_a = match &a {
TspType::BuiltInType(b) => b.id,
_ => panic!("expected BuiltInType"),
TspType::Class(c) => c.id,
_ => panic!("expected Class"),
};
let id_b = match &b {
TspType::BuiltInType(b) => b.id,
Expand Down Expand Up @@ -1140,10 +1205,15 @@ mod tests {
assert_eq!(u.kind, TypeKind::Union);
assert_eq!(u.flags, TypeFlags::NONE);
assert_eq!(u.sub_types.len(), 2);
// First member should be BuiltIn "none"
// First member should be `types.NoneType`.
match &u.sub_types[0] {
TspType::BuiltInType(b) => assert_eq!(b.name, "none"),
other => panic!("expected BuiltInType for first member, got {other:?}"),
TspType::Class(c) => {
let Declaration::Regular(decl) = &c.declaration else {
panic!("expected RegularDeclaration");
};
assert_eq!(decl.name.as_deref(), Some("NoneType"));
}
other => panic!("expected Class for first member, got {other:?}"),
}
// Second member should be BuiltIn "any"
match &u.sub_types[1] {
Expand Down Expand Up @@ -1663,14 +1733,27 @@ mod tests {
let specialized = f.specialized_types.expect("expected specialized_types");
assert_eq!(specialized.parameter_types.len(), 2);
match &specialized.parameter_types[0] {
TspType::BuiltInType(b) => assert_eq!(b.name, "none"),
other => panic!("expected BuiltInType, got {other:?}"),
TspType::Class(c) => {
let Declaration::Regular(decl) = &c.declaration else {
panic!("expected RegularDeclaration");
};
assert_eq!(decl.name.as_deref(), Some("NoneType"));
}
other => panic!("expected Class, got {other:?}"),
}
match &specialized.parameter_types[1] {
TspType::BuiltInType(b) => assert_eq!(b.name, "ellipsis"),
other => panic!("expected BuiltInType, got {other:?}"),
}
assert!(specialized.return_type.is_some());
match specialized.return_type.as_deref() {
Some(TspType::Class(c)) => {
let Declaration::Regular(decl) = &c.declaration else {
panic!("expected RegularDeclaration");
};
assert_eq!(decl.name.as_deref(), Some("NoneType"));
}
other => panic!("expected NoneType Class return type, got {other:?}"),
}
}
other => panic!("expected Function, got {other:?}"),
}
Expand Down Expand Up @@ -1700,12 +1783,19 @@ mod tests {
},
};
let resolver = |module: ModuleName, name: &Name| {
assert_eq!(module, ModuleName::typing());
assert_eq!(name.as_str(), "overload");
Some((
ModulePath::filesystem(PathBuf::from("/typeshed/typing.pyi")),
range,
))
if module == ModuleName::typing() && name.as_str() == "overload" {
return Some((
ModulePath::filesystem(PathBuf::from("/typeshed/typing.pyi")),
range,
));
}
if module == ModuleName::types() && name.as_str() == "NoneType" {
return Some((
ModulePath::filesystem(PathBuf::from("/typeshed/types.pyi")),
range,
));
}
panic!("unexpected export lookup for {module}.{name}");
};
match convert_type_with_resolvers(&ty, None, None, Some(&resolver)) {
TspType::Function(f) => {
Expand Down
Loading