CI: enable a few more mypy checks #60

twoertwein · 2022-07-01T15:18:52Z

require specific error codes (enable_error_code = "ignore-without-code")
require explicit exports of imports (through redudant as or in __all__, pyright does the same, implicit_reexport)
when pyright and mypy disagree, we can use pyright-specific ignore comments (warn_unused_ignores)

What is the goal of the tests inside TYPE_CHECKING? Is that for compatibility with older versions or are these placeholder for future "not-tests"?

Dr-Irv · 2022-07-02T19:05:02Z

require specific error codes (enable_error_code = "ignore-without-code")

require explicit exports of imports (through redudant as or in __all__, pyright does the same, implicit_reexport)

when pyright and mypy disagree, we can use pyright-specific ignore comments (warn_unused_ignores)

What is the goal of the tests inside TYPE_CHECKING? Is that for compatibility with older versions or are these placeholder for future "not-tests"?

The goal of those tests are code that would raise an Exception if you executed it. In theory, each of those lines should be picked up by the type checker as being incorrect, so you would need a # type: ignore to tell the type checker that you are expecting it to find an error. The issue here is that between pyright and mypy, either or both of them will pick up that the code is incorrect. If both do, or just mypy does, we're fine with a # type: ignore comment. If only pyright does, then we can use the pyright notation you included in this PR. If none of them do, it means our stubs are too wide.

Dr-Irv

As I wrote here: #60 (comment) I'm hesitant to make these changes. We want to have tests that the type checker should pick up as being invalid code. Not sure if we change the stubs to make both mypy and pyright pick them up, or we leave the warn_unused_ignores as it was.

Also, see discussion here: microsoft/pyright#2839

pyright did add a feature reportUnnecessaryTypeIgnoreComment as a result of that issue, and maybe you could experiment with that as well.

Dr-Irv · 2022-07-02T18:55:20Z

pyproject.toml

 # Miscellaneous strictness flags
 allow_untyped_globals = false
 allow_redefinition = false
 local_partial_types = false
-implicit_reexport = true
+implicit_reexport = false


Can you add comment here and with enable_error_code that these are changes from pandas?

implicit_reexport is still true in pandas

Dr-Irv · 2022-07-02T18:58:19Z

tests/test_timefuncs.py

@@ -175,6 +175,6 @@ def fail_on_adding_two_timestamps() -> None:
    s1 = pd.Series(pd.to_datetime(["2022-05-01", "2022-06-01"]))
    s2 = pd.Series(pd.to_datetime(["2022-05-15", "2022-06-15"]))
    if TYPE_CHECKING:
-        ssum: pd.Series = s1 + s2  # type: ignore
+        ssum: pd.Series = s1 + s2


So this should have a # type: ignore, because we should be picking up that this is not allowed, i.e., you can't add two series consisting of Timestamps.

Dr-Irv · 2022-07-02T19:10:53Z

tests/test_interval.py

-        i1 + pd.Timestamp("2000-03-03")  # type: ignore
-        i1 * 3  # type: ignore
-        i1 * pd.Timedelta(seconds=20)  # type: ignore
+        20 in i1


Removing this type: ignore means we think this code is legal, but it shouldn't be. You can't check that an int is contained inside of an Interval[Timestamp], so the type checker should pick that up.

I'm hesitant to make this change unless we can fix the stub to error out on this line.

I'm pretty sure at one point it did used to pick that up - not sure what changed.

I think it will take quite some effort to remove annotations like these

def __add__(self, other: TimestampSeries) -> TimestampSeries: ... def __add__(self, other: Timestamp) -> TimestampSeries: ..

but also allow legit combinations.

Dr-Irv · 2022-07-02T19:11:30Z

tests/test_interval.py

-        pd.Timestamp("2001-01-02") in i2  # type: ignore
-        i2 + pd.Timedelta(seconds=20)  # type: ignore
+        pd.Timestamp("2001-01-02") in i2  # pyright: ignore[reportGeneralTypeIssues]
+        i2 + pd.Timedelta(seconds=20)  # pyright: ignore[reportGeneralTypeIssues]


This is one of these cases where pyright is correctly picking up the error, but mypy is not.

twoertwein · 2022-07-02T19:43:17Z

The goal of those tests are code that would raise an Exception if you executed it.

Great, I wasn't aware that there are already negative test cases!

We want to have tests that the type checker should pick up as being invalid code.

The current way of having the ignore comments but not checking whether they are needed, doesn't test these negative test cases. As you mentioned it would be good to also use pyright to check whether it needs the ignore comments.

I will try to look into why mypy/pyright accept some of these negative tests. If we are not able to make mypy/pyright error on the negative tests, should we simply create an issue to keep track of these negative tests being accepted?

Dr-Irv · 2022-07-02T20:06:28Z

The goal of those tests are code that would raise an Exception if you executed it.

Great, I wasn't aware that there are already negative test cases!

We want to have tests that the type checker should pick up as being invalid code.

The current way of having the ignore comments but not checking whether they are needed, doesn't test these negative test cases. As you mentioned it would be good to also use pyright to check whether it needs the ignore comments.

I will try to look into why mypy/pyright accept some of these negative tests. If we are not able to make mypy/pyright error on the negative tests, should we simply create an issue to keep track of these negative tests being accepted?

