Skip to content

fix: treat basedpyright warnings as errors and clear the backlog - #792

Merged
MaximilianSoerenPollak merged 1 commit into
eclipse-score:mainfrom
SebStefenel:fix/753-basedpyright-warnings
Sep 2, 2026
Merged

MaximilianSoerenPollak merged 1 commit into
eclipse-score:mainfrom
SebStefenel:fix/753-basedpyright-warnings

Conversation

@SebStefenel

@SebStefenel SebStefenel commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

📌 Description

Fixes #753.

Root cause

basedpyright exits 0 when a run reports only warnings. Every rule in
[tool.pyright] is deliberately configured as "warning", so the pre-commit
hook invoked the checker, saw a non-empty warning list, and still passed. That
is how 29 warnings accumulated on main.

Setting failOnWarnings = true makes any warning produce a non-zero exit code.
The rules stay declared as "warning" on purpose — they should still render as
warnings in the IDE rather than as hard errors. Only the CLI exit code changes,
which is exactly what gates pre-commit and CI.

Warning fixes (29 → 0)

All of them were fixed by adding annotations or narrowing types. No rule was
relaxed or disabled
, and no # type: ignore was used to paper over a real
typing problem.

  • Sphinx event handlers — annotate_testcase_results and
    use_document_source_as_plantuml_cwd had an untyped doctree parameter.
    This was the highest-leverage fix: an untyped handler also made its imports
    partially unknown in other modules, so three warnings collapsed into one
    annotation.
  • Decoded JSON/TOML — narrowed with the cast("dict[str, object]", ...)
    idiom already established in src/tests/docs_bzl/helpers.py, applied in the
    cross-module compatibility reporter and the module verification report tool.
  • monkeypatch parameters — annotated as pytest.MonkeyPatch, matching
    the existing convention in score_source_code_linker/tests.
  • White-box test imports — the deliberate imports of private helpers are
    marked # pyright: ignore[reportPrivateUsage] - white-box unit test, the
    same form already used in src/incremental_dirty_build_test.py.

Hook scope

BasedPyright always checks the whole project, so it does not take filenames.
It previously ran on every commit, including docs-only and workflow-only
ones. A files: filter now limits this slow hook to Python sources and its own
configuration.

🚨 Impact Analysis

No runtime behaviour changes. The production changes are type annotations and
cast(...) calls, which are erased at runtime, plus one control-flow rewrite
in _flatten_needs/_link_ids that was verified to be behaviour-preserving
(see below). The remaining changes are tooling configuration.

  • This change does not violate any tool requirements and is covered by existing tool requirements
  • This change does not violate any design decisions
  • Otherwise I have created a ticket for new tool qualification

✅ Checklist

  • Added/updated documentation for new or changed features
    — no user-facing documentation needed changing: docs/ does not describe
    the pre-commit or type-checking setup, and the README's pre-commit
    section covers installation only, which is unaffected. The why is
    documented as comments at both changed configuration sites, since the
    non-obvious part is why warnings must fail the CLI while remaining
    warnings in the IDE.
  • Added/updated tests to cover the changes
    — no new tests. Nothing changed at runtime: the edits are type
    annotations and cast(...) calls, which are erased at runtime. The one
    control-flow rewrite is differential-tested against the original logic
    (below) and covered by the existing suite. Enforcement comes from
    failOnWarnings itself, which the pre-commit run in CI already exercises.
  • Followed project coding standards and guidelines

Verification

Verified against a real Bazel-provisioned venv
(bazel run //:ide_support, basedpyright 1.39.10, Python 3.12.12) — not an
approximation:

basedpyright result exit code
main (before) 0 errors, 29 warnings 0 ← the bug
this PR 0 errors, 0 warnings 0
this PR + a deliberately introduced warning 0 errors, 2 warnings 1 ← gate works

The middle row is the point: on main the checker reported 29 warnings and
still exited 0, so the pre-commit hook passed.

Also:

  • ruff check and ruff format --check → clean.
  • pytest src/extensions tools → 283 passed, 2 failed. Both failures
    reproduce identically on unmodified main (verified by checking it out) and
    are unrelated to this PR; one of them additionally needs bazel on PATH,
    which it has in CI.
  • The rewrites of _flatten_needs, _link_ids and
    CompatibilityReporter.from_manifest were differential-tested against the
    original logic across edge cases (non-dict entries, non-str mount_at,
    malformed JSON, missing keys, non-list values) — identical output, including
    error messages.

basedpyright exits 0 when a run reports only warnings, so the
pre-commit hook passed even though the repository had accumulated 29
warnings. Set `failOnWarnings` so any warning fails the run.

The rules stay configured as "warning" on purpose: they should still
render as warnings in the IDE rather than as hard errors. Only the CLI
exit code changes, which is what gates pre-commit.

Fix the warnings this uncovered:

- Annotate the Sphinx event handlers `annotate_testcase_results` and
  `use_document_source_as_plantuml_cwd`, whose untyped `doctree`
  parameter also made every downstream import partially unknown.
- Narrow decoded JSON/TOML with the `cast("dict[str, object]", ...)`
  idiom already used in `src/tests/docs_bzl/helpers.py`, in the
  cross-module compatibility reporter and the module verification
  report tool.
- Annotate `monkeypatch` parameters as `pytest.MonkeyPatch`.
- Mark the deliberate white-box imports of private helpers in tests
  with `pyright: ignore[reportPrivateUsage]`.

Also restrict the hook to Python sources and its own configuration.
It checks the whole project and is slow, so there is no reason to run
it for a docs-only or workflow-only commit.

Signed-off-by: SebStefenel <stefenel.sebastian@gmail.com>

@AlexanderLanin AlexanderLanin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this contribution! Please have a look at some comments I made.

Comment thread .github/workflows/_test.yml Outdated
- name: Setup venv
run: bazel run --lockfile_mode=error //:ide_support

# The pre-commit hook only protects contributors who installed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we already run pre-commit via on-pr.yml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed that. Dropped that commit.

Comment thread tools/tests/tooling_config_test.py Outdated
)

assert warning_rules, "expected some diagnostic rules to be set to 'warning'"
assert settings.get("failOnWarnings") is True, (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

testing a config file doesnt seem reasonable, as this simply duplicates the config flag into this test and now both places set/compare it to true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah fair, removed.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-792: https://eclipse-score.github.io/docs-as-code/pr-792/

@SebStefenel
SebStefenel force-pushed the fix/753-basedpyright-warnings branch from e22a65d to 1307514 Compare September 2, 2026 07:01

@AlexanderLanin AlexanderLanin left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@MaximilianSoerenPollak MaximilianSoerenPollak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@MaximilianSoerenPollak
MaximilianSoerenPollak merged commit abdda20 into eclipse-score:main Sep 2, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Bug: pre-commit / basedpyright config error

3 participants