Route TSP None through the stdlib's version-aware NoneType

Summary:
Follow-up to D110961669 addressing the review feedback on #4043.

D110961669 correctly stopped emitting the off-spec `none` `BuiltInType` and began encoding `None` as a `NoneType` `ClassType`, but it re-derived the `NoneType` location by name (`types` + `"NoneType"`), hardcoding `ModuleName::types()` and bundled `types.pyi`. That diverges from pyrefly's authoritative `Stdlib::none_type()`, which resolves `NoneType` from `types` only on Python 3.10+ and from `_typeshed` on older versions. On a project targeting < 3.10 the TSP declaration would therefore point at a different module than pyrefly's own `None` resolution and goto-definition.

This routes `None` through the real `Stdlib::none_type()` `ClassType` via the existing `convert_class_type(..., TypeFlags::INSTANCE)` path. `TypeConverter` now takes the resolved class, threaded down from the `server.rs` caller (which already computes `get_stdlib` on the warm transaction). Because the encoding reuses the actual class, it inherits the version-correct module and range automatically and is identical to an explicit `types.NoneType` annotation — no duplicated module/name/path logic. The `types_class`/`types_class_declaration`/`make_types_class_declaration` helpers (and their hardcoded `types.pyi`) are removed; resolver-free unit tests build a stand-in `NoneType` class so they still exercise the production path.

Tests are extended to cover the concrete #4035 symptom: a new interaction test asserts `str | None` round-trips with its `None` member as a reconstructable `NoneType` `ClassType`, and the existing test documents that the downstream Pylance hover (`str | Unknown` → `str | None`) should be confirmed against a real client.

Reviewed By: stroxler

Differential Revision: D111355122

fbshipit-source-id: 0486507ab1ab0b766d8cbca4cbf34309f4433d08

Author: kinto0

pyrefly/lib/lsp/non_wasm/server.rs

@@ -6329,11 +6329,16 @@ impl Server {
         let resolve_export = |module_name: ModuleName, name: &Name| {
             resolve_export_location(transaction, source_handle, module_name, name)
         };
+        // `None` is encoded as the stdlib's version-aware `NoneType` class.
+        // Computing `ty` already populated this transaction's `Stdlib`, so
+        // `get_stdlib` stays on the warm path (see the doc comment above).
+        let stdlib = transaction.get_stdlib(source_handle);
         convert_type_with_resolvers(
             ty,
             Some(&resolve_func_range),
             Some(&resolve_module_path),
             Some(&resolve_export),
+            stdlib.none_type(),
         )
     }
 }

pyrefly/lib/test/tsp/tsp_interaction/get_type_queries.rs

