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

Quoted annotations do not work in cyclic imports #70

Closed
jolaf opened this issue Aug 7, 2019 · 37 comments
Closed

Quoted annotations do not work in cyclic imports #70

jolaf opened this issue Aug 7, 2019 · 37 comments

Comments

@jolaf
Copy link
Contributor

jolaf commented Aug 7, 2019

Python usually doesn't allow cyclic imports, but they're sometimes necessary for proper static type annotations. When that is the case, the cyclic import is guarded by if TYPE_CHECKING: and annotations for types imported from such a module are quoted like strings to avoid errors at runtime. That's a standard practice recommended by mypy developers.

However, any program that uses this approach to annotations seems incompatible with typeguard.
Consider this example:

A.py:

from typeguard import TypeChecker

from B import B

class A:
    def f(self, b: B) -> None:
        b.f(self)

with TypeChecker(('__main__', 'A', 'B')):
    A().f(B())

B.py:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from A import A

class B:
    def f(self, a: 'A') -> None:
        print("OK")

If typeguard is disabled, the program performs fairly well as it is in fact absolutely correct in runtime, and also correct from mypy point of view:

$ mypy A.py B.py
$ python3 A.py 
OK

However if it is run with typeguard, the following fatal exception occurs:

$ python3 A.py 
/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py:657: UserWarning: the system profiling hook has changed unexpectedly
  warn('the system profiling hook has changed unexpectedly')
Traceback (most recent call last):
  File "A.py", line 10, in <module>
    A().f(B())
  File "A.py", line 7, in f
    b.f(self)
  File "/home/vmz/git/userfe/B.py", line 7, in f
    def f(self, a: 'A') -> None:
  File "/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py", line 690, in __call__
    memo = self._call_memos[frame] = _CallMemo(func, frame)
  File "/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py", line 55, in __init__
    hints = get_type_hints(func)
  File "/usr/lib/python3.6/typing.py", line 1543, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "/usr/lib/python3.6/typing.py", line 350, in _eval_type
    return t._eval_type(globalns, localns)
  File "/usr/lib/python3.6/typing.py", line 245, in _eval_type
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'A' is not defined
@jolaf
Copy link
Contributor Author

jolaf commented Aug 7, 2019

The reasons for this happening are pretty evident, and the following approaches to resolving the issue seem reasonable:

  • When annotated type is not known, but is represented by a string, do not raise exception, but raise a warning and otherwise treat this particular annotation as Any.

  • Keep separate track of names of things not imported because of TYPE_CHECKING is False, and check if annotation string matches any of those. If it matches, treat the annotation as Any, otherwise raise NameError as before.

  • If NameError occurs at this situation, catch it and then try to match the annotation string contents with actual type of the value passed. If it matches, internally replace the annotation with the type of the value, otherwise re-raise the error.

@agronholm
Copy link
Owner

I have explained in the README how to avoid this situation. Import the module, not the object inside it. Then use an annotation like A.A. Is this not acceptable?

@agronholm
Copy link
Owner

Keep separate track of names of things not imported because of TYPE_CHECKING is False

How do you propose that I accomplish this without creating a typed syntax tree of each module?

@jolaf
Copy link
Contributor Author

jolaf commented Aug 7, 2019

Keep separate track of names of things not imported because of TYPE_CHECKING is False

How do you propose that I accomplish this without creating a typed syntax tree of each module?

Well, probably this particular approach was not a very good idea. :)

@jolaf
Copy link
Contributor Author

jolaf commented Aug 7, 2019

I have explained in the README how to avoid this situation. Import the module, not the object inside it. Then use an annotation like A.A. Is this not acceptable?

I tried that, it doesn't change the situation effectively.

@agronholm
Copy link
Owner

Why not? It lets you avoid the errors from the circular imports, doesn't it? You'd have to forego using typing.TYPE_CHECKING because typeguard is a run time type checker so it really needs the type imported.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

Unfortunately it doesn't help to avoid errors from circular imports.

@agronholm
Copy link
Owner

I am stumped. Can you provide a minimum runnable example with two modules that have an unfixable circular import issue?

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

I consider the one in the issue a good example of that.

Here's the version without using from, it has exactly the same problem:

A.py:

from typeguard import TypeChecker

import B

class A:
    def f(self, b: B.B) -> None:
        b.f(self)

with TypeChecker(('__main__', 'A', 'B')):
    A().f(B.B())

B.py:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    import A

class B:
    def f(self, a: 'A.A') -> None:
        print("OK")

Produces this:

$ python3 A.py
/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py:657: UserWarning: the system profiling hook has changed unexpectedly
  warn('the system profiling hook has changed unexpectedly')