Maybe we should move all of the negative tests to a separate test file, keep the ones (with the correct # type: ignore or #pyright comments, and then note the ones that are currently "failing", in the sense that we want to type checker to error out on them, and those are the ones that go into an issue.

twoertwein · 2022-07-02T22:31:26Z

I will separate this PR into two PRs: w/wo warn_unused_ignores.

twoertwein · 2022-07-03T00:56:44Z

It will be difficult to have mypy and pyright both reporting which ignores the don't need as mypy has no mypy-specific ignore comments (python/mypy#12358) but pyright does not need a lot of the ignores in the pandas-stubs itself:

pandas-stubs/_libs/tslibs/timedeltas.pyi:118:17 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:104:67 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:136:54 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:137:54 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:138:54 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:139:54 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:141:17 - error: Unnecessary "# type: ignore" comment
pandas-stubs/_libs/tslibs/timestamps.pyi:148:17 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/arraylike.pyi:15:72 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/arraylike.pyi:16:72 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/series.pyi:1107:61 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/series.pyi:1138:61 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/series.pyi:1751:61 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/series.pyi:1752:20 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/arrays/datetimelike.pyi:45:104 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/arrays/datetimes.pyi:15:64 - error: Unnecessary "# type: ignore" comment
pandas-stubs/core/indexes/base.pyi:208:51 - error: Unnecessary "# type: ignore" comment

Dr-Irv · 2022-07-04T04:30:47Z

It will be difficult to have mypy and pyright both reporting which ignores the don't need as mypy has no mypy-specific ignore comments (python/mypy#12358) but pyright does not need a lot of the ignores in the pandas-stubs itself:

Yes, and that creates the dilemma for us. We have to put in #type: ignore where we know mypy is wrong, or where we are expecting the type checker to report an error. But we can't separate out the cases between mypy and pyright

I'm presuming that this PR is not ready to be reviewed, given you said you'd split it into two.

twoertwein · 2022-07-04T12:42:46Z

I'm presuming that this PR is not ready to be reviewed, given you said you'd split it into two.

I think it should be ready to merge: I removed warn_unused_ignores.

Personally, I think it would be an improvement to already enable warn_unused_ignores (and use pyright-specific codes where needed without requiring that pyright reports its unneeded ignore comments). Especially, after your recent PRs, there are many negative test cases without any ignore comments.

Dr-Irv · 2022-07-04T19:11:41Z

Thanks @twoertwein

Dr-Irv · 2022-07-04T19:13:14Z

Especially, after your recent PRs, there are many negative test cases without any ignore comments.

Actually, I didn't add any negative tests. It turned out that using pytest.skip() was causing both mypy and pyright to not check the code that followed. But that code would not execute with pytest (e.g., df.plot()). So I used if TYPE_CHECKING to make sure the API was checked

CI: enable a few more mypy checks

10b2a16

Dr-Irv reviewed Jul 2, 2022

View reviewed changes

twoertwein added 4 commits July 2, 2022 18:39

-warn_unused_ignores = false

b39b21f

Merge remote-tracking branch 'upstream/main' into mypy_negative

54e39a1

comment about mypy options

eeb0ffa

keep warn_unused_ignores comment

0515650

twoertwein added 3 commits July 4, 2022 08:24

Merge remote-tracking branch 'upstream/main' into mypy

38e4737

ignore comments

fc6f603

pyupgrade+isort

3a73fd4

Dr-Irv approved these changes Jul 4, 2022

View reviewed changes

Dr-Irv merged commit 0fdebaa into pandas-dev:main Jul 4, 2022

twoertwein deleted the mypy branch July 5, 2022 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: enable a few more mypy checks #60

CI: enable a few more mypy checks #60

twoertwein commented Jul 1, 2022

Dr-Irv commented Jul 2, 2022

Dr-Irv left a comment

Dr-Irv Jul 2, 2022

twoertwein Jul 2, 2022

Dr-Irv Jul 2, 2022

Dr-Irv Jul 2, 2022

twoertwein Jul 3, 2022

Dr-Irv Jul 2, 2022

twoertwein commented Jul 2, 2022

Dr-Irv commented Jul 2, 2022

twoertwein commented Jul 2, 2022

twoertwein commented Jul 3, 2022

Dr-Irv commented Jul 4, 2022

twoertwein commented Jul 4, 2022

Dr-Irv commented Jul 4, 2022

Dr-Irv commented Jul 4, 2022

CI: enable a few more mypy checks #60

CI: enable a few more mypy checks #60

Conversation

twoertwein commented Jul 1, 2022

Dr-Irv commented Jul 2, 2022

Dr-Irv left a comment

Choose a reason for hiding this comment

Dr-Irv Jul 2, 2022

Choose a reason for hiding this comment

twoertwein Jul 2, 2022

Choose a reason for hiding this comment

Dr-Irv Jul 2, 2022

Choose a reason for hiding this comment

Dr-Irv Jul 2, 2022

Choose a reason for hiding this comment

twoertwein Jul 3, 2022

Choose a reason for hiding this comment

Dr-Irv Jul 2, 2022

Choose a reason for hiding this comment

twoertwein commented Jul 2, 2022

Dr-Irv commented Jul 2, 2022

twoertwein commented Jul 2, 2022

twoertwein commented Jul 3, 2022

Dr-Irv commented Jul 4, 2022

twoertwein commented Jul 4, 2022

Dr-Irv commented Jul 4, 2022

Dr-Irv commented Jul 4, 2022