Rename variable to avoid mypy type error

[stephenmcgruer]

Reusing variables can cause a mypy type error (python/mypy#1174).
This previously went unnoticed because the previous and current use of key was str, but in
#23644 the previous use was retyped to Text
and as such caused a unicode-vs-str type error in mypy.

[stephenmcgruer]

I'd like either @Hexcles or @gsnedders to look at this, as their mypy knowledge far exceeds mine.

[stephenmcgruer]

Note that this didn't start complaining until #23644, so it may be that there is some problem here and this is the wrong fix

With #23644 reverted, I get:

cached_properties: Revealed type is 'Any'
key: Revealed type is 'builtins.str'
self.__dict__: Revealed type is 'builtins.dict[builtins.str, Any]'

With that change, key changes to builtins.unicode, so maybe worth digging into.

[stephenmcgruer]

Ok yes, reverting #23644 and doing some bisecting I narrowed this to the change in type of def script_metadata. I've no idea why yet, but reverting the entire change and then applying just that line causes key to flip from str to unicode.

[gsnedders]

With #23644 reverted, I get:

cached_properties: Revealed type is 'Any'
key: Revealed type is 'builtins.str'
self.__dict__: Revealed type is 'builtins.dict[builtins.str, Any]'

What are the revealed types with it, but without the str call?

I'm slightly surprised to see key isn't also Any there, but given it's used to lookup in the Dict[str, Any] I guess it isn't that surprising.

[stephenmcgruer]

What are the revealed types with it, but without the str call?

Not sure if I am misunderstanding, but that PR added the str() call - so those are the types without it.

[gsnedders]

Not sure if I am misunderstanding, but that PR added the str() call - so those are the types without it.

Pretty sure I'm misunderstanding now, given:

Ok yes, reverting #23644 and doing some bisecting I narrowed this to the change in type of def script_metadata. I've no idea why yet, but reverting the entire change and then applying just that line causes key to flip from str to unicode.

So if #23644 caused it to flip to unicode, revealing the types must've changed without the str call to give unicode somewhere, no?

[stephenmcgruer]

Soooo I renamed key to j and:

tools/manifest/sourcefile.py:997: note: Revealed type is 'Any'

I think mypy is getting confused with the earlier declarations of key in the same code?

[stephenmcgruer]

jgraham points me to: python/mypy#1174

[stephenmcgruer]

So options are:

i. Do the type annotation like this PR does - it works today to nudge mypy into realizing the new type of 'key', but not guaranteed to work always I guess?
ii. Change the variable name to something else.
iii. Leave it with the cast.

I'm open to opinions. I probably lean to (ii).

[jgraham]

I think I prefer i to iii in that order. But I'm happy with i or ii and less with iii.

[Hexcles]

Great investigation, folks!

I think the root cause here is indeed the redefinition of key and I prefer renaming the variable (to prop perhaps).

[Hexcles]

Question: was this just a warning instead of an error from mypy? (I'm wondering how #23644 passed.)

[stephenmcgruer]

Question: was this just a warning instead of an error from mypy? (I'm wondering how #23644 passed.)

Because that PR wrapped key in str(key) explicitly.

[Hexcles]

And you noticed when reviewing the code?

…

On Mon., May 18, 2020, 19:06 Stephen McGruer, ***@***.***> wrote: Question: was this just a warning instead of an error from mypy? (I'm wondering how #23644 <#23644> passed.) Because that PR wrapped key in str(key) explicitly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23669 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAK6ZDCG3CXCGMSR4J3H2XLRSG5NXANCNFSM4NEHT3BQ> .

[stephenmcgruer]

And you noticed when reviewing the code?

@foolip did: #23644 (comment)