Re: [PATCH v4 05/11] scripts: generate_rust_analyzer.py: add type hints

[Tamir Duberstein]

On Tue, Mar 25, 2025 at 9:38 AM Daniel Almeida
<daniel.almeida@xxxxxxxxxxxxx> wrote:
>
> Hi Tamir,
>
> > On 22 Mar 2025, at 10:23, Tamir Duberstein <tamird@xxxxxxxxx> wrote:
> >
> > Python type hints allow static analysis tools like mypy to detect type
> > errors during development, improving the developer experience.
> >
> > Python type hints have been present in the kernel since 2019 at the
> > latest; see commit 6ebf5866f2e8 ("kunit: tool: add Python wrappers for
> > running KUnit tests").
> >
> > Run `mypy --strict scripts/generate_rust_analyzer.py --python-version
> > 3.8` to verify. Note that `mypy` no longer supports python < 3.8.
> >
> > This removes `"is_proc_macro": false` from `rust-project.json` in
> > exchange for stricter types. This field is interpreted as false if
> > absent[1] so this doesn't change the behavior of rust-analyzer.
>
> Can this be a separate patch? Not sure how this is related to Python type
> hints, but it makes the current patch harder to review.

Yeah, I'll pop it out into another patch.

> >
> > Link: https://github.com/rust-lang/rust-analyzer/blob/8d01570b5e812a49daa1f08404269f6ea5dd73a1/crates/project-model/src/project_json.rs#L372-L373 [1]
> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
> > ---
> > scripts/generate_rust_analyzer.py | 172 +++++++++++++++++++++++++-------------
> > 1 file changed, 112 insertions(+), 60 deletions(-)
> >
> > diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
> > index 03f55cce673c..0772ea309f94 100755
> > --- a/scripts/generate_rust_analyzer.py
> > +++ b/scripts/generate_rust_analyzer.py
> > @@ -10,8 +10,10 @@ import os
> > import pathlib
> > import subprocess
> > import sys
> > +from typing import Dict, Iterable, List, Literal, Optional, TypedDict
> >
> > -def args_crates_cfgs(cfgs):
> > +
> > +def args_crates_cfgs(cfgs: Iterable[str]) -> Dict[str, List[str]]:
> >     crates_cfgs = {}
> >     for cfg in cfgs:
> >         crate, vals = cfg.split("=", 1)
> > @@ -19,7 +21,45 @@ def args_crates_cfgs(cfgs):
> >
> >     return crates_cfgs
> >
> > -def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> > +
> > +class Dependency(TypedDict):
> > +    crate: int
> > +    name: str
> > +
> > +
> > +class Source(TypedDict):
> > +    include_dirs: List[str]
> > +    exclude_dirs: List[str]
> > +
> > +
> > +class Crate(TypedDict):
> > +    display_name: str
> > +    root_module: str
> > +    is_workspace_member: bool
> > +    deps: List[Dependency]
> > +    cfg: List[str]
> > +    edition: Literal["2021"]
> > +    env: Dict[str, str]
> > +
> > +
> > +# `NotRequired` fields on `Crate` would be better but `NotRequired` was added in 3.11.
> > +class ProcMacroCrate(Crate):
> > +    is_proc_macro: Literal[True]
> > +    proc_macro_dylib_path: Optional[str]  # `pathlib.Path` is not JSON serializable.
> > +
> > +
> > +# `NotRequired` fields on `Crate` would be better but `NotRequired` was added in 3.11.
> > +class CrateWithGenerated(Crate):
> > +    source: Optional[Source]
> > +
> > +
> > +def generate_crates(
> > +    srctree: pathlib.Path,
> > +    objtree: pathlib.Path,
> > +    sysroot_src: pathlib.Path,
> > +    external_src: pathlib.Path,
> > +    cfgs: List[str],
> > +) -> List[Crate]:
> >     # Generate the configuration list.
> >     cfg = []
> >     with open(objtree / "include" / "generated" / "rustc_cfg") as fd:
> > @@ -31,67 +71,75 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >     # Now fill the crates list -- dependencies need to come first.
> >     #
> >     # Avoid O(n^2) iterations by keeping a map of indexes.
> > -    crates = []
> > -    crates_indexes = {}
> > +    crates: List[Crate] = []
> > +    crates_indexes: Dict[str, int] = {}
> >     crates_cfgs = args_crates_cfgs(cfgs)
> >
> >     def build_crate(
> > -        display_name,
> > -        root_module,
> > -        deps,
> > -        cfg=[],
> > -        is_workspace_member=True,
> > -        is_proc_macro=False,
> > -    ):
> > -        crate = {
> > +        display_name: str,
> > +        root_module: pathlib.Path,
> > +        deps: List[str],
> > +        cfg: List[str] = [],
> > +        is_workspace_member: bool = True,
> > +    ) -> Crate:
> > +        return {
> >             "display_name": display_name,
> >             "root_module": str(root_module),
> >             "is_workspace_member": is_workspace_member,
> > -            "is_proc_macro": is_proc_macro,
> >             "deps": [{"crate": crates_indexes[dep], "name": dep} for dep in deps],
> >             "cfg": cfg,
> >             "edition": "2021",
> >             "env": {
> >                 "RUST_MODFILE": "This is only for rust-analyzer"
> > -            }
> > +            },
> >         }
> > -        if is_proc_macro:
> > -            proc_macro_dylib_name = subprocess.check_output(
> > -                [os.environ["RUSTC"], "--print", "file-names", "--crate-name", display_name, "--crate-type", "proc-macro", "-"],
> > -                stdin=subprocess.DEVNULL,
> > -            ).decode('utf-8').strip()
> > -            crate["proc_macro_dylib_path"] = f"{objtree}/rust/{proc_macro_dylib_name}"
> > -        return crate
> > -
> > -    def register_crate(crate):
> > +
> > +    def register_crate(crate: Crate) -> None:
> >         crates_indexes[crate["display_name"]] = len(crates)
> >         crates.append(crate)
> >
> >     def append_crate(
> > -        display_name,
> > -        root_module,
> > -        deps,
> > -        cfg=[],
> > -        is_workspace_member=True,
> > -        is_proc_macro=False,
> > -    ):
> > +        display_name: str,
> > +        root_module: pathlib.Path,
> > +        deps: List[str],
> > +        cfg: List[str] = [],
> > +        is_workspace_member: bool = True,
> > +    ) -> None:
> >         register_crate(
> > -            build_crate(
> > -                display_name, root_module, deps, cfg, is_workspace_member, is_proc_macro
> > -            )
> > +            build_crate(display_name, root_module, deps, cfg, is_workspace_member)
> >         )
> >
> > +    def append_proc_macro_crate(
> > +        display_name: str,
> > +        root_module: pathlib.Path,
> > +        deps: List[str],
> > +        cfg: List[str] = [],
> > +    ) -> None:
> > +        crate = build_crate(display_name, root_module, deps, cfg)
> > +        proc_macro_dylib_name = subprocess.check_output(
> > +            [os.environ["RUSTC"], "--print", "file-names", "--crate-name", display_name, "--crate-type", "proc-macro", "-"],
> > +            stdin=subprocess.DEVNULL,
> > +        ).decode('utf-8').strip()
> > +        proc_macro_crate: ProcMacroCrate = {
> > +            **crate,
> > +            "is_proc_macro": True,
> > +            "proc_macro_dylib_path": f"{objtree}/rust/{proc_macro_dylib_name}",
> > +        }
> > +        register_crate(proc_macro_crate)
> > +
>
> Same here. Is this supposed to be related to Python type hints somehow?

