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

Make the fine-grained mergechecker work again #8313

Merged
merged 3 commits into from
Jan 23, 2020
Merged

Conversation

msullivan
Copy link
Collaborator

This fixes some stuff in the mergechecker that breaks with current
mypy's and also some bugs that the mergechecker caught. The entire
fine-grained test suite now passes with the merge checker on, which I
don't think has ever been true before.

This means that we could turn it on by default, but it doubles the
runtime of the fine-grained tests (from 90s CPU time to 180s CPU time
on my laptop), so I've left it off for now.

The motivation here is that I knew intersect_callable's creation of
types during typechecking used to run afoul of the consistency checker
and so I was nervous that #8305 would cause more problems by adding
more logic of that kind. It no longer does, probably as a result of
the semantic analyzer rewrite, so I think we are in the clear on that.

This fixes some stuff in the mergechecker that breaks with current
mypy's and also some bugs that the mergechecker caught. The entire
fine-grained test suite now passes with the merge checker on, which I
don't think has ever been true before.

This means that we could turn it on by default, but it doubles the
runtime of the fine-grained tests (from 90s CPU time to 180s CPU time
on my laptop), so I've left it off for now.

The motivation here is that I knew intersect_callable's creation of
types during typechecking used to run afoul of the consistency checker
and so I was nervous that #8305 would cause more problems by adding
more logic of that kind. It no longer does, probably as a result of
the semantic analyzer rewrite, so I think we are in the clear on that.
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I just have one suggestion.

# Clear out some stale data to avoid memory leaks and astmerge
# validity check confusion
analyzer.statement = None
delattr(analyzer, 'cur_mod_node')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not del analyzer.cur_mod_node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I forgot how to write python apparently

@msullivan msullivan merged commit 9c2b8b3 into master Jan 23, 2020
@msullivan msullivan deleted the fg-consistency branch January 23, 2020 21:46
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

Successfully merging this pull request may close these issues.

2 participants