pyrefly icon indicating copy to clipboard operation
pyrefly copied to clipboard

Update is not handled well with empty dict type inference

Open QEDady opened this issue 8 months ago • 7 comments

Describe the Bug

d = {}
d.update({1:1}, a=2)
reveal_type(d) 

shows

dict[int, int]

But, it should be

dict[int | str, int]

Sandbox Link

https://pyrefly.org/sandbox/?code=MQAg6gpgNgxg9gWwiALnVALZAFAngJwgDMpcQBlAQwDsATAIzgA8AaTASwGcQuRKQADpQDmEAFCgA7lkIhccAK4gYNEJPzsUENpwyVZNWiCgR91PowUpUuAe2rCe5vCgxxqAOgkSQACQgGBsqKAiYgRKYoCoTccEQc3JyGjEwAXD4AtCAA4hDWhJRQGSjsSDYCyDBYMADW9o4REAyUtZkgAKqcyIQAbqZQAPoothAAFACUqOj2nBUw1vYR+IRGwxWcbRBMFRpI1NaSmhiYyACSACIAouGR0RDcrpQHyJwKAgJw+NajEB7CHnwrHB4AhQnltCBhHBitDaMR7Jp2O42G4+vg2HkYOM2mANFo+HQQPgFM5cK53ME4SBaOxCPNSE45Ip8CB6Pg4JIuvgxGIiOyEOV6jxQZ9rAAqHlGAC8IAA3gBfMS0DxvWhPMaygCMqQATPK2JQpTrsb1+kMRqNaOMgA

(Only applicable for extension issues) IDE Information

No response

QEDady avatar May 18 '25 02:05 QEDady

Thanks for flagging this! TIL you can add string keys via keyword args to dict.update.

yangdanny97 avatar May 18 '25 02:05 yangdanny97

I think this is intentional. When you write {} without any type signature, we finalise the type on the first usage, which is the first argument to update. You can write d: dict[int | str, int] = {} to force it to get the right answer.

That said, we do want to adjust our function calls so they take into account all of the pieces of the signature, which I think would make this work. CC @grievejia @stroxler as we were discussing it.

ndmitchell avatar May 18 '25 19:05 ndmitchell

I see but even the following

d = {}
d.update(a=2)
reveal_type(d) 

will give dict[Unknown, int]. Is this intentional as well?

QEDady avatar May 18 '25 19:05 QEDady

I think that's the behavior is just the what the stdlib stubs for update would typecheck as if it was called like a normal function. As shown by your examples, it has semantics that aren't modeled well by the annotation.

To get the behavior you expect, we probably need to special-case this in the typechecker

yangdanny97 avatar May 18 '25 22:05 yangdanny97

Indeed, the type of update is:

class MutableMapping(Mapping[_KT, _VT]):
    def update(self, **kwargs: _VT) -> None: ...

So nothing about this type signature demands that _KT must be str. That means you can write:

x: dict[int, int] = {}
x.update(x = 1)

And you get no error in MyPy, Pyre1, Pyrefly or Pyright. If I change the type of update to:

    def update(self: Mapping[str, _VT], **kwargs: _VT) -> None: ...

Then it works. I'll see about whether we can get that up to typeshed.

ndmitchell avatar May 19 '25 14:05 ndmitchell

https://github.com/python/typeshed/pull/14099 for the typeshed PR.

ndmitchell avatar May 19 '25 15:05 ndmitchell

a2559c858086214196c5b39c16b6ace312cc14be updates typeshed, which means that we are getting closer here, with better errors. But I don't think we're yet correct in all cases.

ndmitchell avatar May 19 '25 21:05 ndmitchell

I think at this point the issue may be solved.

Looking at the sandbox, we now accept the simple cases

d1 = {}
d1.update(a=2)
reveal_type(d1) 

d2 = {}
d2.update({1:1})
reveal_type(d2) 

and correctly infer dict[str, int] and dict[int, int]

We do not accept

d = {}
d.update({1:2}, a=2)
reveal_type(d)

but this seems correct - the runtime implementation of update allows this, but the stubs do not because the only overload that mixes **kwargs with a mapping requires the mapping to have str keys. Notably pyright agrees with us here.

stroxler avatar Aug 28 '25 16:08 stroxler