Add typing for set_local

[cdce8p]

Ref #1834 (review)
/CC: @DanielNoord

[coveralls]

Pull Request Test Coverage Report for Build 3401620989

• 24 of 24 (100.0%) changed or added relevant lines in 8 files are covered.
• 5 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.004%) to 92.275%

---

Files with Coverage Reduction	New Missed Lines	%
astroid/nodes/scoped_nodes/scoped_nodes.py	5	93.0%

Totals
Change from base Build 3367353146:	0.004%
Covered Lines:	9867
Relevant Lines:	10693

---

💛 - Coveralls

[DanielNoord]

As I said before, locals is not always only NodeNG. See for example:
https://github.com/PyCQA/astroid/blob/dabffad7e84c93f9d6dd8f0eb67e63a30e5e098b/astroid/brain/brain_io.py#L15-L21

We might need to fix that brain or just accept SuccesfulInferenceResult here. I believe there are also other places where this is the case as I looked into typing this previously and was held back by the inconsistencies.

[cdce8p]

Thanks for the followup @DanielNoord! I should have checked it more carefully.
However, it's not as simple as it might seem. instance in the example you referenced is actually a subtype of NodeNG.
https://github.com/PyCQA/astroid/blob/aae90e6480c3fb9da95447587e639af6ad297689/astroid/nodes/node_ng.py#L613-L623

The only example I found now is with nodes.Dict.items which creeps in the SuccessfulInferenceResult typing.
https://github.com/PyCQA/astroid/blob/aae90e6480c3fb9da95447587e639af6ad297689/astroid/nodes/scoped_nodes/scoped_nodes.py#L2282-L2285

I'll update the PR.

[cdce8p]

Update
Seems like instantiate_class can actually return an Instance. It's just not annotated in the base class.
https://github.com/PyCQA/astroid/blob/aae90e6480c3fb9da95447587e639af6ad297689/astroid/nodes/scoped_nodes/scoped_nodes.py#L2516-L2527

[DanielNoord]

Ah yeah, I knew there was something with Dict.items but couldn't find it. Thanks for taking another look.

Like I said, I'm not even sure if it is correct or we should actually reject types like Instance.

I do think that updating this will cause multiple trickle down issues in other places as I think many return values depend on locals. However, it also doesn't seem weird that in many cases where we expect a node it can actually be a SuccessfulInferenceResult.

[cdce8p]

Like I said, I'm not even sure if it is correct or we should actually reject types like Instance.

I do think that updating this will cause multiple trickle down issues in other places as I think many return values depend on locals. However, it also doesn't seem weird that in many cases where we expect a node it can actually be a SuccessfulInferenceResult.

Since it so widely used in brain modules, I suspect any code dealing with it needs to handle Instance already. We'll see it over time, probably.

Just a note, I've only updated locals itself. The typing for the set_local methods can stay the same as far as I can see.

[DanielNoord]

One final question!

[DanielNoord]

Do we want to update all other places where we define local?

[cdce8p]

Pushed an update. AFAICT there wasn't that much to deal with. Added type: ignore in a few places where node.locals would only return node.NodeNG. Those could theoretical be cast as well. Let me know what you think.