| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ-ks9nOY7DaOEO856Xj3g0+54yh5CbVgH=94xV+wG7gs+cMWA@mail.gmail.com>
Date: Tue, 25 Mar 2025 12:47:38 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>, Boris-Chengbiao Zhou <bobo1239@....de>, Kees Cook <kees@...nel.org>,
Fiona Behrens <me@...enk.dev>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, Lukas Wirth <lukas.wirth@...rous-systems.com>
Subject: Re: [PATCH v4 05/11] scripts: generate_rust_analyzer.py: add type hints
On Tue, Mar 25, 2025 at 9:38 AM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Hi Tamir,
>
> > On 22 Mar 2025, at 10:23, Tamir Duberstein <tamird@...il.com> 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@...il.com>
> > ---
> > 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!
Powered by blists - more mailing lists