Fix TSP None type encoding by WilliamK112 · Pull Request #4043 · facebook/pyrefly

WilliamK112 · 2026-07-04T13:28:21Z

Summary

This changes Pyrefly's TSP conversion for None from an off-spec BuiltInType named none to a ClassType instance declared as types.NoneType. The protocol's BuiltInType.name contract only allows sentinel names such as unknown, any, ellipsis, never, and noreturn, so encoding None as a class keeps str | None and other unions reconstructable by TSP consumers.

The change also updates the TSP interaction coverage for x = None so the actual wire response is checked as a class declaration rather than a builtin sentinel.

Test Plan

cargo +stable-x86_64-pc-windows-gnu test -p pyrefly tsp::type_conversion::tests:: -- --nocapture
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly test_get_computed_type_none_is_class -- --nocapture
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly tsp::type_conversion::tests::test_function_declaration_resolves_special_function_via_export -- --exact --nocapture
RUSTUP_TOOLCHAIN=stable-x86_64-pc-windows-gnu python test.py --mode cargo --no-test --no-tensor-shapes --no-conformance --no-jsonschema
git diff --check

github-actions · 2026-07-07T22:57:19Z

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

meta-codesync · 2026-07-07T23:12:23Z

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D110961669.

rchiodo

Thanks for tackling this — the core change (stop emitting the off-spec none builtin and encode None as a NoneType ClassType instead) is the right fix for #4035.

My main concern is that the NoneType location is re-derived by name and hardcoded to the types module, which diverges from pyrefly's authoritative, version-aware stdlib.none_type() on Python < 3.10. I'd suggest routing None through the real stdlib.none_type() class so the encoding is version-correct and identical to an explicit types.NoneType annotation. Details inline. It would also be worth an end-to-end Pylance check to confirm the hover symptom actually round-trips.

rchiodo · 2026-07-07T23:48:23Z

            PyreflyType::Any(_) => builtin("any"),
            PyreflyType::Never(_) => builtin("never"),
-            PyreflyType::None => builtin("none"),
+            PyreflyType::None => self.types_class("NoneType", TypeFlags::INSTANCE),


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().

rchiodo · 2026-07-07T23:48:23Z

+        let symbol = Name::new(name);
+        if let Some((module_path, lsp_range)) = self
+            .resolve_export
+            .and_then(|resolve| resolve(ModuleName::types(), &symbol))


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.

rchiodo · 2026-07-07T23:48:23Z

+/// 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"));


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.

rchiodo · 2026-07-07T23:48:23Z

-    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'");


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).

kinto0 · 2026-07-09T17:10:43Z

thanks!! I'll try merging this today. if you don't have a chance to update it, I'll resolve the comments in a follow-up

kinto0 · 2026-07-09T23:45:18Z

doing that now. also fixing a few other sentinel types that had the same issue: bool, int, Sentinel

stroxler

Review automatically exported from Phabricator review in Meta.

Summary: Follow-up to D110961669 addressing the review feedback on facebook#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 facebook#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. Differential Revision: D111355122

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

meta-codesync · 2026-07-11T08:56:55Z

@kinto0 merged this pull request in 9a08906.

Fix TSP None type encoding

6173b93

meta-cla Bot added the cla signed label Jul 4, 2026

github-actions Bot added the size/m label Jul 4, 2026

yangdanny97 assigned kinto0 Jul 6, 2026

rchiodo reviewed Jul 7, 2026

View reviewed changes

kinto0 mentioned this pull request Jul 9, 2026

Fix TSP inferred returns for definition-site computed types #4045

Closed

stroxler approved these changes Jul 10, 2026

View reviewed changes

meta-codesync Bot closed this in 9a08906 Jul 11, 2026

meta-codesync Bot added the Merged label Jul 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix TSP None type encoding#4043

Fix TSP None type encoding#4043
WilliamK112 wants to merge 1 commit into
facebook:mainfrom
WilliamK112:codex/tsp-none-type-4035

WilliamK112 commented Jul 4, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

meta-codesync Bot commented Jul 7, 2026

Uh oh!

rchiodo left a comment

Uh oh!

rchiodo Jul 7, 2026

Uh oh!

rchiodo Jul 7, 2026

Uh oh!

rchiodo Jul 7, 2026

Uh oh!

rchiodo Jul 7, 2026

Uh oh!

kinto0 commented Jul 9, 2026

Uh oh!

kinto0 commented Jul 9, 2026

Uh oh!

stroxler left a comment

Uh oh!

meta-codesync Bot commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

WilliamK112 commented Jul 4, 2026

Summary

Test Plan

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

meta-codesync Bot commented Jul 7, 2026

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

kinto0 commented Jul 9, 2026

Uh oh!

kinto0 commented Jul 9, 2026

Uh oh!

stroxler left a comment

Choose a reason for hiding this comment

Uh oh!

meta-codesync Bot commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants