[Go to site: main page, start]

Closed Bug 2064223 Opened 1 month ago Closed 1 month ago

Add pyrefly type checking to moz-phab test suite

Categories

(Conduit :: moz-phab, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sheehan, Assigned: sheehan)

Details

(Keywords: leave-open)

Attachments

(30 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

We have type annotation enforcement via the ANN* rules in ruff check, however ruff doesn't check that type hints are correct. We should add pyrefly to the moz-phab dev dependencies and add a test that enforces type hints are correct through the repo.

check_output returned a list, a string or raw bytes depending on its split and
expect_binary flags, so callers had to work with the union of all three. Each
helper is now a shared implementation plus one function per return type --
check_output, check_output_text and check_output_binary, and the same shape for
git_out and the jj helpers -- so callers say which form they want.

hg_out returned a list, a string, raw bytes or nothing depending on its split,
expect_binary and capture flags. hg_run now holds the implementation, with
hg, hg_out, hg_out_text and hg_out_binary naming the form they return, and
hg_log/hg_log_text sharing the command they build.

split_lines returned a bytes | str list whichever it was given, which its only
caller then passed on as strings. Text and binary content now have a function each,
so the element type follows from the name.

The unannotated stubs read as taking and returning None, so every typed override
in Git, Mercurial and Jujutsu looked inconsistent with the base class.
commit_stack also names the single argument its callers pass.

_git_get_children returns the list of "parent child child" lines its docstring
describes, _cat_file and hg_cat return bytes, _get_parent always finds a node,
Git.commit takes the epoch apply_patch passes it, and hgclient only exists as
hglib.client.hgclient. Diff.set_change_kind compares its kind against single
letters rather than a Kind.

edit_revision accepts a revision id or a PHID and a tri-state wip because that
is what Commit carries, and its differential.revision.edit arguments hold more
than the transaction list they are seeded with. call returns whatever the method
it invoked returns, which is a raw string for differential.getrawdiff.

stop_logging iterates the list to remove and close them, and it holds both a
stream and a rotating file handler.

The docstring-only stubs silently returned None where they declare a bool, a
Diff, a node or a list of commits. Every backend implements these, except that
Git has no untracked, so that one keeps its previously implicit empty default.

args started out as None, so every self.args.<option> access in the backends
was an attribute lookup on None, and vcs was untyped for the same reason.
phab_vcs caches through a local so its str return is provable.

Both get_repo_head_branch implementations read the _phab_repo cache field
directly, which is None until something else populates it through the property. A
repository without a callsign reached repository.search with None, and
is_cinnabar_required only existed on the Git based backends even though
build_creation_method reads it from any repository.

repo was an untyped None, so every conduit.repo.<attr> read looked like an
attribute lookup on None. It is now a property backed by an optional field that
raises a clear error when the repository has not been set yet, and the tests reuse
Repository for their stand-in rather than a bare class.

get_revisions rebound its ids parameter from list[int] to list[str], both
lookups let an empty ids list past their argument check and then iterated None,
and edit_revision built its constraint in a dict it splatted. The retry loop
gains an explicit else, so the response is always bound before it is read.

Git._commit_tree needs parent, author_date and tree_hash as strings, which
the backends that support them always provide; those without them leave the field
empty, which reads the same as the old None at the one place that tests it. The
epoch is built as an int, since that is what Phabricator receives.

The hunk header and its lines were collected in an untyped dict and splatted into
Hunk(**hunk), and a header that failed to match raised AttributeError on None
from the four match.group calls that followed. The diff ids Phabricator returns
start empty rather than as None.

set_as_binary stores its bodies for upload, where they are hashed and base64
encoded, so they have to be bytes. Git._parse_diff_change and
Mercurial._get_file_meta each reused one variable for the raw blob and its
decoded text, and the git side could hand a decoded str to one side of a binary
change.

The dicts mixed filenames with the handler to call, so c["fn"] looked like it
could be either a string or a method. get_diff keeps the parent it just resolved
in a local, which is what the handlers need.

_get_successor declares a (rev, node) tuple and its caller unpacks one, but it
returned the list str.split produced. _get_config_options returns (name, value) pairs, so _get_extensions has been logging an empty list in safe mode,
and the revset, bookmark and obsstore paths are filled in before use.

commit_stack tracked whether it had started through the child list rather than the
first node, is_descendant and uplift_commits subscript the revset that
set_args fills in, and finalize checked out the branch without the guard
cleanup uses. rebase_commit takes the Commits the base class and its only
caller pass, and get_public_node drops a cache that _git_to_hg already provides.

Jujutsu has no git attribute, so reporting an invalid email raised
AttributeError instead of naming the address. apply_patch receives an optional
author and creation date, which patch leaves unset when a diff has no commit
attachment, and passed both straight into jj metaedit.

prompt bound options_map only when it had options, replace_reviewers rebound
its str parameter to a list of lines, and check_call_by_line read
process.stdout without stating that the pipe is always requested.

_build_request passed an unset bmo_url straight to urljoin, _sanitise_req
assumed the request always carries headers, and BMOAPIError was handed an
exception where it declares a string.

install_scripts is optional, and Path(None) would have raised TypeError during
a self-update check.

patch tested args.apply_to where the branches above had already decided whether
there is a base node to check. reorganise annotates the collections it builds,
collects the revision ids after the check that they are all present, and leaves text
unlinked for repositories with no Bugzilla, rather than linking to
None/show_bug.cgi.

Commit.rev_id is optional, so the AI review request list and its result lookup
were both typed as possibly holding None keys, and the uplift assessment URLs were
built from the tip commit whether or not it had an id.

The fakes stand in for parsed command line arguments, so they belong to the same
type as the real thing, as test_submit.py already did.

The fake stands in for a repository in local_uplift_if_possible and
update_commits_for_uplift, so it now subclasses Repository, seeds the _phid
cache the phid property reads, and returns the commits from uplift_commits.

test_get_diff_by_id drove the function through an args dict, so the "no diff" case
actually exercised the not-found branch; it now covers the three branches directly.
The reorganise tests pass a repository stand-in rather than None, and HTTPError
and MaxRetryError get the header and reason objects they declare.

test_is_descendant and test_patch asserted on the raised error after the call
that raises, so the assertions never executed, and both read the wrong attribute off
ExceptionInfo. The uplift and user data tests check their optional values before
using them, so a failure surfaces as the assertion rather than an AttributeError.

hglib spawns the command server from hglib.HGPATH, so pointing it at a
configured hg means setting that global. hglib ships no type information, so its
default value gives the global a literal type that no other path is assignable to,
which the cast states explicitly at the assignment.

pyrefly joins the development dependencies, pyrefly.toml configures it against
the oldest supported interpreter, and test_pyrefly fails on any type error that is
not recorded in pyrefly-baseline.json, which is empty now that the tree checks
clean. Regenerate the baseline with uv run pyrefly check --baseline pyrefly-baseline.json --update-baseline from the repository root, since it stores
repository-relative paths.

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/4791143f9f41f28881addb7996422872b5c37342
[main] vcs: name the command output helpers after what they return (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/30ab230df1d869c2d5e39547e128d638d2cf2e6b
[main] mercurial: name the hg output helpers after what they return (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/1d0e68bfab3efac62b35347cbc229e07ec5fb8cf
[main] helpers: split the text and binary line splitting (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/6ce48d22c96e75fe56b9a0d2156da6a172eb6525
[main] repository: annotate the base methods the backends override (Bug 2064223) r=shtrom

Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/e0ee61488e9f3306600854aa1a0e52546bfb0ef8
[main] vcs: correct the annotations that disagree with the code (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/eb654edf01dfeebc96aa3d64f206a209e23507a5
[main] conduit: correct the annotations on the revision helpers (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/2a6c4287af534543f0188114dc9e12a1d83721f7
[main] logger: annotate the list of installed handlers (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ea0dfdfa68b68ba23be81a88feba9c0a9e1e176a
[main] repository: raise NotImplementedError from the base stubs (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/9487a71c90d2b9c575dd1b7f51826daee3343a61
[main] repository: give the command line and VCS attributes real values (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/552fcb6e8ceb9fe5a82f410b036744109dc01126
[main] repository: cover the repository data lookups in the base class (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/b220c0c2f18d15411517ccfbc267de02bb49e08c
[main] conduit: hold the repository in a typed property (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/e3998f341024cb3e44235ffc7af5894fda6f98b6
[main] conduit: type the revision and diff lookups (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/b5feecd4e70aad566ea4b85a5115bb84bc656bfd
[main] commits: type the fields the backends always fill in (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/fad9a6e5bee495f2e9949e393a91ae6e2ef9db24
[main] diff: build hunks from typed values and reject bad headers (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ce797c44884a8b1559fddb7c182424a9a20ea3e6
[main] diff: keep binary file bodies as bytes (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/001a72ebbba0177853acd06b0a52a1d1af2d9c78
[main] mercurial: collect file changes in a frozen dataclass (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/9d040dfaa8db0a92f06ef5bcb537c322c2c3e8a9
[main] mercurial: fix the successor tuple and the state it tracks (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/da0e0b8745e42631191efc4bf27a999b76841e7e
[main] git: type the commit range, branch and rebase arguments (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/d75215e2ca7e90fa157d17b071777ed73e00d4f0
[main] jujutsu: fix the email error message and the patch metadata (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/58d8112d3afbd5d5c210ea4e14321b06cddbaffc
[main] helpers: initialise the values that only one branch assigns (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/564749319dc219e60d89baa9e7c969a57b8db37a
[main] bmo: handle a missing Bugzilla URL and header dict (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/f85bced2b21043cccb067ee071b828e7909a5756
[main] updater: fail clearly when the user scripts directory is unknown (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/8ea88b1e3bf8073949bc9515a9a0a67318fd75ae
[main] patch, reorganise: handle the optional values in the command flow (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/60fc6b72bc21a59a6ad0f1fe5d441b9b304e5d23
[main] submit, uplift: key the follow-up work on the revision ids (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/3e870c740848df671d79181c4294092d7be53e5f
[main] tests: base the fake Args classes on argparse.Namespace (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ca71841f45c4e7e8df9b0cba8710f2aa68dd6ca7
[main] tests: base the uplift fake repo on Repository (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/175427aadcbec9d51ff4d1338e16bd542ee8d123
[main] tests: pass the types the code under test declares (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/694f0b75308588acecceffc903c1294921339a6b
[main] tests: run the assertions that sit inside pytest.raises blocks (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/fcdc319440bf141f9fdfbb0ef80c2ab0ca0dbfcc
[main] mercurial: cast the hg path assigned to the hglib global (Bug 2064223) r=shtrom

Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/111628d7a2995af41fb71ac273cc73162e47b361
[main] tests: check types as part of the suite (Bug 2064223) r=shtrom

Status: REOPENED → RESOLVED
Closed: 1 month ago → 1 month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: