Skip to content

Feature: more typing - #513

Draft
PigBrainOverflow wants to merge 9 commits into
UCSBarchlab:developmentfrom
PigBrainOverflow:feature/more-typing
Draft

Feature: more typing#513
PigBrainOverflow wants to merge 9 commits into
UCSBarchlab:developmentfrom
PigBrainOverflow:feature/more-typing

Conversation

@PigBrainOverflow

Copy link
Copy Markdown

#511

  • Fix the Register.next setter annotation to reflect that augmented assignment passes a Register._Next object back through the setter.
  • Change match_bitwidth's return type to Iterator[WireVector], matching its current generator-based implementation.
  • Change concat_list's parameter type from list[WireVectorLike] to Sequence[WireVectorLike].
  • New implementation of adder and fir.

The typing changes are annotation-only and do not change runtime behavior.

This PR focuses only on fixing the identified type annotations. I have not added repository-wide type-regression testing yet, since doing that robustly is a substantially larger piece of work.

A useful regression test would first require choosing a representative baseline and defining what it means for the typing state to become "no worse." This is not necessarily equivalent to comparing the number of diagnostics: a future change might eliminate several existing errors while introducing a smaller number of different errors, and it is not obvious whether that should be considered an improvement or a regression.

It would therefore be better to design the baseline and comparison policy separately, rather than introducing a fragile diagnostic-counting test as part of this PR.

@fdxmw

fdxmw commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for putting this together!

This PR focuses only on fixing the identified type annotations. I have not added repository-wide type-regression testing yet, since doing that robustly is a substantially larger piece of work.

I agree that regression testing should be added in a separate PR, and that defining exactly what "no worse" means is quite hard. Some thoughts:

  1. Having some tests is generally better than having no tests. I'd rather have some simple regression tests soon, than having perfect regression testing in the indefinite future :) I think I'd start with a short allow-list of examples, tests, etc that currently pass typechecks, and add checks to be sure those stay type-safe in the future.
  2. Type-regression testing on a type-unsafe codebase is a hard problem, but it's a meta-problem. I think our time and energy are better spent on the underlying problem of making PyRTL more type-safe, than finding the best solution to this meta-problem.

@fdxmw fdxmw 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.

There are also some formatting errors, you should be able to reproduce them with uv run just presubmit

Comment thread www/examples/example-fir.py Outdated
zs[i].next <<= zs[i - 1]
regs = [pyrtl.Register(rwidth) for _ in range(ntaps - 1)]
for i, reg in enumerate(regs):
reg.next <<= x if i == 0 else regs[i - 1]

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.

Hmm, I find this harder to read than the original. What do you think about something like this?

regs = [pyrtl.Register(rwidth) for _ in range(ntaps - 1)]
regs[0].next <<= x
for prev, curr in itertools.pairwise(regs):
    curr.next <<= prev
zs = [x, *regs]

@PigBrainOverflow PigBrainOverflow Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Cool! It's clearer. I will adopt it

@PigBrainOverflow
PigBrainOverflow marked this pull request as draft August 24, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants