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

Untangle most of the big import cycle #7397

Merged
merged 8 commits into from Aug 27, 2019

Conversation

@msullivan
Copy link
Collaborator

commented Aug 27, 2019

Currently mypy has a 47 module import cycle, which is not great.

This untangles things, leaving the following cycles:

  • Size 2: mypy.build, mypy.semanal_main
  • Size 2: mypy.plugin, mypy.interpreted_plugin
  • Size 4: mypy.checker, mypy.checkexpr, mypy.checkmember, mypy.checkstrformat. The type checker itself. This could be untangled by having the sub-checkers operate on an interface of the checker, like the semantic analyzer does.
  • Size 5: mypy.nodes, mypy.types, mypy.type_visitor, mypy.visitors, mypy.strconv. This can't really be untangled.
  • Size 14: mypy.messages, mypy.expandtype, mypy.erasetype, mypy.typevars, mypy.maptype, mypy.typeops, mypy.sametypes, mypy.subtypes, mypy.constraints, mypy.applytype, mypy.join, mypy.meet, mypy.solve, mypy.infer. The "typeops tangle". Untangling a little further is probably possible but will require some thought and actual logic changes to code. Messages can definitely be removed.

The untangling done here is pretty simple and consists of two main parts:

  • Remove mypy.types's dependency on the type ops tangle by moving all of its functions that depend on it into mypy.typeops.
  • Remove the dependency of the type ops tangle on the type checker by moving some functions from mypy.checkmember to mypy.typeops.
@msullivan msullivan requested review from JukkaL, gvanrossum and ilevkivskyi Aug 27, 2019
Copy link
Member

left a comment

Yay! Are we going to use black next? :-)

@@ -237,3 +237,55 @@ def callable_corresponding_argument(typ: CallableType,
and is_equivalent(by_name.typ, by_pos.typ)):
return FormalArgument(by_name.name, by_pos.pos, by_name.typ, False)
return by_name if by_name is not None else by_pos


def make_simplified_union(items: Sequence[Type],

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Aug 27, 2019

Member

I know this will break Dropbox's Edgestore plugin. It's easily fixed, but I wonder how many other 3rd party plugins will be affected?

This comment has been minimized.

Copy link
@JukkaL

JukkaL Aug 27, 2019

Collaborator

We should document the change in the issue that tracks plugin API changes (and potentially in mypy release notes).

The introduction of recursive types will break many plugins as well, probably more than this change.

@@ -87,7 +87,9 @@ def visit_literal_type(self, t: LiteralType) -> ProperType:

def visit_union_type(self, t: UnionType) -> ProperType:
erased_items = [erase_type(item) for item in t.items]
return UnionType.make_simplified_union(erased_items)
from mypy.typeops import make_simplified_union # asdf
# XXX: does this need to be simplified?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Aug 27, 2019

Collaborator

I think sometimes it may be useful. For example we probably want Union[List[T], List[S]] become just List[Any], not Union[List[Any], List[Any]]. I think a similar applies to the question below.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

This could be untangled by having the sub-checkers operate on an interface of the checker, like the semantic analyzer does.

I think this makes sense.

The "typeops tangle". Untangling a little further is probably possible but will require some thought and actual logic changes to code.

I would like to point out one nasty dependency: mypy.subtypes.unify_generic_callable() pulls in whole lot of things. This may be not easy to fix however.

Messages can definitely be removed.

Good idea!

@JukkaL
JukkaL approved these changes Aug 27, 2019
Copy link
Collaborator

left a comment

This is great! I've been thinking about doing this myself for a long time but never got around to doing it.

@@ -237,3 +237,55 @@ def callable_corresponding_argument(typ: CallableType,
and is_equivalent(by_name.typ, by_pos.typ)):
return FormalArgument(by_name.name, by_pos.pos, by_name.typ, False)
return by_name if by_name is not None else by_pos


def make_simplified_union(items: Sequence[Type],

This comment has been minimized.

Copy link
@JukkaL

JukkaL Aug 27, 2019

Collaborator

We should document the change in the issue that tracks plugin API changes (and potentially in mypy release notes).

The introduction of recursive types will break many plugins as well, probably more than this change.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

See #6329 (likely caused by the one I mentioned). Also this PR essentially fixes #93

@msullivan

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 27, 2019

I went and mostly fixed #6329; we can now import any module first except for type_visitor.py. (That one is fixable, it's just a bit more work).

Would it be worth adding a test at some point for that?

@msullivan msullivan merged commit 5cd1dab into master Aug 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msullivan msullivan deleted the untangle branch Aug 27, 2019
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

@msullivan

Would it be worth adding a test at some point for that?

If it is not too tricky I think it makes sense to add some tests that would prevent us from recreating (or enlarging) those import cycles.

@JukkaL

This comment has been minimized.

Copy link
Collaborator

commented Aug 29, 2019

We could perhaps run another incremental build after self check in verbose mode and assert that the largest import cycle is below some size limit. Alternatively, we could modify verbose mode to write more details about SCCs in incremental mode and make more specific assertions, such as requiring that the nodes, typeops and checker cycles are disjoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.