Same here - moved into a separate patch.

> >     def append_sysroot_crate(
> > -        display_name,
> > -        deps,
> > -        cfg=[],
> > -    ):
> > -        append_crate(
> > -            display_name,
> > -            sysroot_src / display_name / "src" / "lib.rs",
> > -            deps,
> > -            cfg,
> > -            is_workspace_member=False,
> > +        display_name: str,
> > +        deps: List[str],
> > +        cfg: List[str] = [],
> > +    ) -> None:
> > +        register_crate(
>
> Why is register_crate here now? Maybe this change belongs to the preceding patch?

Good spot, this didn't need to change. Reverted in this patch.

>
> > +            build_crate(
> > +                display_name,
> > +                sysroot_src / display_name / "src" / "lib.rs",
> > +                deps,
> > +                cfg,
> > +                is_workspace_member=False,
> > +            )
> >         )
> >
> >     # NB: sysroot crates reexport items from one another so setting up our transitive dependencies
> > @@ -108,11 +156,10 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >         [],
> >     )
> >
> > -    append_crate(
> > +    append_proc_macro_crate(
> >         "macros",
> >         srctree / "rust" / "macros" / "lib.rs",
> >         ["std", "proc_macro"],
> > -        is_proc_macro=True,
> >     )
> >
> >     append_crate(
> > @@ -121,12 +168,11 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >         ["core", "compiler_builtins"],
> >     )
> >
> > -    append_crate(
> > +    append_proc_macro_crate(
> >         "pin_init_internal",
> >         srctree / "rust" / "pin-init" / "internal" / "src" / "lib.rs",
> >         [],
> >         cfg=["kernel"],
> > -        is_proc_macro=True,
> >     )
> >
> >     append_crate(
> > @@ -137,9 +183,9 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >     )
> >
> >     def append_crate_with_generated(
> > -        display_name,
> > -        deps,
> > -    ):
> > +        display_name: str,
> > +        deps: List[str],
> > +    ) -> None:
> >         crate = build_crate(
> >             display_name,
> >             srctree / "rust" / display_name / "lib.rs",
> > @@ -147,20 +193,23 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >             cfg=cfg,
> >         )
> >         crate["env"]["OBJTREE"] = str(objtree.resolve(True))
> > -        crate["source"] = {
> > -            "include_dirs": [
> > -                str(srctree / "rust" / display_name),
> > -                str(objtree / "rust")
> > -            ],
> > -            "exclude_dirs": [],
> > +        crate_with_generated: CrateWithGenerated = {
> > +            **crate,
> > +            "source": {
> > +                "include_dirs": [
> > +                    str(srctree / "rust" / display_name),
> > +                    str(objtree / "rust")
> > +                ],
> > +                "exclude_dirs": [],
> > +            }
> >         }
> > -        register_crate(crate)
> > +        register_crate(crate_with_generated)
> >
> >     append_crate_with_generated("bindings", ["core"])
> >     append_crate_with_generated("uapi", ["core"])
> >     append_crate_with_generated("kernel", ["core", "macros", "build_error", "bindings", "pin_init", "uapi"])
> >
> > -    def is_root_crate(build_file, target):
> > +    def is_root_crate(build_file: pathlib.Path, target: str) -> bool:
> >         try:
> >             return f"{target}.o" in open(build_file).read()
> >         except FileNotFoundError:
> > @@ -169,7 +218,9 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >     # Then, the rest outside of `rust/`.
> >     #
> >     # We explicitly mention the top-level folders we want to cover.
> > -    extra_dirs = map(lambda dir: srctree / dir, ("samples", "drivers"))
> > +    extra_dirs: Iterable[pathlib.Path] = map(
> > +        lambda dir: srctree / dir, ("samples", "drivers")
> > +    )
> >     if external_src is not None:
> >         extra_dirs = [external_src]
> >     for folder in extra_dirs:
> > @@ -192,7 +243,8 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
> >
> >     return crates
> >
> > -def main():
> > +
>
> What is this extra blank line for? Automatically generated by a formatter?

Yeah, this comes from PEP 8. Moved to "scripts:
generate_rust_analyzer.py: add missing whitespace".

>
> > +def main() -> None:
> >     parser = argparse.ArgumentParser()
> >     parser.add_argument("--verbose", "-v", action="store_true")
> >     parser.add_argument("--cfgs", action="append", default=[])
> >
> > --
> > 2.48.1
> >
> >
>
> Other than what I said above, the type hints themselves are fine.
>
> — Daniel

Thanks for the reviews!