Skip to content

Lint: Use Ruff to format Tools/jit #133123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Lint: Use Ruff to format Tools/jit #133123

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65535ee
Use Ruff for all formatting in pre-commit
AA-Turner Apr 29, 2025
34c1850
Merge branch 'main' into ruff-format
AA-Turner May 1, 2025
ba8e4bf
Add Tools/jit/.ruff.toml
AA-Turner May 1, 2025
42ce014
PEP 8 (keep to 80 chars)
AA-Turner May 1, 2025
e36f747
Revert "PEP 8 (keep to 80 chars)"
AA-Turner May 2, 2025
ef24b96
Restore pylint comments
AA-Turner May 2, 2025
ec80e9b
Merge branch 'main' into ruff-format
AA-Turner May 2, 2025
40f9b0d
Revert non-JIT changes
AA-Turner May 2, 2025
d7e99f0
Add comment to longer line length
AA-Turner May 2, 2025
0488f6d
Merge branch 'main' into ruff-format
AA-Turner May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
repos:
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.11.6
rev: v0.11.7
hooks:
- id: ruff
name: Run Ruff (lint) on Doc/
Expand All @@ -22,16 +22,13 @@ repos:
name: Run Ruff (format) on Doc/
args: [--check]
files: ^Doc/

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 25.1.0
hooks:
- id: black
name: Run Black on Tools/build/check_warnings.py
- id: ruff-format
name: Run Ruff (format) on Tools/build/check_warnings.py
args: [--check, --config=Tools/build/.ruff.toml]
files: ^Tools/build/check_warnings.py
args: [--line-length=79]
- id: black
name: Run Black on Tools/jit/
- id: ruff-format
name: Run Ruff (format) on Tools/jit/
args: [--check, --config=Tools/jit/.ruff.toml]
files: ^Tools/jit/

- repo: https://github.com/pre-commit/pre-commit-hooks
Expand Down
1 change: 0 additions & 1 deletion Tools/build/check_warnings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ def get_unexpected_warnings(
"""
unexpected_warnings = {}
for file in files_with_warnings.keys():

rule = is_file_ignored(file, ignore_rules)

if rule:
Expand Down
5 changes: 5 additions & 0 deletions Tools/jit/.ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
extend = "../../.ruff.toml" # Inherit the project-wide settings

[format]
preview = true
docstring-code-format = true
24 changes: 16 additions & 8 deletions Tools/jit/_llvm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import typing

_LLVM_VERSION = 19
_LLVM_VERSION_PATTERN = re.compile(rf"version\s+{_LLVM_VERSION}\.\d+\.\d+\S*\s+")
_LLVM_VERSION_PATTERN = re.compile(
rf"version\s+{_LLVM_VERSION}\.\d+\.\d+\S*\s+"
)

_P = typing.ParamSpec("_P")
_R = typing.TypeVar("_R")
Expand All @@ -21,9 +23,7 @@ def _async_cache(f: _C[_P, _R]) -> _C[_P, _R]:
lock = asyncio.Lock()

@functools.wraps(f)
async def wrapper(
*args: _P.args, **kwargs: _P.kwargs # pylint: disable = no-member
) -> _R:
async def wrapper(*args: _P.args, **kwargs: _P.kwargs) -> _R:
async with lock:
if args not in cache:
cache[args] = await f(*args, **kwargs)
Expand All @@ -35,7 +35,9 @@ async def wrapper(
_CORES = asyncio.BoundedSemaphore(os.cpu_count() or 1)


async def _run(tool: str, args: typing.Iterable[str], echo: bool = False) -> str | None:
async def _run(
tool: str, args: typing.Iterable[str], echo: bool = False
) -> str | None:
command = [tool, *args]
async with _CORES:
if echo:
Expand All @@ -48,7 +50,9 @@ async def _run(tool: str, args: typing.Iterable[str], echo: bool = False) -> str
return None
out, _ = await process.communicate()
if process.returncode:
raise RuntimeError(f"{tool} exited with return code {process.returncode}")
raise RuntimeError(
f"{tool} exited with return code {process.returncode}"
)
return out.decode()


Expand All @@ -60,7 +64,9 @@ async def _check_tool_version(name: str, *, echo: bool = False) -> bool:

@_async_cache
async def _get_brew_llvm_prefix(*, echo: bool = False) -> str | None:
output = await _run("brew", ["--prefix", f"llvm@{_LLVM_VERSION}"], echo=echo)
output = await _run(
"brew", ["--prefix", f"llvm@{_LLVM_VERSION}"], echo=echo
)
return output and output.removesuffix("\n")


Expand Down Expand Up @@ -92,7 +98,9 @@ async def maybe_run(
return path and await _run(path, args, echo=echo)


async def run(tool: str, args: typing.Iterable[str], echo: bool = False) -> str:
async def run(
tool: str, args: typing.Iterable[str], echo: bool = False
) -> str:
"""Run an LLVM tool if it can be found. Otherwise, raise RuntimeError."""
output = await maybe_run(tool, args, echo=echo)
if output is None:
Expand Down
12 changes: 9 additions & 3 deletions Tools/jit/_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ class COFFSection(typing.TypedDict):
class ELFSection(typing.TypedDict):
"""An ELF object file section."""

Flags: dict[typing.Literal["Flags"], list[dict[typing.Literal["Name"], str]]]
Flags: dict[
typing.Literal["Flags"], list[dict[typing.Literal["Name"], str]]
]
Index: int
Info: int
Relocations: list[dict[typing.Literal["Relocation"], ELFRelocation]]
Expand All @@ -110,11 +112,15 @@ class MachOSection(typing.TypedDict):
"""A Mach-O object file section."""

Address: int
Attributes: dict[typing.Literal["Flags"], list[dict[typing.Literal["Name"], str]]]
Attributes: dict[
typing.Literal["Flags"], list[dict[typing.Literal["Name"], str]]
]
Index: int
Name: dict[typing.Literal["Value"], str]
Relocations: typing.NotRequired[
list[dict[typing.Literal["Relocation"], MachORelocation]]
]
SectionData: typing.NotRequired[dict[typing.Literal["Bytes"], list[int]]]
Symbols: typing.NotRequired[list[dict[typing.Literal["Symbol"], _MachOSymbol]]]
Symbols: typing.NotRequired[
list[dict[typing.Literal["Symbol"], _MachOSymbol]]
]
80 changes: 51 additions & 29 deletions Tools/jit/_stencils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,18 @@ class Stencil:

body: bytearray = dataclasses.field(default_factory=bytearray, init=False)
holes: list[Hole] = dataclasses.field(default_factory=list, init=False)
disassembly: list[str] = dataclasses.field(default_factory=list, init=False)
disassembly: list[str] = dataclasses.field(
default_factory=list, init=False
)

def pad(self, alignment: int) -> None:
"""Pad the stencil to the given alignment."""
offset = len(self.body)
padding = -offset % alignment
if padding:
self.disassembly.append(f"{offset:x}: {' '.join(['00'] * padding)}")
self.disassembly.append(
f"{offset:x}: {' '.join(['00'] * padding)}"
)
self.body.extend([0] * padding)

def add_nops(self, nop: bytes, alignment: int) -> None:
Expand All @@ -230,35 +234,41 @@ def remove_jump(self) -> None:
"""Remove a zero-length continuation jump, if it exists."""
hole = max(self.holes, key=lambda hole: hole.offset)
match hole:
case Hole(
offset=offset,
kind="IMAGE_REL_AMD64_REL32",
value=HoleValue.GOT,
symbol="_JIT_CONTINUE",
addend=-4,
) as hole:
case (
Hole(
offset=offset,
kind="IMAGE_REL_AMD64_REL32",
value=HoleValue.GOT,
symbol="_JIT_CONTINUE",
addend=-4,
) as hole
):
Comment on lines +233 to +241
Copy link
Member

Choose a reason for hiding this comment

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

These case rewrites are... odd. I don't see how it's better in any way. Is there a setting of some sort that's causing this? Maybe because one case has a guard... but that doesn't seem like a good enough reason to indent every case and wrap them in parens.

Copy link
Member

Choose a reason for hiding this comment

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

this is https://astral.sh/blog/ruff-v0.9.0#automatically-parenthesized-if-guards-in-match-case-statements (which Black doesn't do)

Copy link
Member Author

Choose a reason for hiding this comment

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

This case (237-245) doesn't have an if-guard, though. Are all cases put in brackets if any have a guard?

Copy link
Member

Choose a reason for hiding this comment

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

in that case it might just be due to the line length being shorter than it was previously? Addressing #133123 (review) might fix it

Copy link
Member

@brandtbucher brandtbucher May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I’m not sure that’s the issue. This uses more horizontal space than not wrapping in parens, due to the additional indent.

I suspect it is because one of the cases has a guard, which feels like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the wrap-to-80 commit separate (42ce014 (#133123)), and the change was before that.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe it has to do with the name assignment to hole?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it seems to be as hole that triggers the brackets. That does seem like a bug?

Copy link
Member

@brandtbucher brandtbucher May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. It's not just class patterns, outer grouping symbols in the pattern like (), [], and {} aren't being used effectively. See this playground, where I have my expected formatting going in and the buggy formatting coming out: https://play.ruff.rs/81f62f2d-6858-43eb-8a9b-67dcebdc56f6?secondary=Format

Copy link
Member

Choose a reason for hiding this comment

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

astral-sh/ruff#17796

# jmp qword ptr [rip]
jump = b"\x48\xff\x25\x00\x00\x00\x00"
offset -= 3
case Hole(
offset=offset,
kind="IMAGE_REL_I386_REL32" | "R_X86_64_PLT32" | "X86_64_RELOC_BRANCH",
value=HoleValue.CONTINUE,
symbol=None,
addend=addend,
) as hole if (
_signed(addend) == -4
):
case (
Hole(
offset=offset,
kind="IMAGE_REL_I386_REL32"
| "R_X86_64_PLT32"
| "X86_64_RELOC_BRANCH",
value=HoleValue.CONTINUE,
symbol=None,
addend=addend,
) as hole
) if _signed(addend) == -4:
# jmp 5
jump = b"\xe9\x00\x00\x00\x00"
offset -= 1
case Hole(
offset=offset,
kind="R_AARCH64_JUMP26",
value=HoleValue.CONTINUE,
symbol=None,
addend=0,
) as hole:
case (
Hole(
offset=offset,
kind="R_AARCH64_JUMP26",
value=HoleValue.CONTINUE,
symbol=None,
addend=0,
) as hole
):
# b #4
jump = b"\x00\x00\x00\x14"
case _:
Expand All @@ -285,13 +295,21 @@ class StencilGroup:
_trampolines: set[int] = dataclasses.field(default_factory=set, init=False)

def process_relocations(
self, known_symbols: dict[str, int], *, alignment: int = 1, nop: bytes = b""
self,
known_symbols: dict[str, int],
*,
alignment: int = 1,
nop: bytes = b"",
) -> None:
"""Fix up all GOT and internal relocations for this stencil group."""
for hole in self.code.holes.copy():
if (
hole.kind
in {"R_AARCH64_CALL26", "R_AARCH64_JUMP26", "ARM64_RELOC_BRANCH26"}
in {
"R_AARCH64_CALL26",
"R_AARCH64_JUMP26",
"ARM64_RELOC_BRANCH26",
}
and hole.value is HoleValue.ZERO
and hole.symbol not in self.symbols
):
Expand All @@ -314,7 +332,9 @@ def process_relocations(
if hole.value is HoleValue.GOT:
assert hole.symbol is not None
hole.value = HoleValue.DATA
hole.addend += self._global_offset_table_lookup(hole.symbol)
hole.addend += self._global_offset_table_lookup(
hole.symbol
)
hole.symbol = None
elif hole.symbol in self.symbols:
hole.value, addend = self.symbols[hole.symbol]
Expand All @@ -332,7 +352,9 @@ def process_relocations(
self.data.holes.sort(key=lambda hole: hole.offset)

def _global_offset_table_lookup(self, symbol: str) -> int:
return len(self.data.body) + self._got.setdefault(symbol, 8 * len(self._got))
return len(self.data.body) + self._got.setdefault(
symbol, 8 * len(self._got)
)

def _emit_global_offset_table(self) -> None:
got = len(self.data.body)
Expand Down
Loading
Loading