Add type hints to edalize#525
Open
ThVerg wants to merge 12 commits into
Open
Conversation
Addresses olofk#478. * New module edalize/edam.py with TypedDicts modelling the EDAM dict (the FuseSoC <-> Edalize contract): Edam, File, Parameter, HookScript, Hooks, VpiModule, plus Literal aliases (ParamType, DataType, HookName). * PEP 561 marker (edalize/py.typed) so downstream packages see the annotations. * mypy configuration in pyproject.toml with lenient defaults for incremental adoption and stricter overrides on the three base classes and edam.py. * Added typing_extensions>=4.6 runtime dep for Python <3.11 (NotRequired). * Hand-annotated the three base classes: edatool.py, tools/edatool.py, flows/edaflow.py. * Annotated ~75 backends across edalize/, edalize/tools/, edalize/flows/. * Annotated utils.py, build_runners/make.py and the *_reporting.py modules. * Added 'from __future__ import annotations' to 79/83 files. * Added tests/test_type_hints_regressions.py with 274 hand-written tests that probe annotation-driven behaviour changes; all pass. Verified state: mypy: 0 errors across 83 source files pytest: 474 passing (200 upstream + 274 new regression tests)
* DataType now includes 'real' to mirror FuseSoC's CAPI2 schema. * Parameter.default widens to include float so real-valued defaults type-check. * Edam.name is Required (Edatool.__init__ indexes it unconditionally). * Add FuseSoC-emitted top-level keys: version, cores, dependencies, filters. * Replace ToolDoc = Dict[str, Any] with a proper TypedDict and a ToolDocEntry row TypedDict; unroll the dynamic-key get_doc loop so TypedDict literal-key access type-checks. * Annotate the six backends that compose ToolDoc from inline dict literals: apicula, gatemate, icestorm, mistral, oxide, symbiflow. mypy: 0 errors across 83 source files pytest: 474 passing
* self.edam: Edam | None -> Edam (narrowed after eda_api fallback, with a type: ignore where upstream test_empty_edam intentionally relies on TypeError raised by indexing None). * self.files: list[Any] -> list[EdamFile]. This would have caught the tools/vpr.py f.name typo at type-check time. * edalize.tools.edatool.setup() now also types self.files: list[EdamFile]. * flows.edaflow.Node: tool: str | None = None -> tool: str (required). tool.capitalize() is called unconditionally; the sole caller already passes a non-None string. Reordered parameters so tool comes before the mutable defaults. mypy: 0 errors pytest: 474 passing (200 upstream + 274 regression)
Codex review noted that several class-level mutable defaults and silent-no-op changes I added during the first typing pass amount to API drift, not annotations. Revert each to match pristine behaviour while keeping the type information that drove the changes: * Edatool.argtypes: list[str] (annotation only, no shared list default). * Edatool.tool_options: dict[str, Any] (annotation only). * Nextpnr.flow_config: dict[str, str] (annotation only). * EdaCommands.default_target: str (annotation only). * _apply_parameters no longer guards against None; signature is now RunArgs (the caller is expected to never pass None, matching main). * Edaflow.configure_flow is hidden behind 'if TYPE_CHECKING:' so the attribute is documented for static checkers but does not exist at runtime; subclasses that forget to override still hit AttributeError. Regression tests rewritten to assert the restored pristine behaviour (AttributeError instead of NotImplementedError / clean RuntimeError / empty default). mypy: 0 errors pytest: 474 passing
Codex pointed out that FlowGraph.fromdict(d: dict[str, Any]) was loose
enough to let the 'ftdo' misspelling in flows/vpr.py slip past static
checks. Add a FlowNodeSpec TypedDict modelling the {deps, fdto, tool}
keys and narrow:
* FlowGraph.fromdict signature
* the _flow class attribute on apicula / icestorm / trellis
* the local flow dict in apicula, generic, gls, icestorm, trellis,
vivado, vpr
Apply the 'ftdo' -> 'fdto' fix in flows/vpr.py to match the standalone
upstream PR olofk#519 so this branch type-checks; will rebase cleanly once
the upstream PR lands.
mypy: 0 errors
pytest: 474 passing
Codex pointed out that the override on the four 'strict' modules was just two flags, not the full strict bundle. Fix: * edalize.edam, edalize.tools.edatool, edalize.flows.edaflow now pass mypy --strict cleanly. The override sets every strict-flag explicitly. * edalize.edatool is kept on a lighter override (disallow_incomplete_defs + warn_return_any) because making it fully strict requires changing its constructor signature (Edam | None, str | None), which upstream test_empty_edam pins. ~22 strict errors remain there, tracked as the next module to tighten. * Fix three real strict-mode issues found along the way: - subprocess_run_3_9 was missing param/return annotations. - Asserted Popen.poll() returncode after the process has exited. - Removed two unused-type-ignore comments revealed by --strict. mypy (project config): 0 errors mypy --strict (3 modules): 0 errors pytest: 474 passing
* Apply 5 upstream bug fixes locally (mirrors the standalone PRs so the
branch is self-contained and free of pre-existing-bug type-ignores):
- flows/f4pga.py: return FlowGraph.fromdict({...}), not a list.
- tools/surelog.py: append to verilog_defines, not verilog_params.
- tools/vivado.py: build() returns the local args list.
- tools/vpr.py: build() returns [], not undefined self.args.
- openfpga.py: super().__init__ forwards eda_api before verbose.
* Tighten three TypedDicts that were total=False where the keys are
indexed unconditionally:
- HookScript.name / cmd: Required
- ToolDocEntry.name / type / desc: Required
- ToolDoc.description: Required
Six legacy backends that build an accumulator dict now include a
placeholder description (the final return statement overrides it).
* Edaflow.configure_flow: bring back the runtime
raise NotImplementedError instead of hiding behind TYPE_CHECKING.
The behaviour is a strict upgrade from pristine's AttributeError.
* tools/nextpnr.py: type output_files: list[EdamFile] and drop the
type-ignore on self.edam['files'] += output_files.
* Documented note on Node default-arg anti-pattern: upstream golden
files actually pin the leaky shared-state behaviour, so the fix
belongs in a separate cleanup PR.
mypy: 0 errors
mypy --strict on edam, tools.edatool, flows.edaflow: 0 errors
pytest: 474 passing
Address each of the six error groups Codex identified, bringing
edalize.edatool to mypy --strict cleanliness:
Group 1: assert retcode after Popen.poll() in subprocess_run_3_9.
Group 2: explicit 'if edam is None: raise TypeError(...)' after the
eda_api fallback. Preserves upstream test_empty_edam while
narrowing self.edam to non-Optional.
Group 3: keep work_root: str | None at the API boundary (upstream
tests construct backends without one) but route the strict
errors via type:ignore on the env-dict assignment and assert
in render_template / _run_tool.
Group 4: assert __spec__ is not None and __spec__.parent is not None
around the importlib metadata lookup.
Group 5: guard every get_doc(0) call site:
- _extend_options returns early if get_doc returns None.
- parse_args / _apply_parameters fall back to an empty
ToolDoc shape.
- gen_tool_docs skips backends whose get_doc returns None.
Group 6: run_pre lets parsed_args flow as Optional with a single
targeted type:ignore — the existing AttributeError path on
run_pre(None) is intentional.
Promoted edalize.edatool into the fully-strict overrides block, so
all four base modules now require --strict cleanliness on each commit.
mypy --strict on edalize.edam, edalize.edatool, edalize.tools.edatool,
edalize.flows.edaflow: 0 errors
mypy (project config): 0 errors across 83 source files
pytest: 474 passing
* Remove allow_redefinition = true from the project mypy config. It
was masking real strict-mode errors in edatool.py and tools/vpr.py.
Fixed the three uncovered redefinitions explicitly:
- edatool.parse_args: annotate the parse_args 'default' local as Any.
- tools/vpr.setup: type 'depends' as str | list[str], assert before
passing as a single dep.
- ise_reporting._parse_twr_stats: annotate pp_units as
pp.ParserElement so the | reassignment widens cleanly.
* Restore Node(__init__) positional ordering to pristine
(name, deps=[], fdto={}, tool=None) so positional callers don't break.
Validate tool with an assert inside the body; pristine crashes on
None at .capitalize() anyway, this gives a clearer message.
* Apply the sandpipersaas depends=[] fix locally (mirrors PR olofk#520) and
drop the silenced type:ignore.
* Apply the flows/vpr.py build_tool_graph dead-method removal locally
(the local-only branch that I held back at the user's request) and
drop the silenced type:ignore.
mypy: 0 errors
mypy --strict on the four strict modules: 0 errors
pytest: 474 passing
Schema: * File.name is now Required[str]; the other keys are NotRequired. FuseSoC always emits 'name' and every backend indexes f['name'] unconditionally. * Parameter.datatype and paramtype are Required (CAPI2 rejects either missing); default and description stay NotRequired. * VpiModule.name, src_files, include_dirs, libs are all Required — FuseSoC emits all four when it emits a VPI entry and several backends index them directly. Failure-mode preservation (per Codex round 3): * _extend_options now raises RuntimeError when get_doc(0) returns None instead of silently dropping the contribution. Matches pristine's loud failure (it crashed at help['members'] in that case). * parse_args / _apply_parameters raise RuntimeError on the same None case instead of treating it as 'no backend args'. Ignore cleanup: * verilator.py: drop the unused type:ignore[operator] on the toplevel concatenation (self.toplevel: Any already suppresses the error). * flows/edaflow.py: replace the hooks-assignment type:ignore with a proper cast to dict[HookName, list[HookScript]], and type add_scripts(hook_name: HookName). * edatool.py run_pre: replace the type:ignore on _apply_parameters with cast(RunArgs, parsed_args). Same AttributeError on None path. mypy: 0 errors mypy --strict on the four base modules: 0 errors pytest: 474 passing
* gen_tool_docs now raises RuntimeError when get_doc(0) returns None, matching the loud-fail pattern used by _extend_options / parse_args / _apply_parameters. The previous 'silently skip the section' was the one remaining silent-drop Codex flagged. * Two upstream test fixtures used 'tags': 'simulation' (string) where File.tags is now typed list[str]. Runtime substring-matching tolerated the bug; the tests still passed only because '"simulation" in "simulation"' is True. Fix to ['simulation'] in tests/test_tool_yosys.py and tests/test_tool_vivado.py to match the schema and the obvious intent. * tools/gowin.py: type _handle_src / _handle_tcl / src_file_filter / _append_library with EdamFile instead of Any. Removes the last two 'returning Any as str' strict errors in edalize.tools. * sandpipersaas.py: drop the pre-existing no-op .format(...) call on the empty OUTPUTDIR line. The format had no placeholder so the argument was always silently dropped; the rewrite removes the type:ignore and the dead code. Remaining type:ignore comments are down to two, both pinned by unreachable Python <3.7 dead code (the TimeoutExpired/_mswindows fallback in flows/edaflow.py and edalize/edatool.py). mypy: 0 errors mypy --strict on edalize.edam, edalize.edatool, edalize.tools.edatool, edalize.flows.edaflow: 0 errors pytest: 474 passing
The subprocess_run_3_9 fallback is gated on sys.version_info < (3, 8), so it is unreachable on Python >= 3.8, not >= 3.7 as the comment said. Either way the branch is dead under edalize's requires-python >= 3.9. Spotted by Codex.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds type hints across the edalize codebase. Closes #478.