fix [Docs+feature] Consider how Var should be represented in reveal_type and hover #280#2589
fix [Docs+feature] Consider how Var should be represented in reveal_type and hover #280#2589asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Pyrefly’s reveal_type behavior and documentation to better reflect unpinned partial types (showing @_) and to prevent reveal_type(...) from counting as a “first use” that pins partial container types.
Changes:
- Add a non-pinning usage mode in the binder and route
typing.reveal_type/typing_extensions.reveal_typeargument evaluation through it. - Add special-export recognition for
reveal_typeand implement display-only overrides so empty[]/{}reveals print@_when still unpinned. - Add regression coverage and update docs to explain
@_in revealed types and hovers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/error-kinds.mdx | Documents @_ placeholder semantics with a reveal_type example. |
| pyrefly/lib/test/simple.rs | Adds regression test ensuring reveal_type doesn’t pin partial types. |
| pyrefly/lib/export/special.rs | Adds RevealType to SpecialExport and recognizes it from typing/typing_extensions. |
| pyrefly/lib/binding/expr.rs | Introduces Usage::NonPinningRead and uses it for RevealType calls. |
| pyrefly/lib/binding/bindings.rs | Ensures NonPinningRead lookups don’t consume first-use inference. |
| pyrefly/lib/alt/special_calls.rs | Implements reveal display override logic for unpinned empty list/dict assignments. |
Comments suppressed due to low confidence (3)
pyrefly/lib/binding/expr.rs:670
- This non-pinning special-case only triggers when the callee resolves to
SpecialExport::RevealType(i.e.,typing.reveal_type/typing_extensions.reveal_type). However, the typechecker also treats an unimported barereveal_type(...)as a pseudo-builtin inalt/call.rs; in that case this branch won’t run and the argument will be ensured under the normalusage, which can still consume first-use inference and pin partial types. Consider extending this handling to also recognize the bare namereveal_typeso the non-pinning behavior is consistent with the solver’s pseudo-builtin support.
node_index: _,
range: _,
func,
arguments,
}) if self.as_special_export(func) == Some(SpecialExport::RevealType) => {
pyrefly/lib/alt/special_calls.rs:151
- This Forward-chain walk uses an arbitrary
remaining = 100cutoff to avoid infinite loops. That makesreveal_typebehavior depend on chain length and can silently stop resolving to the underlyingKey::Definitionfor legitimately deep chains. Prefer using a visited set (as done elsewhere in bindings) to break cycles deterministically without a magic limit.
if let Some(mut idx) = self.bindings().key_to_idx_hashed_opt(Hashed::new(&key))
{
let mut remaining = 100usize;
let def_ident = loop {
if remaining == 0 {
break None;
}
pyrefly/lib/alt/special_calls.rs:238
- When
reveal_partialis true, this transforms allAnyStyle::Implicitoccurrences in the displayed type intoVar::ZERO(@_). That can misrepresent legitimate implicitAnythat are unrelated to the empty-container placeholder (e.g.,PartialTypeWithUpstreamsCompletedintentionally introducesAnyto prevent upstream Vars from leaking). Consider restricting theAny(Implicit) -> @_substitution to the specific override types you synthesize for empty[]/{}(e.g., only whenreveal_overrideisSome) so realAnyremainsAnyin the reveal output.
if reveal_partial {
display_ty.transform_mut(&mut |t| {
if matches!(t, Type::Any(AnyStyle::Implicit)) {
*t = Var::ZERO.to_type(self.heap);
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@asukaminato0721 could you rebase this? I've made some changes to partial type inference that might have already been in when you did this, but now I've also implemented a fixpoint for strongly-connected components. I think we can probably support a non-usage mode for reveal_type, but it would be nice to know the PR is up to date with the latest changes before I consider it too much - I actually think for the most part it should now be very difficult to get a raw var in the display, because I'm isolating the Var much more aggressively than before It's possible that this isn't worth the rebase, we might want to just close it - I think the original issue is mostly moot at this point because of recent changes |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@stroxler done |
|
Diff from mypy_primer, showing the effect of this PR on open source code: sympy (https://github.com/sympy/sympy)
- ERROR sympy/utilities/tests/test_pickling.py:61:26-39: Argument `list[ModuleType]` is not assignable to parameter `iterable` with type `Iterable[int | [_T](x: _T) -> _T | [_T](x: _T, memo: dict[int, Any] | None = None, _nil: Any = ...) -> _T]` in function `list.extend` [bad-argument-type]
+ ERROR sympy/utilities/tests/test_pickling.py:61:26-39: Argument `list[ModuleType]` is not assignable to parameter `iterable` with type `Iterable[int | [_T](x: _T, memo: dict[int, Any] | None = None, _nil: Any = ...) -> _T | [_T](x: _T) -> _T]` in function `list.extend` [bad-argument-type]
|
Primer Diff Classification➖ 1 neutral | 1 project(s) total
Detailed analysis➖ Neutral (1)sympy (+1, -1)
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (1 heuristic) |
|
Great. I'll try to take a look this week, although I have a hackathon so it might take me some time to get to it. I'm a little surprised it works at all to print Vars downstream, given how partial type inference is now handled inline, so I'll have to take a close look and make sure nothing here is a determinism risk. I would have naively expected to see the pinned value of the Var in the reveal_type the way things are set up, and I'm having trouble understanding why that's not what's happening here. Regardless, thanks for doing this - I expect I'll learn something I didn't know about how Pyrefly works from the review |
Summary
Fixes #280
Added a non‑pinning usage mode in bindings and wired reveal_type calls to use it, so calling reveal_type(x) doesn’t count as a “first use” that pins partial types. This preserves the intended behavior where later real usage (like x.append(1)) still pins the partial.
Updated special‑export handling to recognize
typing.reveal_type/typing_extensions.reveal_type, enabling special handling in the binder.Implemented reveal_type display overrides for empty list/dict assignments so that, when the variable is still unpinned, the printed type shows
@_rather than Unknown. This is display‑only and doesn’t mutate solver state.Test Plan
Added a regression test that matches the docs example (initial reveal shows
list[@_], subsequent reveal after append(1) shows list[int]) and updated the docs to explain@_.