Traceback (most recent call last):
  File "A.py", line 10, in <module>
    A().f(B.B())
  File "A.py", line 7, in f
    b.f(self)
  File "/home/vmz/git/userfe/B.py", line 7, in f
    def f(self, a: 'A.A') -> None:
  File "/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py", line 690, in __call__
    memo = self._call_memos[frame] = _CallMemo(func, frame)
  File "/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py", line 55, in __init__
    hints = get_type_hints(func)
  File "/usr/lib/python3.6/typing.py", line 1543, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "/usr/lib/python3.6/typing.py", line 350, in _eval_type
    return t._eval_type(globalns, localns)
  File "/usr/lib/python3.6/typing.py", line 245, in _eval_type
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'A' is not defined

It starts working if I remove 'A.A' annotation or if I disable typeguard.

Maybe I miss something.
If you know a way to make it work without removing any annotations or turning them to Any - I'd be grateful.
Thanks!

@agronholm
Copy link
Owner

The problem is that you still have if TYPE_CHECKING: in the code. Remove it as I suggested and try again.

@agronholm
Copy link
Owner

And replace the the annotation in the A module with the string equivalent.

@agronholm
Copy link
Owner

If you're running Python 3.7 you can also add from __future__ import annotations. Then you don't need to convert the annotations to strings. On Python 3.8 you don't even need the future import.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

For now I can only use 3.6 unfortunately.

Here it goes:

A.py:

from typeguard import TypeChecker

import B

class A:
    def f(self, b: 'B.B') -> None:
        b.f(self)

with TypeChecker(('__main__', 'A', 'B')):
    A().f(B.B())

B.py:

import A

class B:
    def f(self, a: 'A.A') -> None:
        print("OK")

Produces this even with typeguard disabled:

$ python3 A.py
Traceback (most recent call last):
  File "A.py", line 3, in <module>
    import B
  File "/home/vmz/git/userfe/B.py", line 1, in <module>
    import A
  File "/home/vmz/git/userfe/A.py", line 10, in <module>
    A().f(B.B())
AttributeError: module 'B' has no attribute 'B'

@agronholm
Copy link
Owner

The problem now is the A module. Guard the entry point with if __name__ == '__main__': and it should work.

@agronholm
Copy link
Owner

Actually I'm not sure about that. Moving the entry point to a separate module would be your best bet. The reason being that now there are two versions of the A class: one in the __main__ module and one in the A module.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

Yep, it is:

$ python3 A.py
/usr/local/lib/python3.6/dist-packages/typeguard/__init__.py:694: TypeWarning: [MainThread] call to B.B.f() from A.py:7: type of argument "a" must be A.A; got __main__.A instead
  warn(TypeWarning(memo, event, frame, exc))
OK

@agronholm
Copy link
Owner

agronholm commented Aug 8, 2019

This worked:

a.py:

import b

class A:
    def f(self, x: 'b.B') -> None:
        x.f(self)

b.py:

import a


class B:
    def f(self, x: 'a.A') -> None:
        print("OK")

c.py:

import b
from a import A

from typeguard import TypeChecker

with TypeChecker(('__main__', 'a', 'b')):
    A().f(b.B())

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

Well, I see your point.

Unfortunately, I have a rather large code base authored by many people, and there's a lot of situations like this there.

Of course I can invest in refactoring all the code for it to be usable with typeguard, I'm now considering it, but it seems like a disadvantage of the tool.

The Python itself handles the problem perfectly well - the problem of circular dependencies exists only when defining new things, otherwise you can use any object in any module without importing it's class, passing the reference is enough.

When we started annotating our code to use mypy we've struck this circular dependency problem but happily mypy provided a workaround of string annotations and if TYPE_CHECKING: that helped us to annotate the code successfully without changing the code itself essentially.

But now it seems that to use typeguard (that is otherwise a wonderful and mega-useful tool and works perfectly with mypy), we have to refactor a lot of essential code, like replacing hundreds of from Module import Class imports with plain import Module imports, and crowd the code with thousands of Module.Class constructions.

It seems to me that for a code analysis tool requiring such heavy modifications to the code being analyzed is not very desirable.

I understand that it may be not easy to fix, so all I ask is not disregard this issue as seemingly non-important.

Thank you!

@agronholm
Copy link
Owner

Unfortunately I'm not sure that any of the proposed solutions are good enough for me to integrate to typeguard, at least by default. Let's review your proposals one by one:

  • When annotated type is not known, but is represented by a string, do not raise exception, but raise a warning and otherwise treat this particular annotation as Any. <- doable and not incorrect, but you'd get a lot of warnings this way given the code base you described
  • Keep separate track of names of things not imported because of TYPE_CHECKING is False, and check if annotation string matches any of those. If it matches, treat the annotation as Any, otherwise raise NameError as before. <- covered already in a previous comment; too burdensome and error prone
  • If NameError occurs at this situation, catch it and then try to match the annotation string contents with actual type of the value passed. If it matches, internally replace the annotation with the type of the value, otherwise re-raise the error. <- what if you have two classes by the same name but from different modules? I deal with this kind of thing fairly often in my work. There would be no way to tell which one is the "correct" one.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

