-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add a new, lower priority for imports inside "if MYPY" #2167
Conversation
@@ -3024,12 +3039,9 @@ def infer_if_condition_value(expr: Node, pyversion: Tuple[int, int], platform: s | |||
elif name == 'PY3': | |||
result = ALWAYS_TRUE if pyversion[0] == 3 else ALWAYS_FALSE | |||
elif name == 'MYPY' or name == 'TYPE_CHECKING': | |||
result = ALWAYS_TRUE | |||
result = MYPY_TRUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer TYPECHECK_TRUE
or TYPE_CHECK_TRUE
to be intuitively linked to the TYPE_CHECK
flag, especially if you intend to phase MYPY
flag out. (Similarly MarkImportsTypechekOnlyVisitor
, mark_block_typecheck_only
, Import.is_typecheck_only
, and related documentation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we don't really care about other type checkers in this codebase, do we? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. My concern was greppability, but it is a minor one (it's only the connection between MYPY_TRUE and TYPE_CHECKING that is not immediate; each is perfectly greppable on its own).
@@ -394,20 +407,21 @@ def correct_rel_imp(imp: Union[ImportFrom, ImportAll]) -> str: | |||
for imp in file.imports: | |||
if not imp.is_unreachable: | |||
if isinstance(imp, Import): | |||
pri = PRI_MED if imp.is_top_level else PRI_LOW | |||
pri = import_priority(imp, PRI_MED) | |||
ancestor_pri = import_priority(imp, PRI_LOW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the story of ancestor_pri
. I'm sure I can figure it out but it feels like it deserves a comment - how ancestor_pri
is pri
with a different default? It's not obvious to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used once, for the ancestors. The reason is that if you have "import x.y" you depend on both x.y and on x, but the priority for x should be less than for x.y. There are many other ways we could potentially improve the heuristics here but they're still just heuristics, so I don't really care. The change here is really just that if we decide an import is inside if MYPY
it should get an even lesser priority.
@@ -308,9 +308,22 @@ def default_lib_path(data_dir: str, pyversion: Tuple[int, int]) -> List[str]: | |||
PRI_MED = 10 # top-level "import X" | |||
PRI_LOW = 20 # either form inside a function | |||
PRI_INDIRECT = 30 # an indirect dependency | |||
PRI_MYPY = 40 # inside "if MYPY" or "if typing.TYPE_CHECKING" | |||
PRI_ALL = 99 # include all priorities | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only a single additional priority? Looks like the concerns are pretty independent. I would think a more detailed set of priorities, e.g. lexicographical ordering of the form
(is_mypy, is_toplevel, is_from)
might help the algorithm breaking cycles less arbitrarily, Alternatively,
(is_mypy, nesting_level, is_from)
perhaps even more so (the actual tuple is of course unimportant; I only talk about the implied ordering).
I don't know if all this will have any effect on any real codebase, but the current system feels somewhat arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be arbitrary. There are ideas for something better in the tracker, but I'd like to see how things work out with this small improvement first.
TRUTH_VALUE_UNKNOWN: TRUTH_VALUE_UNKNOWN, | ||
MYPY_TRUE: MYPY_FALSE, | ||
MYPY_FALSE: MYPY_TRUE, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you consider this numbering more readable than the range [-2, 2], with the obvious negation? An explicit mapping can still be used just to be sure. TRUTH_UNKNOWN should be zero this way or the other,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numbers are entirely uninteresting. I intentionally changed them around a bit to show that this is so.
# The condition is always false, so we skip the if/elif body. | ||
mark_block_unreachable(s.body[i]) | ||
elif result == ALWAYS_TRUE: | ||
elif result in (ALWAYS_TRUE, MYPY_TRUE): | ||
# This condition is always true, so all of the remaining | ||
# elif/else bodies will never be executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrasing here is now slightly inaccurate, and perhaps the previous comment too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the meaning of "always". :-)
def visit_import_all(self, node: ImportAll) -> None: | ||
node.is_mypy_only = True | ||
|
||
|
||
def is_identity_signature(sig: Type) -> bool: | ||
"""Is type a callable of form T -> T (where T is a type variable)?""" | ||
if isinstance(sig, CallableType) and sig.arg_kinds == [ARG_POS]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Completely off topic but such comparisons assumes that arg_kinds
is a list, in a way that mypy does not warn about. Feels like a subtle bug waiting to happen).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have a bug open about that already.
# Inferred value of an expression. | ||
ALWAYS_TRUE = 0 | ||
ALWAYS_FALSE = 1 | ||
TRUTH_VALUE_UNKNOWN = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments: uniform prefix will make it slightly easier to search and possibly replace, as will happen eventually when it will become an Enum. Not that TRUTH_VALUE_UNKNOWN is a hard name to search :)
How about giving this type a dedicated alias until then? making it under the namespace class Truth: ...
already, before its values gets spread any further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that would just cause more code churn and the benefit would be minuscule.
I have the time, sure. I can't say I already know how to test this; I will study it tomorrow. |
|
The new truth values suddenly makes it hard to be sure what's happening in other functions that return truth values. I don't have any solution for that but I do think it's a problem. |
I'll fix the docstring. There are only 5 functions using these constants.
Once we drop Python 3.3 support (or we decide to pull in enum34) we can
switch this to an enum so that's clearer. We can also rename the constants
so they have a common unique prefix (like the priority values), which makes
them easier to grep for. But I think it's not a problem right now, and I
don't want to worry too much about everything that could possibly go wrong.
|
Do you want me to add tests to I am not familiar enough with the code, but I couldn't find anything like a [case testSomeDep]
[module x]
class Base: pass
def foo() -> None:
import y
reveal_type(y.Sub.attr) # E: Revealed type is builtins.int
[module y]
import x
class Sub(x.Base):
attr = 0
[check y] This is more general that this PR however. I think this PR should also be asserted in the assertion pass somehow, verifying that nested dependencies are prioritized lower than dependencies on enclosing scope, for example. Of course there are no assertion passes yet. |
These tests should probably just be added as new test cases to
test-data/unit/check-modules.test. I think it has all the mechanism needed
to construct the multiple test modules already. I would like to sees tests
that fail without this PR and pass with it applied.
|
I missed that, sorry. I'm on it. |
@@ -2983,12 +2993,16 @@ def infer_reachability_of_if_statement(s: IfStmt, | |||
platform: str) -> None: | |||
for i in range(len(s.expr)): | |||
result = infer_if_condition_value(s.expr[i], pyversion, platform) | |||
if result == ALWAYS_FALSE: | |||
if result in (ALWAYS_FALSE, MYPY_FALSE): | |||
# The condition is always false, so we skip the if/elif body. | |||
mark_block_unreachable(s.body[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, the intention is that if not TYPE_CHECKING:
means "don't type check"? That's fine but not explicitly documented in PEP-484.
In this case
if TYPE_CHECKING:
import expensive_mod
else:
# I don't care about runtime annotations
expensive_mod = CleverPseudoModule('SomeClass')
def a_func(arg: expensive_mod.SomeClass) -> None:
...
The else branch will not be checked at all. Again, it's fine, but I think it should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though PEP 484 only gives a positive example, it clearly states that TYPE_CHECKING
is considered
True
during type checking (or other static analysis) butFalse
at runtime.
I don't see any ambiguity there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I'll push a new version with some docstring/comment updates. Do you have unit tests for this yet?
@@ -308,9 +308,22 @@ def default_lib_path(data_dir: str, pyversion: Tuple[int, int]) -> List[str]: | |||
PRI_MED = 10 # top-level "import X" | |||
PRI_LOW = 20 # either form inside a function | |||
PRI_INDIRECT = 30 # an indirect dependency | |||
PRI_MYPY = 40 # inside "if MYPY" or "if typing.TYPE_CHECKING" | |||
PRI_ALL = 99 # include all priorities | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be arbitrary. There are ideas for something better in the tracker, but I'd like to see how things work out with this small improvement first.
@@ -394,20 +407,21 @@ def correct_rel_imp(imp: Union[ImportFrom, ImportAll]) -> str: | |||
for imp in file.imports: | |||
if not imp.is_unreachable: | |||
if isinstance(imp, Import): | |||
pri = PRI_MED if imp.is_top_level else PRI_LOW | |||
pri = import_priority(imp, PRI_MED) | |||
ancestor_pri = import_priority(imp, PRI_LOW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used once, for the ancestors. The reason is that if you have "import x.y" you depend on both x.y and on x, but the priority for x should be less than for x.y. There are many other ways we could potentially improve the heuristics here but they're still just heuristics, so I don't really care. The change here is really just that if we decide an import is inside if MYPY
it should get an even lesser priority.
# Inferred value of an expression. | ||
ALWAYS_TRUE = 0 | ||
ALWAYS_FALSE = 1 | ||
TRUTH_VALUE_UNKNOWN = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that would just cause more code churn and the benefit would be minuscule.
TRUTH_VALUE_UNKNOWN: TRUTH_VALUE_UNKNOWN, | ||
MYPY_TRUE: MYPY_FALSE, | ||
MYPY_FALSE: MYPY_TRUE, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numbers are entirely uninteresting. I intentionally changed them around a bit to show that this is so.
@@ -2983,12 +2993,16 @@ def infer_reachability_of_if_statement(s: IfStmt, | |||
platform: str) -> None: | |||
for i in range(len(s.expr)): | |||
result = infer_if_condition_value(s.expr[i], pyversion, platform) | |||
if result == ALWAYS_FALSE: | |||
if result in (ALWAYS_FALSE, MYPY_FALSE): | |||
# The condition is always false, so we skip the if/elif body. | |||
mark_block_unreachable(s.body[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though PEP 484 only gives a positive example, it clearly states that TYPE_CHECKING
is considered
True
during type checking (or other static analysis) butFalse
at runtime.
I don't see any ambiguity there.
# The condition is always false, so we skip the if/elif body. | ||
mark_block_unreachable(s.body[i]) | ||
elif result == ALWAYS_TRUE: | ||
elif result in (ALWAYS_TRUE, MYPY_TRUE): | ||
# This condition is always true, so all of the remaining | ||
# elif/else bodies will never be executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the meaning of "always". :-)
@@ -3024,12 +3039,9 @@ def infer_if_condition_value(expr: Node, pyversion: Tuple[int, int], platform: s | |||
elif name == 'PY3': | |||
result = ALWAYS_TRUE if pyversion[0] == 3 else ALWAYS_FALSE | |||
elif name == 'MYPY' or name == 'TYPE_CHECKING': | |||
result = ALWAYS_TRUE | |||
result = MYPY_TRUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we don't really care about other type checkers in this codebase, do we? :-)
def visit_import_all(self, node: ImportAll) -> None: | ||
node.is_mypy_only = True | ||
|
||
|
||
def is_identity_signature(sig: Type) -> bool: | ||
"""Is type a callable of form T -> T (where T is a type variable)?""" | ||
if isinstance(sig, CallableType) and sig.arg_kinds == [ARG_POS]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have a bug open about that already.
I didn't find any useful test case yet. Sorry. I could use pointers if you (or @JukkaL) have any. |
I will try to boil down some existing case we have, Monday.
…--Guido (mobile)
|
I think there's some mismatch between the unit tests and an installed mypy (I'm embarrassed to say I don't know how to run mypy without actually installing it :\ ). Running mypy, Jukka's example from Aug 12 #2016 fails without this PR and passes with it, but when I try to transform it into a unit test it crashes. I will investigate this crash. |
Turns out it was the usual missing fixture which pops up in the wrong places. Test case: [case testTypeCheckPrio]
# cmd: mypy -m part1 part2 part3 part4
[file part1.py]
from part3 import Thing
class FirstThing: pass
[file part2.py]
from part4 import part4_thing as Thing
[file part3.py]
from part2 import Thing
reveal_type(Thing)
[file part4.py]
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from part1 import FirstThing
def part4_thing(a: int) -> str: pass
[out]
tmp/part3.py:2: error: Revealed type is 'def (a: builtins.int) -> builtins.str'
[builtins fixtures/bool.pyi] |
Thanks for the test! Unfortunately some of our own code (full of import
cycles and hacks to work around them) actually no longer type-checks with
this fix. I'll have to dive into that deeper. Maybe your idea of using more
priorities based on the import form inside "if TYPE_CHECKING" will help.
|
What do you think about the idea of permuting the names in command line arguments? I've said it has significant slowdown but it's only so for a handful of cases - the cases for which it is important - so in total it's not a significant hit. |
I don't recall. Sorry! What was the idea?
…--Guido (mobile)
|
I wrote it as an edit, sorry... Here's the cut&paste: Since the original problem is sensitive to order of command line arguments, I have tried running all possible permutations, and it passes them all. I'm not sure whether this is a useful addition, but if you want to give it quick try, just replace from itertools import permutations
for module_data in permutations(self.parse_module(original_program_text, incremental)):
# indent the rest of the function I think it is a nice sanity check (unless I miss something and these were all actually the same test somehow). |
The command line argument ordering is only used when nothing else can
distinguish two files, so I doubt it is very useful to test different
orderings. (IIRC there's one test that tests specifically for argument
ordering.)
BTW To run mypy without installing, I use `PYTHONPATH=~/src/mypy python3 -m
mypy`.
|
I'm tempted to abandon this. I tried a bunch of variations, including increasing the priorities for code inside |
Personally I believe that the real solution requires taking incremental to the extreme, type checking expressions on demand (i.e explicitly building the dataflow graph and checking/inferring by need, caching whenever possible). This will require an even larger change than #1292, so I guess it will never happen. Back to the real world: is it possible to extract the dependency graphs from actual codebases, so they can be visualized and analyzed independently of the code, and perhaps we can find a common pattern? If we build such a tool, it can be applied any time someone opens an issue on the subject. |
Agreed. I am planning to round up the resources to do this, but it will be a while.
It will.
Extracting imports is pretty simple; I've written throwaway tools for this several times. I've also tried to visualize the dependency graphs, but the problem is that once you have a graph of several 100 nodes (like we do) a traditional node + arrow diagram just looks like an entirely black page. I've looked into other visualization styles too, and even once wrote something that can show dependencies as an Adjacency Matrix (Google it) and lets you interactively show and hide certain parts of the graph. But I wrote it in JS using D3, which is a bit out of my regular skill set, so I gave up before I got it working well. |
@gvanrossum Being somewhat familiar with the "C" codebase, I suspect the problems may be at least partially due to there being a "hub" module that gets imported by a huge number of modules in the cycle using Here's one idea that might be worth trying, if you haven't tried it yet: decrease the priority of @elazarg Fine-grained incrementalism would certainly solve this problem. We'd basically type check the entire cycle as a single unit, in dependency order. However, you are correct that it would be a big change, and we should first try any simpler approaches that are available to us. It's still perhaps the best thing to do -- the original design didn't anticipate large import cycles, and so it wasn't designed to cope with them. A simpler option would be run multiple type checking passes over each cycle until all the needed types have been inferred. We already do this over individual files, and it shouldn't be very hard to generalize this to cycles -- it would almost certainly be much easier that doing "real" finer-grained incrementalism. |
@gvanrossum Having a database containing dependency graphs "from the wild" can be a nice playground for experimenting with heuristics and other solutions for breaking cycles. I don't know what kind of information is needed to be there (the shape alone is not enough), so it might not be practical; but currently the only codebase available for me to look at is mypy. @JukkaL Fine-grained incrementalism can be helpful for IDEs too. |
@elazarg Yeah, that's part of what originally motivated the idea. IDE support is definitely on our radar. |
@elazarg You can also look at Zulip. It's open source and nearly 100%
annotated.
|
@JukkaL I tried that now, and it gives yet another set of errors. I suppose
I could go on and see for every variation what set of changes to the "C"
codebase would make them go away, but that's all pretty unsatisfactory, and
we don't really know (without reading a lot of code) which hacks they've
already employed to deal with the import cycle. I do know they've
complained about it a lot.
|
But if we don't do something to improve things (even it requires a significant set of tweaks to migrate to the new version) they'll have to keep adding those hacks in the future. Once the module ordering within the cycle is more stable, there should less trouble in the future (though there still likely is going to be some trouble). One-time, significant pain can be better than continuous, low-level pain. I wonder if it would be possible to look at the ordering of the import cycle historically, using both the old sorting method and some of the proposed variants. For example, pick 5 revisions over some time period, sort the biggest import cycle in each revision using all the algorithms, and see which algorithm produces the most stable module orderings, using some relevant metric. This wouldn't be perfect but at least it would be independent of the hacks they've employed. Stable ordering could mean that the relative order of as few modules as possible is changed: i.e., for each pair (x, y) of modules, we'd calculate whether x comes before y (or vice versa) in both orderings. Stability between two orderings would be the fraction of pairs that have the same order in both orderings (we'd only consider pairs that appear in both orderings). |
Do you think ranking modules by the number of externally-used definitions (within a single scc) can help? |
Abandoning this. |
FWIW, this would fix the specific example given in #2015. The reason is that that example specifically puts one import inside |
Ok, I'll probably look at this this tomorrow. |
Oops, lost track of this. Will continue tomorrow. |
return PRI_LOW | ||
if imp.is_mypy_only: | ||
# Inside "if MYPY" or "if typing.TYPE_CHECKING" | ||
return max(PRI_MED, toplevel_priority) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we'd make the priority of mypy-only imports even lower, since they can never affect the runtime module initialization order? I feel that more priority levels is generally better, as there will be fewer ties when sorting cycles and thus we should get a more stable ordering. So my suggestion is to create a new priority level for mypy-only imports, which would be either between PRI_LOW
and PRI_MED
or even lower than PRI_LOW
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And by lower I meant numerically higher...)
@@ -1313,3 +1313,27 @@ pass | |||
[file b] | |||
pass | |||
[out] | |||
|
|||
[case testTypeCheckPrio] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test case fail without the changes in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Current coverage is 83.17% (diff: 97.36%)@@ master #2167 diff @@
==========================================
Files 72 72
Lines 19048 19068 +20
Methods 0 0
Messages 0 0
Branches 3920 3921 +1
==========================================
+ Hits 15840 15860 +20
- Misses 2612 2613 +1
+ Partials 596 595 -1
|
@elazarg Do you have time to review this? I am missing tests -- do you understand how to add tests for this?