Skip to content

Rewrite how OneShotCfg.build_modes gets constructed#158

Open
rswarbrick wants to merge 1 commit intolowRISC:masterfrom
rswarbrick:one-shot-build-modes
Open

Rewrite how OneShotCfg.build_modes gets constructed#158
rswarbrick wants to merge 1 commit intolowRISC:masterfrom
rswarbrick:one-shot-build-modes

Conversation

@rswarbrick
Copy link
Copy Markdown
Contributor

This is a bit tricky because self.build_modes comes from parsing an hjson dictionary and is actually a list of dictionaries that describe build modes.

These get parsed and converted to BuildMode objects by Mode.create_modes.

I've written a rather silly loose check that makes sure the types look reasonable, which is enough to let us convince Pyright to allow the code to claim the correct eventual types.

This commit also fixes an incorrect type in the second argument to create_modes.

This is a bit tricky because self.build_modes comes from parsing an
hjson dictionary and is actually a list of dictionaries that describe
build modes.

These get parsed and converted to BuildMode objects by
Mode.create_modes.

I've written a rather silly loose check that makes sure the types look
reasonable, which is enough to let us convince Pyright to allow the
code to claim the correct eventual types.

This commit also fixes an incorrect type in the second argument to
create_modes.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

mode_dicts.append(mode_dict)

base_modes: Sequence[Mode] = Mode.create_modes(BuildMode, mode_dicts)
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

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

Not having looked much into the original code for Mode, this seems like the ideal use case for bounded generics. Also, looking at the the method, it should just be a @classmethod, rather than a @staticmethod where we pass in the class. So, something like:

from typing import Type, TypeVar

T = TypeVar("T", bound="Mode")

class Mode:

    @classmethod
    def create_modes(cls: Type[T], mdicts: Sequence[Mapping[str, Any]]) -> list[T]:

This code would then just call e.g.

    base_modes: Sequence[BuildMode] = BuildMode.create_modes(mode_dicts)

with no need for a runtime check on its type.


@staticmethod
def create_modes(mode_type: "type[Mode]", mdicts: Mapping) -> Sequence["Mode"]:
def create_modes(mode_type: "type[Mode]", mdicts: Sequence[Mapping]) -> Sequence["Mode"]:
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

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

Nit:

Suggested change
def create_modes(mode_type: "type[Mode]", mdicts: Sequence[Mapping]) -> Sequence["Mode"]:
def create_modes(mode_type: "type[Mode]", mdicts: Iterable[Mapping]) -> Sequence["Mode"]:

Aside: I know these are problems in the original code, but there are a couple of parts that I dislike and so I wanted to mention. Feel free to address them or leave them.

  • Using the generic collection type Mapping is implicitly just Mapping[Any, Any] which disregards all type checks of the keys and values. Since it's loaded JSON the value is indeed Any, but the keys should at least be strings so this is better to explicitly be Iterable[Mapping[str, Any]].
  • I would prefer returning a list["Mode"] here. Since we know we are returning a list, this makes mutability a part of the contract, and stops users needing to copy into another list to mutate it. I see the argument for keeping a more generic interface, but practically this is not very useful and a list is more ergonomic.

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