Lot of warnings is not a problem as they're easily filtered by filterwarnings().

For the way No.3 - so, typeguard would miss errors when the value passed has the wrong type but with the same name as the right one. That's sad, and not perfect, but the code would run and a lot of other important bugs would get caught.

For now instead, I have to either refactor the code permanently, or temporary replace all problematic annotations with Any (which is effectively way No.1 but with manual code modification) - it works, but this way a lot more potential bugs remain unnoticed.

And I certainly agree that such shortcuts are better as configurable options, and not as default behavior.

@agronholm
Copy link
Owner

If I had to choose, I'd go for option 1 as an opt-in setting.

@agronholm
Copy link
Owner

Thinking about this more, I could actually implement both as a policy setting on TypeChecker.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 8, 2019

Both sounds really good.

Thank you very much!

@agronholm
Copy link
Owner

I still need to document this but the README is getting kinda long. I need to consider building actual Sphinx documentation.

@jolaf
Copy link
Contributor Author

jolaf commented Aug 11, 2019

Cool, thanks!

@HitLuca
Copy link

HitLuca commented Jul 14, 2021

I am having the same issue as @jolaf , but in my case my project entrypoint uses install_import_hook. Is there a way to make it work too? My main.py file looks like this:

from typeguard.importhook import install_import_hook

install_import_hook("my_project")

from my_project.this import do_this
from my_project.that import do_that

if __name__ == "__main__":
    do_this()
    do_that()

@agronholm
Copy link
Owner

Is there a way to make what work? Elaborate.

@HitLuca
Copy link

HitLuca commented Jul 14, 2021

Sorry about that, I just tried to deal with this issue for enough time to make it obvious to myself and forgot to add context.
Basically I stumbled upon the same issue as @jolaf, in which circular imports through type hints break typeguard. From what I can see (and test), changing the entrypoint of the sample project from

with TypeChecker(('__main__', 'A', 'B')):
    A().f(B())

to

with TypeChecker(('__main__', 'A', 'B'), forward_refs_policy=ForwardRefPolicy.WARN):
    A().f(B())

fixes the issue.

Now, my project uses install_import_hook, and I was wondering how to fix the circular import issue (or have something similar to the policy change) in my case (shown above). Similar to @jolaf case, refactoring my entire project is not feasible, but I would love to be able to use this package since it's very useful in conjunction with mypy.
Even having the same kind of workaround as with TypeChecker (a forward_refs_policy parameter) would be great, but since I didn't dig into the codebase I'm not sure if it's feasible or makes sense.

@HitLuca
Copy link

HitLuca commented Jul 14, 2021

A minimal working example for my situation would be this:

A.py

from typeguard.importhook import install_import_hook

install_import_hook("B")

from B import B


class A:
    def f(self, b: B) -> None:
        b.f(self)


if __name__ == "__main__":
    A().f(B())

B.py

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from A import A


class B:
    def f(self, a: "A") -> None:
        print("OK")

@agronholm
Copy link
Owner

Couldn't you just import the modules and annotate like a: "A.A"?

@HitLuca
Copy link

HitLuca commented Jul 15, 2021

As for OP, I'm working on a large codebase, which makes refactoring each occurrence not feasible. Given that the sample are fully Python (code and type hints) compliant, I would suggest having a way to ignore or avoid failure on such occasions, similar to the workaround implemented for the TypeChecker class.

@agronholm
Copy link
Owner

The best I could do is to not touch TYPE_CHECKING, but would the tool be of any use to you then?

@HitLuca
Copy link

HitLuca commented Jul 15, 2021

Surely it would be better than having to not use it, assuming it's something that can be toggled. In that way the user has to actively allow typeguard to ignore some type hints

@agronholm
Copy link
Owner

I think I understand the problem a bit better now. It's not TYPE_CHECKING that is the issue (since typeguard does not mess with it anyway). What you want here is to ignore the NameError raised by TypeGuard when it's trying to evaluate the "A" annotation. The @typeguard_ignore decorator works here, but a configuration option could be added to ignore annotations that raised NameError during evaluation. Would that work for you?

@HitLuca
Copy link

HitLuca commented Jul 15, 2021

Yes exactly, since I know that the annotations make sense I would like typeguard to ignore the errors, as with ForwardRefPolicy.WARN in the TypeChecker initialization. Of course, having typeguard know that there isn't an issue in the project would be the best, but if we have this limitation at the moment an option is the best workaround in my opinion.
Thank you for taking the time to help me out on this issue, which for some reason is not nearly as common as I thought (since no issues similar to this have been opened)

@HitLuca
Copy link

HitLuca commented Aug 4, 2021

Any update on this?

@agronholm
Copy link
Owner

No (see #198).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants