Expand imports in classes

[A5rocks]

Description

(each on their own line since I don't know what GitHub does for syntax for these :P)
Fixes #11045, external reference huggingface/transformers#13390
Fixes #10488
Fixes #7045
Fixes #7806
Fixes #11641
Fixes #11351
Fixes #10488

(Sadly this does not fix the crash in #8891)

This PR makes from module import function in a class assign that function to the class.

Explicitly out of scope (right now) is import name in a class making ClassThing.name.func_in_name work.

Test Plan

I verified these changes by running the reproduction steps of the issues listed as fixed, and put one of them in the unit tests.

[A5rocks]

Aha, I forgot about imports in methods. Whoops.

[97littleleaf11]

lgtm!

[A5rocks]

Hello! I pushed some changes that should have resolved the comments! (I actually did this 24 days ago, but didn't realize it was probably best to leave a comment...)

[JukkaL]

Thanks for the PR! This fixes a fairly nasty issue, but I think that we either need a somewhat more involved fix, or we can fall back to an even simpler workaround where we just reject this but don't crash.

[JukkaL]

Unfortunately this doesn't seem right/enough. We modify the function as defined in the original module, but it should only be treated as a method in this class, not elsewhere. I wonder what would happen if import the same function into multiple classes. For completeness, we should probably do this for Decorator and OverloadedFuncDef as well.

Implementing this in a full and correct way seems somewhat complicated. We could perhaps take a copy of the original FuncDef (or other node) and only modify the copy.

Alternatively, you could detect this and generate a blocking error, and not add anything to the symbol table. This would be the easiest fix. The error message can contain a note that suggests doing something like name = mod.func instead.

[JukkaL]

Test what happens if we import two functions with the same name (from different modules)?

Also test what happens if we import a function over an existing name.

Test multiple classes that import the same function.

Test calling the original function via module and instance.

[hauntsaninja]

I was running into this issue at work and foolishly missed this excellent PR. I ended up with something very similar to this, here's my patch:

diff --git a/mypy/semanal.py b/mypy/semanal.py
index 22d6b2e57..fdbf6c10d 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -4756,7 +4756,16 @@ class SemanticAnalyzer(NodeVisitor[None],
                             module_hidden: bool) -> None:
         """Add an alias to an existing symbol through import."""
         assert not module_hidden or not module_public
-        symbol = SymbolTableNode(node.kind, node.node,
+
+        node_node = node.node
+        if self.is_class_scope() and isinstance(node_node, FuncBase):
+            # Imports inside class scope do not produce methods.
+            # We construct a Var so as to treat them more like assignments.
+            node_node = Var(node_node.name, node_node.type)
+            assert self.type is not None
+            node_node.info = self.type
+
+        symbol = SymbolTableNode(node.kind, node_node,
                                  module_public=module_public,
                                  module_hidden=module_hidden)
         self.add_symbol_table_node(name, symbol, context)

It differs from this PR in that:

• we don't bind self, which I think is accurate to the runtime
• we don't modify the original definition
• it implicitly handles import forms other than ImportFrom
• it implicitly handles other FuncBases (such as OverloadedFuncDef)

Curious if @A5rocks @JukkaL this looks workable to you?

Edit: I was wrong about self binding. We do bind self if we import a Python function, but I tested with a C import.

[hauntsaninja]

Just went through issues and updated the PR body with things I think are duplicate reports of this crash. It's quite a lot!

[A5rocks]

Looks quite good to me (as long as it works); I haven't worked much on this PR as I ran into the mutable problem and couldn't figure it out.

I can push more of my test cases in a bit.

Edit: I must have lost that stash :S

[A5rocks]

Superseded by #12023