Expand mypy static type checking

[bradenmacdonald]

Description

The edx-platform codebase has mypy set up, but only a single django app (learning_sequences) is currently type checked.

This PR expands the type checking to include several django apps that I've developed:

• openedx/core/djangoapps/content_staging
• openedx/core/djangoapps/content_libraries
• openedx/core/djangoapps/xblock

The type checking found a couple actual bugs in the code, which I have now fixed. It also required a bunch of additional tweaks here are there to get the type checks passing.

Testing instructions

MyPy tests should be run by CI, so just confirm that they ran.

Deadline

None

Other information

Private ref MNG-3784

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

[bradenmacdonald]

Turns out this was defined twice in this file. The pylint warning was suppressed by an automatic lint amnesty line.

[bradenmacdonald]

It was unclear if this should be changed to allow None or if None values should be converted to "" when stored in this field, but there were clear examples if it being explicitly set to None in the tests, so I went with this.

https://github.com/openedx/edx-platform/blob/478c301df5ab0a58dad43cedfc05e4003828c2d3/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py#L55

[bradenmacdonald]

Until python typing supports intersections there is no way to tell the type system that we want to add a real_user attribute onto the User class, so I just ignored this line.

[bradenmacdonald]

This was a clear bug found by the type check. 🐛 str here was the global string type :/

[bradenmacdonald]

Not sure why it didn't like this line, but I'm planning to remove this filter implementation soon anyways. So just ignored it for now.

[bradenmacdonald]

I had to rename this variable because the f used as an iterator in this for loop is considered the same as the f used as the iterator in the second for loop in this function. I don't like how python does scoping.

[Agrendalath]

👍

• I tested this: checked that the tests are passing
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: n/a
• I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

[Agrendalath]

@bradenmacdonald, I left a few suggestions, but none are blockers. Thanks for implementing this - I really like that we're adding more typing to Open edX!

Btw. did you think about using pyright? We're running mypy only on a few packages in Open edX, so it could be nice to investigate it as an alternative. After doing this review, I ran it, and it detects more errors than mypy. E.g., it detects that this code can potentially raise AttributeErrors when user is None.

pyright openedx/core/djangoapps/xblock/runtime/runtime.py
/edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:113:38 - error: Cannot access member "id" for type "User"
    Member "id" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:143:49 - error: Cannot access member "get_site_root_url" for type "AppConfig"
    Member "get_site_root_url" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:177:13 - error: Argument of type "int | str" cannot be assigned to parameter "__value" of type "str" in funct
ion "__setitem__"
    Type "int | str" cannot be assigned to type "str"
      "int" is incompatible with "str" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:178:9 - error: Argument of type "dict[Any, Any]" cannot be assigned to parameter "__value" of type "str" in f
unction "__setitem__"
    "dict[Any, Any]" is incompatible with "str" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:187:26 - error: "is_anonymous" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:244:21 - error: Argument of type "None" cannot be assigned to parameter "__value" of type "FieldData" in func
tion "__setitem__"
    Type "None" cannot be assigned to type "FieldData" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:250:26 - error: "is_anonymous" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:259:41 - error: "is_staff" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:366:64 - error: Cannot access member "get_site_root_url" for type "AppConfig"
    Member "get_site_root_url" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:436:16 - error: Object of type "XBlockRuntime" is not callable (reportGeneralTypeIssues)
10 errors, 0 warnings, 0 informations

[bradenmacdonald]

@Agrendalath I thought about pyright and I tried it out, because it seems nicer and faster. But it doesn't have a plugin framework, so it doesn't support attrs which is important for most of these modules in edx-platform. Didn't you run into this issue? When I tried pyright, it gave a ton of errors related to the attrs data classes.

[Agrendalath]

@bradenmacdonald, good point - I didn't think about attrs. According to the attrs docs, we could use @attr.define instead of @attr.s, and then use explicit type annotations. Given that we have only 6 import attrs usages (excluding tests), it could be possible to change it.

Actually, the speed of both mypy and pyright seems to be comparable, but pyright is much more sensitive. I ran both on the whole edx-platform repo

• mypy -> time: 17.051; Found 460 errors in 207 files (checked 2469 source files)
• pyright --skipunannotated -> time: 17.654s; 789 errors, 33 warnings, 11784 informations
• pyright -> time: 33.477s; 8198 errors, 37 warnings, 0 informations

For mypy, I only changed the following in the mypy.ini:

files =
    lms,
    openedx,
    cms,
    xmodule

For pyright, I created the following pyproject.toml:

[tool.pyright]
exclude = ['**/migrations', '**/__pycache__', '**/tests']

[bradenmacdonald]

@Agrendalath Thanks, that seems doable. Another issue is Django though - while mypy has a plugin that works automatically together with django-stubs, for pyright you still have to add a lot of manual type annotations. https://github.com/sbdchd/django-types https://news.ycombinator.com/item?id=34223647 . Presumably same for DRF.

I suspect that many of the issues pyright is flagging are due to it not understanding django as fully?

[Agrendalath]

@bradenmacdonald, you're right - mypy with plugins works much better with Django. pyright has more false positives, and using django-types with it requires plenty of hacks to work properly.

We could try using mypy with check_untyped_defs = true to catch more errors (though still less than pyright).

[openedx-webhooks]

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.