Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cycle in inheritance hierarchy because items have the same name #4175

Closed
quodlibetor opened this issue Oct 27, 2017 · 15 comments
Closed

Cycle in inheritance hierarchy because items have the same name #4175

quodlibetor opened this issue Oct 27, 2017 · 15 comments
Labels
bug mypy got something wrong

Comments

@quodlibetor
Copy link
Contributor

quodlibetor commented Oct 27, 2017

mypy does not distinguish between different objects with the same name in a file.

Given:

class same(object):
    pass

class same(same):
    pass
$ mypy /tmp/samesame/supername.py
/tmp/samesame/supername.py:6: error: Name 'same' already defined on line 1
/tmp/samesame/supername.py:6: error: Cycle in inheritance hierarchy

The first error is a legitimate style nit, but the second item is not technically correct. Worse, it appears that it cannot be silenced by --follow-imports=silent:

from dateutil.zoneinfo import tzfile  # type: ignore
$ MYPYPATH=$VIRTUAL_ENV/lib/python3.6/site-packages mypy --follow-imports=silent /tmp/samesame/other.py
.../lib/python3.6/site-packages/dateutil/zoneinfo/__init__.py:25: error: Cycle in inheritance hierarchy

To be clear, I don't think that this is an example of good code, but as you can tell it came up for me in dateutil, and, AFAICT it's basically impossible to workaround.

If there was a way to have a hard blacklist on modules to treat as Any without importing that would also solve this for me in the short time.

@gvanrossum
Copy link
Member

This is mostly a dupe of #1174, except the issue about Cycle in inheritance hierarchy is different. It also cannot be suppressed with # type: ignore. I presume it's insuppressible (and blocking) because mypy needs to compute the MRO before it starts type checking properly.

If you have control over the source code there are various possible workarounds, e.g. renaming the first class.

However, your original sin is adding site-packages to MYPYPATH. There's an explicit recommendation against that in the source code, at the very end of http://mypy.readthedocs.io/en/latest/basics.html.

@quodlibetor
Copy link
Contributor Author

I presume it's insuppressible (and blocking) because mypy needs to compute the MRO before it starts type checking properly.

Yeah this is what I assume as well. I had hoped that using --follow-imports=skip would cause mypy to do a best-effort analysis and fall back to Any in horrible cases like this.

If you have control over the source code there are various possible workarounds, e.g. renaming the first class.

I've patched dateutil, the next release should be fine (and my internal registry has the patch too, so I'm fine).

However, your original sin is adding site-packages to MYPYPATH.

Right, this came around because the recent PEP-561 pointed out that other people were ignoring this advice and adding site-packages, and I decided to experiment.

From what I can tell, there are very strong advantages to adding site-packages to MYPYPATH, if you can get it to work. The biggest one is that we have a large (internal) ecosystem of packages that all specify type information inline and build with --deny-untyped-defs. With MYPYPATH including site-packages, --ignore-missing-imports and --follow-imports=skip we get type-checking for all of our internal packages and upstream problems mostly don't affect us. AFAICT there is no real other option for this use case? Or is it expected that users have a separate directory with only downloads of known-compatible libraries? That's a lot more work than the fallback-to-Any behavior that I expected, and it would provide strictly less information to mypy.

@gvanrossum
Copy link
Member

The problem is indeed getting MYPYPATH=site-packages to work, which is not easy in the general case -- the problem you encountered with dateutil is just the tip of the iceberg. I do have sympathy for your use case, but no real solution yet, and a ton of other priorities. Would PEP 561 help you at all?

@quodlibetor
Copy link
Contributor Author

Honestly I'm not sure. From what I can tell the tools that PEP-561 introduces are:

  • a Typed: None|inline|stubs key in METADATA
  • a suggested change to the import path for type checkers

The work that I expect to do going forward is to make it possible for my organization to trivially pip download --no-deps $VIRTUALENV/$TYPEDIR everything in our package namespace. Having a Typed key will make it, I suppose, easier to do this for things that aren't in our namespace.

The PEP doesn't explain what the expected workflow will be once it has been implemented. Will it be suggested that distribution tools grow an option to automatically copy packages that have a typed key into some $TYPEDIR? The PEP kind of suggests that type-checking tools should refuse to check packages that don't opt-in via the Typed key.

Because of the fact that the PEP focuses on the stubs use case it's not super clear to me that inline in the PEP means what I assume it means, which is that code uses type comments or py3 type annotation syntax. It's also not clear to me if inline packages should be installed into the $TYPEDIR, or if it's meant to make setting MYPYPATH=site-packages safer to use by making type-checkers ignore packages that haven't opted in.

tl;dr: my issue is that I'm not sure what the best value for MYPYPATH is or what the best way to populate the target directories is, and it's not clear to me how PEP 561 will make that more obvious or reduce the total amount of work I will need to do to set it correctly.

@gvanrossum
Copy link
Member

Yes, the PEP uses inline to mean what you think.

I would assume that the main mechanism for getting the right collection of packages should be a requirements.txt file or equivalent.

I'm pretty sure the PEP wants to avoid setting MYPY=site-packages.

@emmatyping
Copy link
Collaborator

@quodlibetor Thank you for the feedback, I will be clearer and define terms in the next version of my PEP. The goal of the PEP is to use existing packaging tools and install locations to distribute typed packages. The workflow for users should be to indicate a package is typed (eg through a setup() keyword) and install the package. The type checker should be able to pick it up after that.

@quodlibetor
Copy link
Contributor Author

@ethanhs glad to be helpful!

Just to be clear, then, is the PEP meant to make it so that pip install --types requirements.types.txt does the right thing, or to do the opposite and make it so that MYPYPATH=$site-packages works, and that static checkers ignore things with no Typed key?

(this seems like it might no longer be the correct conversation for this venue, I could switch conversation to the python-dev thread after the next revision?)

@emmatyping
Copy link
Collaborator

The pip install command will handle it automatically, no need for a types argument since for inline types there is no difference between runtime code to install and types to install. The idea is that you install a package with the type information, and the type checker picks it up automatically.

Perhaps discussion can continue here python/typing#84

@bugreport1234
Copy link

I just ran into this issue myself. When a class has the same name as its superclass, mypy mistakenly thinks there's a cycle in the inheritance hierarchy. I'd like to mention that the issue persists independently of library imports and MYPYPATH configuration. The original example given above with just 5 lines is enough to trigger the bug.

I hope it's possible to resolve this soon, because right now it seems mypy completely aborts any typechecking of modules that import the affected class. When I run mypy with --html-report the table entries for the importing modules are completely missing, and their parent packages are listed as "0 LOC". It's quite confusing because it's not clear whether mypy has problems detecting the files, or has aborted typechecks because of the cyclic dependencies.

In my codebase the affected class is a pretty low-level class that almost all modules in the package depend on in some way, and as a result the amount of LOC reported as analyzed is less than 1% of the total.

This also happens with code like this:

from module import Class

class Class(Class):
    def new_method(self):
        ...

which I believe is a more common scenario than defining two classes with the same name in the same module. I've used it on multiple occasions to quickly provide a drop-in replacement for a class augmented with some methods.

While in some cases it's possible to work around this by renaming the superclass, it's not always possible (e.g. if the problem exists in a library file) and it's not technically an inheritance cycle, so it seems like a bug.

@gvanrossum
Copy link
Member

You could solve it easily with

from module import Class as _Class
class Class(_Class):
    ...

The root cause is that mypy only understands a single definition of a name in any given namespace. Your example violates that; my work-around solves it. I don't think there's a quick fix in mypy possible. Also, the idiom seems questionable.

@bugreport1234
Copy link

Yeah, for now I've changed my classes to use the underscore import workaround.

(It would still be great if mypy could detect this kind of situation and output something like "Redefinition of class not supported", and possibly continue parsing affected modules, ignoring the unsupported class).

@gvanrossum
Copy link
Member

gvanrossum commented Dec 30, 2017 via email

@ilevkivskyi
Copy link
Member

Agreed that the error should be better.

I think it is also possible to make it non-blocking. (For example by setting fallback_to_any or similar.)

@gvanrossum
Copy link
Member

Yup. (In fact I think many blocking errors can be demoted to "normal".)

kernc added a commit to pdoc3/pdoc that referenced this issue Jul 8, 2019
@AlexWaygood
Copy link
Member

The Cycle in inheritance hierarchy error is not reported on mypy 0.720+.

@AlexWaygood AlexWaygood added the bug mypy got something wrong label Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

6 participants