@@ -281,6 +281,14 @@ fn test_get_computed_type_string_is_class() {
 
 #[test]
 fn test_get_computed_type_none_is_class() {
+    // Regression test for https://github.com/facebook/pyrefly/issues/4035:
+    // `None` must be a `NoneType` `ClassType`, not an off-spec `none`
+    // `BuiltInType`, so consumers can reconstruct unions like `str | None`.
+    // The declaration is sourced from the stdlib's `NoneType` class, so it
+    // carries a real location (e.g. `types.pyi`) that matches goto-definition
+    // rather than a synthesized stub. This asserts the wire shape; the
+    // downstream Pylance hover fix (`str | Unknown` → `str | None`) should be
+    // confirmed against a real client, which this test harness cannot drive.
     let (mut tsp, file_uri, snapshot) = setup_project("x = None\n");
 
     let result = get_computed_type_ok(&mut tsp, &file_uri, 0, 0, snapshot);
@@ -290,6 +298,50 @@ fn test_get_computed_type_none_is_class() {
     let name = declaration.get("name").and_then(|v| v.as_str());
     assert_eq!(name, Some("NoneType"), "Expected class name 'NoneType'");
 
+    // The declaration points at NoneType's real definition, not an empty stub.
+    let uri = declaration
+        .get("node")
+        .and_then(|n| n.get("uri"))
+        .and_then(|v| v.as_str())
+        .expect("Expected declaration node URI");
+    assert!(
+        !uri.is_empty(),
+        "Expected a real NoneType definition URI, got empty"
+    );
+
+    tsp.shutdown();
+}
+
+#[test]
+fn test_get_computed_type_str_or_none_union_reconstructs_none() {
+    // The concrete #4035 symptom: `str | None` must round-trip with its `None`
+    // member encoded as a reconstructable `NoneType` `ClassType` (previously an
+    // off-spec `none` `BuiltInType` that Pylance rendered as `Unknown`).
+    let code = "x: str | None = None\n";
+    let (mut tsp, file_uri, snapshot) = setup_project(code);
+
+    tsp.server.get_declared_type(&file_uri, 0, 0, snapshot);
+    let resp = tsp.client.receive_response_skip_notifications();
+    let result = resp.result.expect("Expected result");
+    assert_kind(&result, TypeKind::Union);
+
+    let sub_types = result
+        .get("subTypes")
+        .and_then(|v| v.as_array())
+        .expect("Expected subTypes array");
+    let has_none_class = sub_types.iter().any(|member| {
+        member.get("kind").and_then(|v| v.as_u64()) == Some(TypeKind::Class as u64)
+            && member
+                .get("declaration")
+                .and_then(|d| d.get("name"))
+                .and_then(|v| v.as_str())
+                == Some("NoneType")
+    });
+    assert!(
+        has_none_class,
+        "Expected a NoneType Class member in the union, got {sub_types:?}"
+    );
+
     tsp.shutdown();
 }
 

pyrefly/lib/tsp/type_conversion.rs

@@ -26,9 +26,9 @@
 //!  - `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.
+//! Note: `None` is emitted as a `NoneType` `ClassType`, not as a `BuiltInType`,
+//! because `BuiltInType.name` is restricted to protocol sentinel names. See
+//! [`convert_type_with_resolvers`] for how the version-correct class is sourced.
 //! All `Type` variants are explicitly handled; no types fall through to a
 //! generic `SynthesizedType` stub.
 
@@ -110,16 +110,24 @@ pub type ExportLocationResolver<'a> =
 
 /// Convert a pyrefly `Type` to a TSP protocol `Type` using optional
 /// source-range and module-URI resolvers.
+///
+/// `none_type` is the stdlib's authoritative `NoneType` class, used to encode
+/// `None`. Passing the real class (rather than re-deriving `NoneType` by name)
+/// keeps the declaration version-correct — sourced from `types` on Python 3.10+
+/// and `_typeshed` before — and identical to an explicit `types.NoneType`
+/// annotation.
 pub fn convert_type_with_resolvers<'a>(
     ty: &PyreflyType,
     func_range_resolver: Option<&'a FuncRangeResolver<'a>>,
     module_path_resolver: Option<&'a ModulePathResolver<'a>>,
     export_location_resolver: Option<&'a ExportLocationResolver<'a>>,
+    none_type: &'a PyreflyClassType,
 ) -> TspType {
     TypeConverter {
         resolve_func_range: func_range_resolver,
         resolve_module_path: module_path_resolver,
         resolve_export: export_location_resolver,
+        none_type,
     }
     .convert(ty)
 }
@@ -131,14 +139,43 @@ pub fn convert_type_with_resolvers<'a>(
 /// locations are needed.
 #[cfg(test)]
 pub fn convert_type(ty: &PyreflyType) -> TspType {
-    convert_type_with_resolvers(ty, None, None, None)
+    convert_type_with_resolvers(ty, None, None, None, &test_none_type())
+}
+
+/// Build a stand-in for `Stdlib::none_type()` used by the resolver-free tests,
+/// which have no real stdlib. Mirrors the real class closely enough to exercise
+/// the production `None` path: a top-level `NoneType` class in bundled `types`.
+#[cfg(test)]
+fn test_none_type() -> PyreflyClassType {
+    use pyrefly_python::module::Module;
+    use pyrefly_python::nesting_context::NestingContext;
+    use pyrefly_types::class::ClassDefIndex;
+    use pyrefly_types::types::TArgs;
+    use ruff_python_ast::Identifier;
+
+    let module = Module::new(
+        ModuleName::types(),
+        ModulePath::bundled_typeshed(PathBuf::from("types.pyi")),
+        Arc::new(String::new()),
+    );
+    let class = Class::new(
+        ClassDefIndex(0),
+        Identifier::new(Name::new("NoneType"), TextRange::default()),
+        NestingContext::toplevel(),
+        module,
+        None,
+    );
+    PyreflyClassType::new(class, TArgs::default())
 }
 
 /// Holds an optional range resolver and drives recursive type conversion.
 struct TypeConverter<'a> {
     resolve_func_range: Option<&'a FuncRangeResolver<'a>>,
     resolve_module_path: Option<&'a ModulePathResolver<'a>>,
     resolve_export: Option<&'a ExportLocationResolver<'a>>,
+    /// The stdlib's authoritative `NoneType` class, used to encode `None`; see
+    /// [`convert_type_with_resolvers`].
+    none_type: &'a PyreflyClassType,
 }
 
 impl TypeConverter<'_> {
@@ -148,7 +185,8 @@ impl TypeConverter<'_> {
             // --- Built-in special types ---
             PyreflyType::Any(_) => builtin("any"),
             PyreflyType::Never(_) => builtin("never"),
-            PyreflyType::None => self.types_class("NoneType", TypeFlags::INSTANCE),
+            // `None` → the stdlib's real `NoneType` class (see `none_type`).
+            PyreflyType::None => self.convert_class_type(self.none_type, TypeFlags::INSTANCE),
             PyreflyType::Ellipsis => builtin("ellipsis"),
 
             // --- Class instances (int, str, list[int], user-defined classes, etc.) ---
@@ -662,19 +700,6 @@ 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.
@@ -697,28 +722,6 @@ 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))
-        {
-            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,
@@ -875,21 +878,6 @@ 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"));
-    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.
@@ -1422,7 +1410,13 @@ mod tests {
                 None
             }
         };
-        let tsp = convert_type_with_resolvers(&ty, None, Some(&module_path_resolver), None);
+        let tsp = convert_type_with_resolvers(
+            &ty,
+            None,
+            Some(&module_path_resolver),
+            None,
+            &test_none_type(),
+        );
         match tsp {
             TspType::Module(m) => {
                 assert_eq!(m.module_name, "pkg");
@@ -1499,7 +1493,7 @@ mod tests {
                 range,
             ))
         };
-        match convert_type_with_resolvers(&ty, None, None, Some(&resolver)) {
+        match convert_type_with_resolvers(&ty, None, None, Some(&resolver), &test_none_type()) {
             TspType::Class(c) => {
                 let Declaration::Regular(decl) = c.declaration else {
                     panic!("expected RegularDeclaration");
@@ -1618,7 +1612,7 @@ mod tests {
             ))
         };
         let ty = PyreflyType::TypeAlias(Box::new(TypeAliasData::Ref(make_ref())));
-        match convert_type_with_resolvers(&ty, None, None, Some(&resolver)) {
+        match convert_type_with_resolvers(&ty, None, None, Some(&resolver), &test_none_type()) {
             TspType::Class(c) => {
                 let Declaration::Regular(decl) = c.declaration else {
                     panic!("expected RegularDeclaration");
@@ -1679,7 +1673,7 @@ mod tests {
                 range,
             ))
         };
-        match convert_type_with_resolvers(&ty, None, None, Some(&resolver)) {
+        match convert_type_with_resolvers(&ty, None, None, Some(&resolver), &test_none_type()) {
             TspType::Var(v) => {
                 let Declaration::Regular(decl) = v.declaration else {
                     panic!("expected RegularDeclaration");
@@ -1710,7 +1704,7 @@ mod tests {
                 lsp_types::Range::default(),
             ))
         };
-        match convert_type_with_resolvers(&ty, None, None, Some(&resolver)) {
+        match convert_type_with_resolvers(&ty, None, None, Some(&resolver), &test_none_type()) {
             TspType::Var(v) => {
                 let Declaration::Regular(decl) = v.declaration else {
                     panic!("expected RegularDeclaration");
@@ -1857,22 +1851,19 @@ mod tests {
                 character: 12,
             },
         };
+        // The `None` return converts through the stdlib `NoneType` class, not
+        // the export resolver, so the only lookup here is `typing.overload`;
+        // any other lookup is a regression.
         let resolver = |module: ModuleName, name: &Name| {
             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)) {
+        match convert_type_with_resolvers(&ty, None, None, Some(&resolver), &test_none_type()) {
             TspType::Function(f) => {
                 let Declaration::Regular(decl) = f.declaration else {
                     panic!("expected RegularDeclaration");