Repository: posit-dev/py-shiny (language: python)

Issue:
[regression] Mypy fails with shiny==1.0.0
Seems like the same issue was fixed before but crept back in: https://github.com/posit-dev/py-shiny/issues/1332

Shiny is incompatible with `mypy`:

```
% mypy .
venv/lib/python3.11/site-packages/shiny/render/_data_frame_utils/_selection.py:154: error: invalid syntax  [syntax]
Found 1 error in 1 file (errors prevented further checking)
```

Unfortunately seems like it's [not possible](https://stackoverflow.com/questions/78345753/mypy-checking-pyi-in-venv-despite-excluding-it) to edit one's `mypy` configuration to get past a syntax error in a dependency. 

I'm working around it by just deleting line 154 of `_selection.py` for now. 
[regression] Mypy fails with `shiny==0.9.0`
Hi, 
I've found the following regression.

## Problem

With `mypy==1.9.0` and `shiny==0.9.0` and 

```py
import shiny
```

in `app.py`.

`mypy app.py` raises

```
➜ mypy app.py
/Users/pasza/Library/Caches/pypoetry/virtualenvs/test-shiny09-qk-unJu3-py3.12/lib/python3.12/site-packages/shiny/render/_data_frame_utils/_selection.py:155: error: invalid syntax  [syntax]
Found 1 error in 1 file (errors prevented further checking)
```

This doesn't happen with `shiny==0.8.1`, I get the expected `Success: no issues found in 1 source file`.

## Reason

The reason is the commented-out code in the source code 🙈 
In `_selection.py`, lines 154-155 are:

```py
# class CellSelectionAll(TypedDict):
#     type: Literal["all"]
```

`mypy` (I guess) interprets `# type:` as some kind of type hint for the thing in front of the `#` sign. Or something like that, just like `# type: ignore`. Or maybe `Literal["all"]` is just not proper syntax for `# type:...`

## Potential fix

Remove unused code, or mangle the commented code to not make it clash with `mypy`.

## After thought

Maybe this check could've/should've been added to CICD?

Best, 
Pasza

<details>

```
[tool.poetry]
name = "test-shiny09"
version = "0.1.0"
description = ""
authors = ["Piotr Pasza Storozenko <piotr@appsilon.com>"]
readme = "README.md"

[tool.poetry.dependencies]
python = "^3.12"
shiny = "0.9.0"
mypy = "^1.9.0"


[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
```

</details>


Discussion / hints:

I made a quick fix in 94d84c31.

We currently use pyright in strict mode for type checking this project. In my opinion, pyright works much better in my than mypy.

In theory, type checking with mypy in CI would be good, to avoid problems like the one that you ran into, where a project that that imports shiny makes use of `mypy` to do type checking. However, I don't think that using mypy in addition to pyright is practical for Shiny, because there are differences in the behavior of mypy and pyright, as documented here:
https://github.com/microsoft/pyright/blob/main/docs/mypy-comparison.md

On the Shiny code base, pyright reports zero type errors, but mypy reports 180 errors. I don't think getting it to pass both systems would not make the code meaningfully better.

The change I made in 94d84c31 fixed the syntax error, but mypy still thinks there are type errors. Hopefully fixing just the syntax error is enough to satisfy the use case you have run into.
Thanks for the fix.

Hmm, I personally don't personally like the fact that `pyright` has js dependencies and always preferred `mypy` for CI/CD (I use `pyright` in vs code), but it's good to know `shiny` has pyright-first support when it comes to type checking, thanks.

As you mentioned, making `shiny` internal code base pass both systems doesn't make sense to me neither, but making sure that `mypy` users can just import shiny without problems in their code base could be beneficial. 
It would be nice if it were as simple as having an `app.py` with `import shiny` in it, like in your case. However, there are a number submodules which are not automatically loaded this way, and so they would not be "tested" for syntax errors under mypy.

