Re: [PATCH v2 4/7] scripts: generate_rust_analyzer.py: add type hints

[Trevor Gross]

On Tue, Mar 11, 2025 at 9:18 PM 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 `uv tool run mypy --strict scripts/generate_rust_analyzer.py` to
> verify.

>From the discussion, it may be better to instead mention the direct
invocation (without uv).

Could you also mention the target min version? Since apparently the
kernel has a spread. It looks like maybe 3.8 based on what is used
here.

> 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.
>
> 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 | 130 ++++++++++++++++++++++++++++----------
>  1 file changed, 96 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
> index 7e78b926e61f..c73ea8d116a4 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
> +import typing as T

Nit: is there any need to keep everything namespaced? I think it
should be fine to import `Iterable` `TypedDict` etc directly since
they aren't confusable.

Same for `pathlib.Path` since there is no other `Path` (some of that
is preexisting).

> +    def append_proc_macro_crate(
> +        display_name: str,
> +        root_module: pathlib.Path,
> +        deps: list[str],
> +        cfg: list[str] = [],
> +    ) -> None:
> +        append_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", "-"],

Nit, may as well use this opportunity to wrap the line.

> +            stdin=subprocess.DEVNULL,
> +        ).decode('utf-8').strip()
> +        crate: ProcMacroCrate = {
> +            **crates[-1],
> +            "is_proc_macro": True,
> +            "proc_macro_dylib_path": f"{objtree}/rust/{proc_macro_dylib_name}",
>          }
> -        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}"
> -        crates_indexes[display_name] = len(crates)
> -        crates.append(crate)
> +        crates[-1] = crate

The unpacking is a bit confusing here, can `crates[-1]` just be set
rather than duplicating and replacing it?

Maybe the body of `append_crate` should be `build_crate(...) -> Crate`
(which could then be a top-level function), then `append_crate`,
`append_crate_with_generated`, etc call that and handle modification /
appending themselves.

> +        crate: CrateWithGenerated = {
> +            **crates[-1],
> +            "source": {
> +                "include_dirs": [
> +                    str(srctree / "rust" / display_name),
> +                    str(objtree / "rust")
> +                ],
> +                "exclude_dirs": [],
> +            }
>          }
> +        crates[-1] = crate

Same note as above regarding rebuilding the last item.

- Trevor