assert failing in lookup_qualified_node with -i/--incremental

[gvanrossum]

I've got some proprietary code where mypy reliably crashes on the second -i run, as follows:

(v36) bash-3.2$ ci/mypy3_all.sh -i
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/runpy.py", line 170, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/local/lib/python3.5/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/__main__.py", line 5, in <module>
    main(None)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/main.py", line 46, in main
    res = type_check_only(sources, bin_dir, options)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/main.py", line 93, in type_check_only
    options=options)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/build.py", line 188, in build
    graph = dispatch(sources, manager)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/build.py", line 1570, in dispatch
    process_graph(graph, manager)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/build.py", line 1806, in process_graph
    process_fresh_scc(graph, prev_scc)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/build.py", line 1875, in process_fresh_scc
    graph[id].fix_cross_refs()
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/build.py", line 1341, in fix_cross_refs
    self.manager.options.quick_and_dirty)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/fixup.py", line 24, in fixup_module_pass_one
    node_fixer.visit_symbol_table(tree.names)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/fixup.py", line 92, in visit_symbol_table
    self.visit_type_info(value.node)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/fixup.py", line 58, in visit_type_info
    self.visit_symbol_table(info.names)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/fixup.py", line 83, in visit_symbol_table
    self.quick_and_dirty)
  File "/Users/guido/src/client/.mypy/venv/lib/python3.5/site-packages/mypy/fixup.py", line 276, in lookup_qualified_stnode
    assert isinstance(node, TypeInfo)
AssertionError

[gvanrossum]

And guess what the value of cross_ref is when the lookup fails? _importlib_modulespec.Loader.module_repr.

@pkch or @ilevkivskyi -- any idea?

[gvanrossum]

(And the source file is typeshed/stdlib/3/importlib/abc.pyi.)

[pkch]

python 2 or 3?

[gvanrossum]

Python 3.

[gvanrossum]

I haven't pinpointed it yet but I fear it's another import cycle, probably involving _importlib_modulespec and importlib.abc.

[gvanrossum]

(Sorry for the many messages.)

This is in fixup_module_pass_one() for importlib/abc.pyi. This module is fake-defining Loader, which it actually imports from _importlib_modulespec:

# Loader is exported from this module, but for circular import reasons
# exists in its own stub file (with ModuleSpec and ModuleType).
from _importlib_modulespec import Loader as Loader  # Exported

However (presumably due to the magic renaming going on here) the cache file importlib/abc.data.json contains the full definition for the Loader class, while the cache file for _importlib_modulespec contains a dummy Loader that has cross_ref set to importlib.abc.Loader. (So in the cache it appears the actual definition is in importlib.abc while in the typeshed source the actual definition is in _importlib_modulespec -- and in each case the other module just contains a reference.)

I think there's another rename or anti-rename missing, but I haven't figured it out yet.

[pkch]

Maybe my entire solution in #3203 and #3235 should be replaced with something more fundamental. I'm thinking maybe we should not create a symbol table for _importlib_modulespec at all, and when we parse it, we just add those 3 names to the corresponding symbol tables for types, importlib.abc and importlib.machinery?

Alternatively, we can make mypy handle arbitrarily re-exported of names between modules, although I doubt it's worth it.

[ilevkivskyi]

Maybe the problem is simpler, there is code:

    if '_importlib_modulespec' in modules:
        # we are using python 3, so renaming is necessary
        name = rev_module_rename_map.get(name, name)

But IIUC the name is actually '_importlib_modulespec.Loader.module_repr', so it is simply not renamed? What happens if renaming a sub-string with str.replace or similar instead?

[pkch]

Yes, that name would not be renamed. But if that name appears, it's a problem with the original renaming. By the time we're in this code, we should not be seeing _importlib_modulespec at all.

So maybe my original renaming isn't comprehensive enough. I rename _importlib_modulespec.Loader --> importlib.abc.Loader in the first pass of sem. analyzer in visit_file. Perhaps it's not enough.

[pkch]

I reproduced the error, looking into it now.

[pkch]

But IIUC the name is actually '_importlib_modulespec.Loader.module_repr', so it is simply not renamed? What happens if renaming a sub-string with str.replace or similar instead?

Actually the renaming inside fixup.lookup_qualified_stnode (in which the assert is raised) is happening in the opposite direction: importlib.abc.Loader -> _importlib_modulespec.Loader. The name is already _importlib_modulespec.Loader.module_repr, so as far as rev_module_rename_map intent is concerned, there's nothing to be done.

My original idea in #3203 was that these names will continue to live inside _importlib_modulespec symbol table, and any attempts to access them through public names (importlib.abc.Loader, etc.) would be redirected to that table. I'm not sure if it's done correctly as is, or if it does require a substring search in some cases. But in this particular crash scenario, #3203 worked as intended in the sense that it did find the symbol that corresponds to importlib.abc.Loader.

The crash happened because the symbol that was found is defective; it's node value is None. It turns out that there are actually two different symbols that correspond to importlib.abc.Loader: one (defective) inside _importlib_modulespec table and one (good) inside importlib.abc.Loader table. All the instance methods, etc. are added only to the second symbol.

Before trying to fix it, I wanted to see why I redirected importlib.abc.Loader etc. to _importlib_modulespec in the first place. Well, the crash that me and @ilevkivskyi saw on dde184f happened because importlib.abc.Loader wasn't available yet when ModuleType symbol was requested. However, when I reverted my commit from #3203 on the current master, the crash does not occur. Perhaps some changes in typeshed or elsewhere changed the order in which names appear.

In short, I think my #3203 + #3235 are not good, since they create two separate symbols that correspond to the same item. Let's figure out if the original crash I was fixing is still a risk, and if so, what's a proper way to fix it is.

[gvanrossum]

This is a bit disturbing. There don't seem to be any typeshed log entries for importlib, so I'm not sure what to think of that theory. But I do think that maybe #3202 did not require urgent fixing, and the current crashes do, so maybe rolling back both #3203 and #3235 is best. (And please do at least verify that you no longer have this crash -- I'm not sure if I can repro the previous one.)

[pkch]

The repro for both crashes is here:

#!/bin/bash
mkdir -p crashmypy
cd crashmypy

# crash #1
# it was present on commit dde184f
# see https://github.com/python/mypy/issues/3202
# fixed in https://github.com/python/mypy/pull/3203
# fix updated in https://github.com/python/mypy/pull/3235
# on (latest) commit 665a810, the original crash no longer occurs even
# if the two fixes are rolled back
cat > b.py <<EOF
from a import A

def f(x: A) -> None:
    x.g()
EOF
cat > a.py <<EOF
class A:
    def g(self) -> None: pass
EOF
rm -rf .mypy_cache
mypy -i b.py
touch a.py
mypy -i b.py
echo "End of crash 1 test"

# crash #2
# it is present on both (latest) commit 665a810 and (old) commit dde184f
# if #3203 and #3235 are rolled back, this crash does not occur
rm -rf .mypy_cache
cat > b.py <<EOF
from importlib.abc import Loader
Loader().load_module('xyz')
EOF
rm -rf .mypy_cache
mypy -i b.py
touch b.py
mypy -i b.py
echo "End of crash 2 test"

I am going to roll back #3203 and #3235. I suspect the problem they were trying to solve was due to the fact that _importlib_modulespec.pyi contained symbols such as Loader before Loader is defined. (Both Loader and ModuleType refer to each otehr, so there's going to be a cylce). I can't really say for sure whether that problem disappeared for good or just temporarily. Sem. analyzer stores those names in a dictionary, but I'm on 3.6 where dictionary order is deterministic. So I shouldn't see intermittent crashes unless (non-deterministic) set is used somewhere earlier.

The important thing is that for now, at least, the crashes do not happen with the two PRs rolled back. If anyone experiences them again, we can come back to this discussion.

[pkch]

Well, to be more precise... Reverting the #3203, #3235 and some of the changes in my older PR #3107 removes both crashes. The only feature we lose is that reveal_type says ''_importlib_modulespec.ModuleType', etc. instead of 'types.ModuleType'. Since we didn't have any tests that rely on that particular output, no tests break.

Let's deal with the reveal_type() issue later.

[ilevkivskyi]

@pkch
OK, I applied your revert, it fixes both crashes (see my comment on PR). Concerning reveal_type and error messages, the right way to do this is to add a renaming rule to either messages.format_simple or messages.reveal_type or types.TypeStrVisitor.visit_instance. These will not affect any mypy symbol table logic, only pretty-printing of types.

[pkch]

Should we add the repro from my comment above as a new regression test? Or it's pretty safe with the offending code removed?

[gvanrossum]

Yes, please add such a test. Maybe it can go in check-incremental.test? (Or is the need to run with -i twice a problem?)

[pkch]

Hmm so far I've been able to repro it only with a command-line test which unfortunately is too slow (10 sec). I'll try again later.

[gvanrossum]

Oh, this may be because the check*test files use fake stubs.

There are some other tests that use real stubs.

But 10 seconds? That seems excessive.