Skip to content

Detect metaclass conflicts, refs #13563 #13565

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

Closed
wants to merge 17 commits into from

Conversation

sobolevn
Copy link
Member

Two questions:

  1. Is copying cpython's TypeError message is good enough?
  2. Should I ignore .pyi files from check? Or should it say consistent with .py files?

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 31, 2022

Without known exceptions it generates a lot of false-positives. Let's wait for the latest mypy-primer report.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

I need to fix sympy's case, right now we handle it incorrectly.

@github-actions

This comment has been minimized.

@sobolevn sobolevn marked this pull request as draft August 31, 2022 12:50
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 31, 2022

Ok, looks like I found the problem:

  • Usually metaclass is set here:
    defn.info.metaclass_type = defn.info.calculate_metaclass_type()
  • But, in case of sympy for some reason metaclass= points to a PlaceholderNode, so it does not calculate .metaclass_type and returns early:

    mypy/mypy/semanal.py

    Lines 2070 to 2072 in 9393fa1

    if isinstance(sym.node, PlaceholderNode):
    self.defer(defn)
    return
  • .metaclass_type is never recalculated

I will address this in a separate PR.

@sobolevn
Copy link
Member Author

So, basically we have a bug in mypy that was revealed by this.

@sobolevn
Copy link
Member Author

Proof, I added these lines to sympy on master branches (both tools):

reveal_type(AssocOp)
reveal_type(type(AssocOp))
reveal_type(AssocOp.__lt__)
sympy/core/operations.py:460: note: Revealed type is "Any"
sympy/core/operations.py:461: note: Revealed type is "Type[Any]"
sympy/core/operations.py:462: note: Revealed type is "Any"

Link: https://github.com/sympy/sympy/blob/c07abcc94e5b1521062c84d6d0a21b96dedfe07a/sympy/core/operations.py#L459

This project has lots of circular imports. They are everywhere:

Some of them are causing this.

@sobolevn sobolevn marked this pull request as ready for review September 1, 2022 10:17
@github-actions

This comment has been minimized.

Michael0x2a and others added 2 commits September 2, 2022 16:36
…python#13575)

This diff:

- Fixes python#8129
- Fixes python#13043
- Fixes python#13167

For more concise repros of these various issues, see the modified test files. But in short, there were two broad categories of errors:

1.  Within the deferred pass, we tend to infer 'Any' for the types of different variables instead of the actual type. This interacts badly with our unreachable and disallow-any checks and causes spurious errors.

    Arguably, the better way of handling this error is to only collect errors during the final pass. I briefly experimented with this
    approach, but was unable to find a clean, efficient, and non-disruptive way of implementing this. So, I settled for sprinkling in a few more `not self.current_node_deferred` checks.

2.  The `self.msg.disallowed_any_type(...)` call is normally guarded behind a `not self.chk.current_node_deferred` check. However, we were bypassing this check for `except` block assignments because we were deliberately setting that flag to False to work around some bug. For more context, see python#2290.

    It appears we no longer need this patch anymore. I'm not entirely sure why, but I'm guessing we tightened and fixed the underlying problem with deferred passes some time during the past half-decade.
@sobolevn
Copy link
Member Author

sobolevn commented Sep 3, 2022

Blocked by: #13579

@sobolevn
Copy link
Member Author

sobolevn commented Sep 3, 2022

Going to open a new one, since this PR was affected by several other commits.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

sobolevn added a commit that referenced this pull request Sep 4, 2022
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.

3 participants