[pyupgrade] Preserve parenthesis when fixing native literals containing newlines (UP018)

[maxmynter]

Closes #17124

Summary

Add a check whether a native literal contains a newline (e.g. int(+\n1)) in which case we must preserve the parenthesis to not break syntax.

Note: An alternative could have been stripping the newline, but I think if it's there it's there for a reason; if not other formatting rules can take care of it.

Test Plan

Extended the existing snapshot test by the affected case.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[dscorbett]

Does arg_code.contains('\n') work when the newline is a carriage return? For example:

$ printf 'int(-\r1)\r' | ruff --isolated check - --select UP018 --diff 2>&1 | grep error:
error: Fix introduced a syntax error. Reverting all changes.

If not, see #15230 and #15703 for ways to detect all newlines.

[dhruvmanila]

This does not seem to have a carriage return character as newline. It might have been stripped by your either / git. You'd need to create a separate file for this test case and include it in https://github.com/astral-sh/ruff/blob/main/.gitattributes similar to how other files are included there.

[maxmynter]

Dang. Mus've been git or pre-commit hooks because i was aware of editor autoformatting and checked it before committing.

[maxmynter]

I'm unsure how the testfiles will behave when running on different machines and I couldn't test as I have only access to windows machines. So let me know if my approach doesn't work.

For the rules RUF046 and UP018 i've added 2 test fixtures each; one with cr the other with lr newlines. All are persisted in .gitattributes.

Alternatively, we could merge those and use test eol=keep but that runs at the risk of devs running into formatting errors when they edit the fixtures.

[MichaReiser]

that runs at the risk of devs running into formatting errors when they edit the fixtures.

That's a problem that exists anyway. Not when commiting, but it means they may accidentally change the line ending locally which might be confusing when debugging a test. You can create an .editorconfig file to disable formatting in editors that support and respect .editorconfig

https://github.com/astral-sh/ruff/blob/46ab9dec181f7736a0d32be4bad59d00228c1819/crates/ruff_linter/resources/test/fixtures/pycodestyle/.editorconfig#L9-L8

[maxmynter]

I added the .editorconfigs in 67348be.

---

I think it makes sense to keep the separate files for the cr and lf cases. That way we enforce the desired line endings via .gitattributes, revert any unwanted formatting changes and the tests fail if functionality breaks.

With eol=keep we would keep accidental changes to the file. If these changes slip in, they may easily happen with the snapshot, too.

I've had some issues getting the linebreak's to render in different editors.

What do you think?

[dhruvmanila]

Do we need to include the snapshot files in here? I don't think so considering that other snapshot files aren't included and there are .editorconfig files present in both ruff and pyupgrade snapshot directories.

[maxmynter]

I think we need them.

I added both mechanisms for specific reasons:

• .editorconfig (added in 67348be): Prevents editors from automatically reformatting the test fixtures when opened
• .gitattributes: Ensures Git maintains specific line endings (CR/LF) during checkout/staging irrespective of OS

This way i want to avoid (1) tests from passing incorrectly (if both files get normalized) or (2) breaking unexpectedly (if only one gets normalized).

In 5edfdc8 I wanted to add tests for CR and LF lineendings but local normalization made tests passing as both were normalized thus not testing for CRs.

The other test cases don't seem to be sensitive to the specific line endings and should be fine as long as both fixture and snapshot are regularized on checkout by git. But fixing only the fixture, not the snapshot could make tests flaky.

Or am I missing something?

[MichaReiser]

Looking at the crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF046_RUF046_CR.py.snap snapshot, it does seem to render the \r differently (^M). That's why I think adding the snapshot here isn't needed.

[maxmynter]

Allright, addressed in 005a528.