If it were possible to run `mypy` on the whole shiny codebase and report only mypy syntax errors, that is probably what we would want. However, I took a quick look at the mypy documentation and did not see a way to check only for mypy syntax errors. If you know of a way to do that, please let us know.
The same problem re-appeared on `shiny==1.0.0`

Looks like some work in the [mypy codebase](https://github.com/python/mypy/issues/13177) has been done to try ignore the error above, but it's currently still not fixed.

Currently one way to check for this specific issue would be to run: `grep -R '^# \+type:' shiny`
@gabrielecalvo Do you know which lines in the Shiny code base cause the problems with mypy?
yep, it's the same one as before actually..

```bash
$ mypy app.py

.venv\Lib\site-packages\shiny\render\_data_frame_utils\_selection.py:154: error: invalid syntax  [syntax]
Found 1 error in 1 file (errors prevented further checking)
```

(using current latest versions mypy==1.11.1 and shiny==1.0.0)

Fix patch:
diff --git a/Makefile b/Makefile
index 22a67d7cd..cc6eb67f2 100644
--- a/Makefile
+++ b/Makefile
@@ -225,6 +225,14 @@ ci-install-rsconnect: FORCE
 	uv pip install "rsconnect-python @ git+https://github.com/rstudio/rsconnect-python.git"
 
 
+# This is just to check if mypy can run for other users.
+# Not added to `make check` or `make check-fix` as all lint errors are supporessed (as we use pyright).
+ci-check-mypy-can-run: FORCE
+	@echo "-------- Checking types with mypy -----------"
+	uv pip install mypy
+	mypy shiny
+
+
 # ## If caching is ever used, we could run:
 # install-deps: FORCE ## install latest dependencies
 # 	pip install --editable ".[dev,test]" --upgrade --upgrade-strategy eager
diff --git a/pyproject.toml b/pyproject.toml
index ca94dbb50..079839741 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -144,3 +144,10 @@ ignore = ["E302", "E501", "F403", "F405", "W503", "E203", "E701", "E704"]
 [tool.isort]
 profile = "black"
 skip = ["__init__.py", "typings/", "_dev/", ".venv", "venv", ".tox", "build"]
+
+[tool.mypy]
+# The goal of our usage of mypy is to make to sure mypy can run, not that it catches any errors (we use pyright to find our errors).
+# Therefore, ignore_errors but do not ignore runtime errors while checking
+# Note: This setting can not be done via CLI and must be set within a config
+ignore_errors = true
+exclude = ["shiny/api-examples", "shiny/templates"]
diff --git a/shiny/render/_data_frame_utils/_selection.py b/shiny/render/_data_frame_utils/_selection.py
index fb86a3175..52125f5b6 100644
--- a/shiny/render/_data_frame_utils/_selection.py
+++ b/shiny/render/_data_frame_utils/_selection.py
@@ -150,8 +150,8 @@ def _has_rect(self) -> bool:
 # Should only contain a single selection area
 
 # Do not include `BrowserCellSelectionAll` as it should be represented by a row, column, or region with appropriate values.
-# class BrowserCellSelectionAll(TypedDict):
-#    type: Literal["all"]
+# # class BrowserCellSelectionAll(TypedDict):
+# #    type: Literal["all"]
 
 
 class BrowserCellSelectionNone(TypedDict):


Test patch:
diff --git a/.github/workflows/pytest.yaml b/.github/workflows/pytest.yaml
index 7fd04a9e9..6775e2f21 100644
--- a/.github/workflows/pytest.yaml
+++ b/.github/workflows/pytest.yaml
@@ -58,6 +58,11 @@ jobs:
         run: |
           make check-format
 
+      - name: Verify code can run with mypy (not Windows)
+        if: steps.install.outcome == 'success' && (success() || failure()) && matrix.os != 'windows-latest'
+        run: |
+          make ci-check-mypy-can-run
+
   pypi:
     name: "Deploy to PyPI"
     runs-on: ubuntu-latest