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

Possible bug with Generics coming from pandas-stubs #3955

Closed
Dr-Irv opened this issue Sep 17, 2022 · 3 comments
Closed

Possible bug with Generics coming from pandas-stubs #3955

Dr-Irv opened this issue Sep 17, 2022 · 3 comments
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working

Comments

@Dr-Irv
Copy link

Dr-Irv commented Sep 17, 2022

I tried to come up with a simple example to illustrate this, but couldn't do it. So maybe you want to pull the pandas-stubs code and look into it. I'm using pyright 1.1.271 and python 3.9.

If you grab the branch https://github.com/Dr-Irv/pandas-stubs/tree/tdindex_pyright and do pyright tests/test_timefuncs.py pandas-stubs, you will get these errors:

  c:\Code\pandas-stubs\tests\test_timefuncs.py:431:23 - error: "assert_type" mismatch: expected "DatetimeIndex" but received "TimedeltaIndex" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:432:23 - error: "assert_type" mismatch: expected "DatetimeIndex" but received "TimedeltaIndex" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:433:23 - error: "assert_type" mismatch: expected "DatetimeIndex" but received "TimedeltaIndex" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:451:21 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:454:21 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:456:23 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)

If in the file pandas-stubs/core/indexes/accessors.pyi, on lines 159-160, you interchange the order of DatetimeIndex and TimedeltaIndex in the declaration of the TypeVar, the error messages change:

  c:\Code\pandas-stubs\tests\test_timefuncs.py:451:21 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:454:21 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)
  c:\Code\pandas-stubs\tests\test_timefuncs.py:456:23 - error: "assert_type" mismatch: expected "TimedeltaIndex" but received "Any" (reportGeneralTypeIssues)

Questions:

  1. Why should the order of the types in the TypeVar matter?
  2. Why does it not infer that the return type of TimedeltaIndex.round() is TimedeltaIndex ?

Note that the classes DatetimeIndex and TimedeltaIndex are similar. This got uncovered when adding the accessors for TimedeltaIndex when the ones for DatetimeIndex already worked.

Note that mypy passes the tests independent of the order by typing mypy pandas-stubs tests\test_timefuncs.py --no-incremental

We have a workaround for now, by just repeating the declarations of the methods in the class _DatetimeRoundingMethods that need to return TimedeltaIndex (that code is commented out), but it's puzzling that the pattern I used before doesn't work.

@erictraut
Copy link
Collaborator

The problem is that you've created a circular dependency in your class declarations.

  1. The class _DatetimeRoundingMethods is parameterized by the TypeVar _DTRoundingMethodReturnType.
  2. The TypeVar _DTRoundingMethodReturnType uses the class TimedeltaIndex as a constraint, which means that TimedeltaIndex needs to be resolved to validate any explicit specialization of _DTRoundingMethodReturnType.
  3. The class TimedeltaIndex derives from the class TimedeltaIndexProperties.
  4. The class _DatetimeRoundingMethods derives from the class _DatetimeRoundingMethods[TimedeltaIndex], which is an explicit specialization of _DatetimeRoundingMethods.

When pyright's just-in-time type evaluator attempts to evaluate the types of any of these symbols, it runs into a situation where it cannot fully resolve any of them. It does its best to deal with the situation, but it's dealing with partially-evaluated types (e.g. classes where the full class hierarchy and MRO list is not yet understood or TypeVars where the constraint list contains partially-evaluated classes). If you change the order of the constraints in the TypeVar, you change the behavior.

Mypy is a multi-pass type evaluator, so it re-evaluates entire source files as many times as necessary until all of the types "settle" to their final types. This approach is much less efficient than pyright's, but it is able to handle certain circular dependencies that pyright struggles to handle.

If you are able to break that circular dependency somehow, that would be preferable. It would work with pyright as is and would improve type evaluation in mypy and other multi-pass evaluators.

I have added some (rather hacky) logic to pyright to handle this particular circular dependency in a more elegant manner. So your tests will pass as currently written with the next version of pyright.

erictraut pushed a commit that referenced this issue Sep 17, 2022
…hy — in particular, where a class is parameterized by a constrained TypeVar where one of the constraints includes the class or its ancestors. This addresses #3955.
@erictraut erictraut added bug Something isn't working addressed in next version Issue is fixed and will appear in next published version labels Sep 17, 2022
@Dr-Irv
Copy link
Author

Dr-Irv commented Sep 17, 2022

Thanks for your analysis and your feedback. What's interesting here is that the same circular dependency exists with DatetimeIndex and DatetimeIndexProperties, with the same _DatetimeRoundingMethods and _DTRoundingMethodReturnType, yet there were no issues there.

@erictraut
Copy link
Collaborator

This is addressed in pyright 1.1.272. It will also be included in a future release of pylance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants