Stop reporting an implicit Any the expectation already absorbed - #4409
Open
IBlackVoid wants to merge 1 commit into
Open
Stop reporting an implicit Any the expectation already absorbed#4409IBlackVoid wants to merge 1 commit into
Any the expectation already absorbed#4409IBlackVoid wants to merge 1 commit into
Conversation
Assigning an empty container display where the target is declared `Any` reported
`implicit-any-empty-container`, warning that an un-inferred placeholder would
leak out as `Any` when the annotation had already accounted for it:
def from_response(response: dict[str, Any]) -> None:
response["meta"] = {"time_to_live": None}
The diagnostic is raised by `sanitize_answer_vars` when an unpinned
`PartialContained` var survives into a binding's answer, and two independent
paths were leaving one behind.
A subscript assignment calls `__setitem__`, which checks the value against its
parameter contextually, and then infers the same expression a second time, bare,
purely to produce the type the subscript narrows to. That second inference mints
a fresh placeholder that nothing ever solves. Separately, checking an expression
against `Any` is skipped because it always succeeds - but succeeding trivially
does not solve the placeholder either, which left the same spurious report on
attribute assignment, return position, default arguments, global assignment and
TypedDict fields.
Pin those placeholders rather than reporting them, in both places, when nothing
can observe them. The target's own type must be known: no unpinned placeholder,
and no `Any` that was inferred rather than declared, since a container pinned
from `{}` propagates the same uncertainty instead of absorbing it. Containment
against the expression's range keeps this to placeholders the expression minted
itself - one belonging to a name it merely mentions outlives the assignment and
keeps its own diagnostic. For subscripts the test runs on the whole target
before distributing over unions, because pinning is global and a solved arm must
not silence what an unsolved arm genuinely leaks.
Because the rule asks whether the placeholder can escape rather than what the
target is, `dict`, `list`, `defaultdict`, a `MutableMapping` protocol and a
hand-written `__setitem__` are covered by the same code with no per-container
branches.
Two behaviours are deliberate and covered by tests. A fully-known target that
cannot hold the value at all, such as `d["k"] = {}` on `dict[str, int]`, now
reports only `Cannot set item`, since nothing can observe a placeholder inside a
value being rejected outright. And unpacked targets still report: `bind_unpacking`
does not contextually type unpacks, so the target's `Any` never reaches the
value.
Fixes facebook#4301
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D114530099. (Because this pull request was imported automatically, there will not be any future comments.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4301.
response["meta"] = {"time_to_live": None}on adict[str, Any]reportedimplicit-any-empty-container, warning that a placeholder would leak out asAnywhen the annotation had already accounted for it.Where the report comes from
Not from checking the assignment.
implicit-any-empty-containeris raised bysanitize_answer_varswhen an unpinnedPartialContainedvar survives into abinding's answer — which is why the
ErrorStyle::Nevercollector on thesubscript path does not suppress it.
Two independent paths were leaving one behind:
The subscript re-inference.
__setitem__is called and checks the valueagainst its parameter contextually — that part is correct. The code then
infers the same expression a second time, bare, purely to produce the type
the subscript narrows to, minting a fresh placeholder nothing ever solves.
Checking against
Anynever pins.expr_with_optionsskips the checkwhen the expectation is
Any, correctly, since it always succeeds — butsucceeding trivially does not solve the placeholder either. This is what left
the same spurious report on attribute assignment, return position, default
arguments, global assignment and TypedDict fields.
The rule
Pin those placeholders instead of reporting them, when nothing can observe them:
no
Anythat pyrefly inferred rather than the user declaring. A containerpinned from
{}propagates the same uncertainty rather than absorbing it, soits contents keep the diagnostic.
expression minted itself. One belonging to a name it merely mentions
outlives the assignment and keeps its own diagnostic.
pinning is global: a solved arm must not silence what an unsolved arm leaks.
The rule asks whether the placeholder can escape, never what the target is, so
dict,list,defaultdict, aMutableMappingprotocol and a hand-written__setitem__are covered by the same code with no per-container branches.This follows the direction @yangdanny97 set out on the issue — "be smarter about
when we emit these errors based on the contextual type" — and the counter-example
given there,
x = {}followed byx["y"] = {"z": None}, is a passing control.Relationship to #4391
@lyydsheep's #4391 (draft) fixes the reported case by matching the target against
builtins.dict[builtins.str, Any], and got there first. This generalises thesame idea. Measured, each patch applied to the same base and run on the same
inputs:
dict[str, Any](the report)dict[int, Any]MutableMapping[str, Any]defaultdict[str, Any]list[Any]__setitem__c.attr = {...},return {...},x: Any = {...}default, TypedDict fieldx = {}thenx["y"] = {...}Happy to fold this into #4391 rather than land it separately if that is easier —
the analysis matters to me, not whose branch it lands on.
Real-world check
CPython 3.12 stdlib plus transitively-imported site-packages (numpy, scipy,
idlelib, importlib, unittest, configparser) — 137,627 diagnostics on main:
implicit-any-empty-container83 false positives removed, and every changed line is
implicit-any-empty-container— no other error kind moved by a single line.3,122 of 3,205 instances survive, so this is not a blanket disabling of the
check. Real examples it removes:
Two deliberate behaviours, both tested
d["k"] = {"t": None}ondict[str, int]keepsCannot set itemand drops thesecond diagnostic, since nothing can observe a placeholder inside a value being
rejected outright. No existing test covered this and [pyrefly] Contextualize dict subscript literals for Any #4391's test asserts both
errors, so flagging it — it is isolated to
target_absorbsand easy to revert.bugtest.bind_unpackingnotes that "we never contextually type unpacks, we do theunpacking at type level for simplicity (for now)", so the target's
Anyneverreaches the value and there is no origin range to key containment on. That
belongs to contextually typing unpacks, not here.
Verification
cargo test -p pyrefly— 7441 passed, 0 failed.code,line,columnormessage.main: names nested insidedisplays, aliased/chained/union targets with an unsolved arm, slice assignment,
late-solved targets, plain assignment. All still report; no new diagnostics.
solve.rshalf brings thereported case back, and the
expr.rshalf alone leaves every subscript probeidentical to
main.pinning in place.
where both sides emit identical diagnostics: 1087 ms before, 1079 ms after. No
measurable overhead.