Skip to content

Add pyright for static type checking#17744

Merged
SaschaCowley merged 8 commits intomasterfrom
pyright
Mar 6, 2025
Merged

Add pyright for static type checking#17744
SaschaCowley merged 8 commits intomasterfrom
pyright

Conversation

@seanbudd
Copy link
Copy Markdown
Member

@seanbudd seanbudd commented Feb 26, 2025

Link to issue number:

Related to #17301

Summary of the issue:

NVDA has no type checking for python.
Python is a dynamically typed language meaning typing issues can occur at runtime.
Static analysers like mypy or pyright can be used to avoid typing errors.

Description of user facing changes

None

Description of development approach

pyright was chosen over mypy for performance reasons.
Additionally, pyright has better pre-commit integration, and is built-in to VS Code, which is used by most NVDA developers.

We are far from compliant with static type checking, so most rules are currently disabled.
The intention is to fix compliance and turn on rules over time.

Various trivial typing fixes have occurred, particularly:

  • removing unnecessary casts
  • fixing type hints
  • adding relevant type ignore comments
  • removing redundant type ignore comments
  • adding type stubs for gettext builtins and _buildVersion generated build vars
  • remove duplicate imports
  • adding __all__ to mark export for private classes
  • adding missing match cases

Testing strategy:

  • Smoke test NVDA
  • pyright passing
  • Automated testing passes

Known issues with pull request:

We are far from compliant with static type checking, so most rules are currently disabled.
The intention is to fix compliance and turn on rules over time.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd seanbudd requested a review from a team as a code owner February 26, 2025 03:21
@seanbudd seanbudd added api-breaking-change release/blocking this issue blocks the milestone release labels Feb 26, 2025
@seanbudd seanbudd added this to the 2025.1 milestone Feb 26, 2025
@seanbudd seanbudd requested a review from a team as a code owner February 26, 2025 04:55
@hwf1324
Copy link
Copy Markdown
Contributor

hwf1324 commented Feb 26, 2025

Can you clean up the other trailing spaces in requirements.txt by the way?

@seanbudd
Copy link
Copy Markdown
Member Author

@hwf1324 - this is probably better done as its own PR, it's relatively trivial

@seanbudd
Copy link
Copy Markdown
Member Author

Note: there's no inbuilt way of automatically generating type: ignore comments like we can with ruff and noqa comments.
A custom script could be written to take the JSON output of pyright, and add comments using the data.
It would be great to do this and open source it for others

Copy link
Copy Markdown
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, however I'm a little concerned about the inclusion of a bunch of internal classes in various modules' __all__.

Co-authored-by: Sean Budd <sean@nvaccess.org>
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit f2649a8e74

@seanbudd seanbudd requested a review from SaschaCowley March 4, 2025 01:24
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 445074b0b8

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 4, 2025
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit defc94e191

Copy link
Copy Markdown
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @seanbudd

@SaschaCowley SaschaCowley merged commit f3c7d28 into master Mar 6, 2025
6 checks passed
@SaschaCowley SaschaCowley deleted the pyright branch March 6, 2025 23:38
seanbudd pushed a commit that referenced this pull request Mar 26, 2025
Follow-up to 17744

Summary of the issue:
As pointed out in #17744 (comment), the type hint for the dict argument to baseObject.ScriptableType.__new__ is incorrectly [str, Any] since #17744 was merged.
@seanbudd seanbudd mentioned this pull request Oct 10, 2025
5 tasks
seanbudd added a commit that referenced this pull request Oct 14, 2025
Closes #17749
Summary of the issue:

#17744 introduced pyright using it's default server nodeenv.
The pyright python package recommends installing pyright[nodejs] for a faster and more reliable server.
Hwever we couldn't use it while NVDA was 32bit.
This is because the node wheel binaries required a 64bit environment.
Now that NVDA has migrated to 64bit, we should update the pyright package version to using nodejs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. release/blocking this issue blocks the milestone release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants