Skip to content

Do not block on duplicate base classes#15367

Merged
sobolevn merged 4 commits intomasterfrom
issue-15349
Jun 5, 2023
Merged

Do not block on duplicate base classes#15367
sobolevn merged 4 commits intomasterfrom
issue-15349

Conversation

@sobolevn
Copy link
Copy Markdown
Member

@sobolevn sobolevn commented Jun 4, 2023

@JelleZijlstra I don't really have a prefence on this issue. I am fine with both blocking and non-blocking errors here.

But, it looks like that making this non-blocking is quite straight-forward.
Closes #15349

@github-actions

This comment has been minimized.

Comment thread test-data/unit/check-classes.test Outdated
...

b: B
# We use dummy type info, so no real attrs will be there:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's pretty unfortunate, can we make it inherit from Any or just skip the duplicate base instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeap, got it. Fixed!

@github-actions

This comment has been minimized.

@sobolevn sobolevn requested a review from JelleZijlstra June 4, 2023 17:43
Copy link
Copy Markdown
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This looks good to me!

One possible improvement here could be to deduplicate the base classes when calculating MRO, so instead of Any MRO we still get the MRO as if the base class was unduplicated.

Comment thread test-data/unit/check-classes.test Outdated
@sobolevn
Copy link
Copy Markdown
Member Author

sobolevn commented Jun 4, 2023

@hauntsaninja I don't fill comfortable doing this. Because we can have two generic bases: Some[int] and Some[str] - which one we should remove? This way we can generate strange errors that won't help much. Any is the default way to handle errors inside, so let's stick to it.

@hauntsaninja
Copy link
Copy Markdown
Collaborator

Okay! I went ahead and added a test case for that situation

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 4, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision) got 1.36x slower (40.8s -> 55.5s)

@sobolevn sobolevn merged commit 66bd2c4 into master Jun 5, 2023
@sobolevn sobolevn deleted the issue-15349 branch June 5, 2023 07:21
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.

“Duplicate base class” blocks checking

3 participants