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
+43 −41
Draft

Lint: Use Ruff to format Tools/jit #133123

Show file tree
Hide file tree
Changes from all 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
9 changes: 3 additions & 6 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,9 @@ repos:
name: Run Ruff (format) on Tools/build/check_warnings.py
args: [--check, --config=Tools/build/.ruff.toml]
files: ^Tools/build/check_warnings.py

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 25.1.0
hooks:
- 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
7 changes: 7 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,7 @@
extend = "../../.ruff.toml" # Inherit the project-wide settings

line-length = 88 # slightly longer than PEP 8, for readability

[format]
preview = true
docstring-code-format = true
4 changes: 1 addition & 3 deletions Tools/jit/_llvm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,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: # pylint: disable = no-member
async with lock:
if args not in cache:
cache[args] = await f(*args, **kwargs)
Expand Down
52 changes: 29 additions & 23 deletions Tools/jit/_stencils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,35 +230,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 Down
12 changes: 3 additions & 9 deletions Tools/jit/_targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,7 @@ def build(
jit_stencils_new.unlink(missing_ok=True)


class _COFF(
_Target[_schema.COFFSection, _schema.COFFRelocation]
): # pylint: disable = too-few-public-methods
class _COFF(_Target[_schema.COFFSection, _schema.COFFRelocation]): # pylint: disable = too-few-public-methods
def _handle_section(
self, section: _schema.COFFSection, group: _stencils.StencilGroup
) -> None:
Expand Down Expand Up @@ -321,9 +319,7 @@ def _handle_relocation(
return _stencils.Hole(offset, kind, value, symbol, addend)


class _ELF(
_Target[_schema.ELFSection, _schema.ELFRelocation]
): # pylint: disable = too-few-public-methods
class _ELF(_Target[_schema.ELFSection, _schema.ELFRelocation]): # pylint: disable = too-few-public-methods
def _handle_section(
self, section: _schema.ELFSection, group: _stencils.StencilGroup
) -> None:
Expand Down Expand Up @@ -411,9 +407,7 @@ def _handle_relocation(
return _stencils.Hole(offset, kind, value, symbol, addend)


class _MachO(
_Target[_schema.MachOSection, _schema.MachORelocation]
): # pylint: disable = too-few-public-methods
class _MachO(_Target[_schema.MachOSection, _schema.MachORelocation]): # pylint: disable = too-few-public-methods
def _handle_section(
self, section: _schema.MachOSection, group: _stencils.StencilGroup
) -> None:
Expand Down
Loading