Re: [PATCH v7 22/59] perf python: Add perf.pyi stubs file

sashiko-bot
2026-04-25T23:06:28Z

Sashiko AI review found 1 potential issue(s):
- [Low] Inaccurate and incomplete Python type stubs in `perf.pyi`
--

commit 123e8822bf0adadf53e06cddf8aa0079bfd967d6
Author: Ian Rogers <irogers@google.com>

perf python: Add perf.pyi stubs file

This commit adds Python type stubs for the perf module to improve IDE
support and static analysis.

> diff --git a/tools/perf/python/perf.pyi b/tools/perf/python/perf.pyi
> new file mode 100644
> index 000000000000..91e19704e595
> --- /dev/null
> +++ b/tools/perf/python/perf.pyi
> @@ -0,0 +1,581 @@
> +"""Type stubs for the perf Python module."""
> +from typing import Callable, Dict, List, Optional, Any, Iterator

Are there a few module-level items missing from this file? The perf.pmu
class and the tracepoint module method appear to be omitted.

[ ... ]

> +def metrics() -> List[Dict[str, str]]:
> + """Get a list of available metrics.
> +
> + Returns:
> + A list of dictionaries, each describing a metric.
> + """
> + ...

Is this return type fully accurate? The MetricGroup key can contain a list
of strings rather than just a string, which might cause static analysis to
flag valid iterations over metric groups as errors.

> +
> +def syscall_name(sc_id: int, *, elf_machine: Optional[int] = None) -> str:

Can syscall_name() return None if the syscall is not found? If so, should
the return type be Optional[str]?

[ ... ]

> +def parse_events(
> + event_string: str,
> + cpus: Optional[cpu_map] = None,
> + threads: Optional[Any] = None
> +) -> 'evlist':

Should the threads argument be typed as Optional[thread_map] rather than
Optional[Any]?

> + """Parse an event string and return an evlist.
> +
> + Args:
> + event_string: The event string (e.g., 'cycles,instructions').
> + cpus: Optional CPU map to bind events to.
> + threads: Optional thread map to bind events to.
> +
> + Returns:
> + An evlist containing the parsed events.
> + """
> + ...
> +
> +def parse_metrics(metrics_string: str) -> 'evlist':
> + """Parse a string of metrics or metric groups and return an evlist."""
> + ...

Does the C implementation of parse_metrics also accept pmu, cpus, and threads
arguments that might be missing from this stub?

[ ... ]

> +class counts_values:
> + """Raw counter values."""
> + val: int
> + ena: int
> + run: int

Are the id and lost attributes missing from this class?

[ ... ]

> +class evsel:
> + """Event selector, represents a single event being monitored."""
[ ... ]
> + def open(self) -> None:
> + """Open the event selector file descriptor table."""
> + ...

Can evsel.open() take optional arguments like cpus and threads in the C
extension?

[ ... ]

> +class sample_event:
> + """Represents a sample event from perf."""
> + evsel: evsel
> + sample_cpu: int
> + sample_time: int
> + sample_pid: int
> + type: int
> + brstack: Optional['branch_stack']
> + callchain: Optional['callchain']
> + def __getattr__(self, name: str) -> Any: ...

Should srccode and insn be explicitly defined as methods or attributes on
sample_event?

> +
> +class mmap_event:
> + """Represents a mmap event from perf."""
> + type: int
> + pid: int
> + tid: int
> + addr: int

Does the C extension use start rather than addr for mmap events?

Also, are the misc field and inherited sample_members expected to be present
here?

[ ... ]

> +class evlist:

Are the sequence methods __len__ and __getitem__ missing from the evlist
stub?

> + def __init__(self, cpus: cpu_map, threads: thread_map) -> None: ...
> + def open(self) -> None:
> + """Open the events in the list."""
> + ...
> + def close(self) -> None:
> + """Close the events in the list."""
> + ...
> + def mmap(self) -> None:
> + """Memory map the event buffers."""
> + ...

Does evlist.mmap() accept optional pages and overwrite arguments?

> + def poll(self, timeout: int) -> int:

Is timeout an optional argument for evlist.poll()?

[ ... ]

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260425224951.174663-1-irogers@google.com?part=22