[import] Check type suggestions against import aliases

[ezragoss]

If a tool like dmypy suggest recommends,

{
"func_name": "nop",
"path": "mod1.py",
"line": 1,
"signature": {
    "arg_types": ["mod1.MyClass"],
    "return_type": "mod2.AnotherClass"},
}

for the code

import mod2 as bar
def nop(foo):
    return bar.AnotherClass()
class MyClass: pass

We used to incorrectly annotate

import mod2 as bar
def nop(foo):
    # type: (MyClass) -> mod2.AnotherClass
    return bar.AnotherClass()
class MyClass: pass

But we now correctly annotate

import mod2 as bar
def nop(foo):
    # type: (MyClass) -> bar.AnotherClass
    return bar.AnotherClass()
class MyClass: pass

[pmolodo]

So, came to a few conclusions:

• dmypy always returns "full package.module(s).type" for it's arg_types
• these are the values that we are looking up in our find_import* funcs
• so we are always looking for the "full/true" name, and NEVER the aliased name

This led to two conclusions:

• we only care about the alias when returning info - never when deciding if a given import matches! So we need to stop comparing against the alias
• since we're always feed in a full path, we need to make sure always match the "full path" - functionally, that means we need to make sure we're always comparing against the package
  • however, we're missing a few spots:
    • fixer_utils.py:_get_import_binding_info:handle_as_name:271 - need "and not package"
    • fixer_utils.py:_has_import_name:219 - need "and not package"
    • fixer_utils.py:_get_import_binding_info:304,307 - package should be None
    • fixer_utils.py:_has_import_name:231 - think we need an "else" branch (ie, if not package) - because "name" can actually be a package!

But... seeing this made me realize that what name is, and what package is, can be a bit confusing. I think we can make things simpler / clearer if we switch our "checking" paradigm.

Currently, our paradigm is:

• Have checker functions
  • we feed them what we're looking for, and the parsed tree node
  • they return the import-info only if it matches, and None otherwise

I think it will simplify things if:

• We have two basic top level functions:
  • an import-info-grabber, which takes a tree node, and output input-info (unconditionally)
  • we have a checker, which takes input-info, and what we're looking for, and returns a boolean

This way, we're not having to worry about what we're looking for while we're doing the parsing, which simplifies that part... and when we're checking, we're checking against a "standardized format", which should simplify the checking portion too.

There's one other advantage to this approach - at some point, we'd like to switch to an approach where we go through and find all import statements, get their import info, and then cache out a big dictionary, indexed by the things we're looking for, and mapping to the available bindings. This will avoid a lot of unnecessary repeated "parsing" of import statements!

[pmolodo]

Oh, and the other thing I realized - right now, in type_by_import_stmt, we search for (in order):

name_binding = find_import_info(name, root, package)
 mod_binding = find_import_info(package, root, None)

The underlying reason is that, if we're looking for pkg.mod1.MyClass, we want to prefer imports that bring MyClass directly into the top-level namespace, over imports of just pkg.mod1. However, since in our case we're always looking for classes/types - or, at the very least, things that are highly unlikely to be modules - this basically just means that we always prefer from X import Y statements!

This is because, though this statement would make MyClass available with a top-level binding:

import pkg.mod1.MyClass as theclass

...it's invalid unless MyClass is a module, which it shouldn't be! So, the only way to bring in a type/class is with a from statement.

So, we just prefer from statements.

If we make our ImportInfo something like:

import pkg.mod1 as foobar    => ImportInfo(pkg='pkg.mod1', entry=None, binding='foobar')
import pkg.mod1  => ImportInfo(pkg='pkg.mod1', entry=None, binding='pkg.mod1')
from pkg.mod1 import MyClass as thing => ImportInfo(pkg='pkg.mod1', entry='MyClass', binding='thing')

...then we're just looking for ImportInfo(pkg='pkg.mod1', entry='MyClass', ...) first, followed by ImportInfo(pkg='pkg.mod1', entry=None, ...)

[pmolodo]

First - sorry for any noise I generated by addding :rocket: emoji - was using that as a way to mark comments that were addressed, until I realized that they can be hidden...

Anyway - I think nearly everything has been addressed, and all your latest changes looked great! There's only remaining thing I'd still like added - the 2nd test in this comment. It's just a new test, and I think it should work... but it will be a good test, both now and for any future regressions.

Assuming that passes, though, I think this PR is ready - it doesn't look like I have permissions to authorize the merge myself, so we may have to message Chad for that...