Skip to content
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.

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

Stop overriding locations for expressions within f-strings #1494

Merged
merged 2 commits into from
Dec 31, 2022

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Dec 31, 2022

RustPython/RustPython#4384 fixed the location of FormattedValue. Now, expressions within f-strings should have the correct locations. Here's a quick demo.

Screen.Recording.2022-12-31.at.13.03.53.mov
> cargo run -- --show-source --no-cache --select C,F a.py
a.py:1:4: C408 Unnecessary `list` call (rewrite as a literal)
  |
1 | f"{list()}"
  |    ^^^^^^ C408
  |
a.py:2:6: F821 Undefined name `x`
  |
2 | f"{  x   }"
  |      ^ F821
  |
a.py:4:6: C408 Unnecessary `dict` call (rewrite as a literal)
  |
4 | f"""{dict()}"""
  |      ^^^^^^ C408
  |
a.py:6:5: F821 Undefined name `x`
  |
6 | rf"{x}"
  |     ^ F821
  |
a.py:8:7: F821 Undefined name `x`
  |
8 | rf"""{x}"""
  |       ^ F821
  |
a.py:11:2: F821 Undefined name `x`
   |
11 | {x}
   |  ^ F821
   |
a.py:14:6: F821 Undefined name `x`
   |
14 | f"\n{x}"
   |      ^ F821
   |
a.py:16:10: F821 Undefined name `x`
   |
16 | f"\u1234{x}"
   |          ^ F821
   |
a.py:18:6: F821 Undefined name `x`
   |
18 | f"\\{x}"
   |      ^ F821
   |
a.py:20:14: F821 Undefined name `x`
   |
20 | f"\\\\\\\\\\{x}"
   |              ^ F821
   |
a.py:23:2: F821 Undefined name `x`
   |
23 | {x}"
   |  ^ F821
   |
Found 11 error(s).
2 potentially fixable with the --fix option.

@charliermarsh
Copy link
Member

What?!?!? Noooo way!

@charliermarsh
Copy link
Member

How the heck did you do this? That looks like a really impressive change.

@harupy
Copy link
Contributor Author

harupy commented Dec 31, 2022

What I did was the following:

  1. Use the right offset when parsing expressions within f-strings.
  2. Fix the lexer to preserve the original string to compute the right offset.

@@ -53,7 +53,7 @@ pub fn native_literals(
.slice_source_code_range(&Range::from_located(arg));
if lexer::make_tokenizer(&arg_code)
.flatten()
.filter(|(_, tok, _)| matches!(tok, Tok::String { .. } | Tok::Bytes { .. }))
.filter(|(_, tok, _)| matches!(tok, Tok::String { .. }))
Copy link
Contributor Author

@harupy harupy Dec 31, 2022

Choose a reason for hiding this comment

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

Tok::Bytes has been removed. The new lexer only uses Tok::String.

Old lexer:

# Implicitly concatenated string
b"" ""  # Tok::Bytes, Tok::String

New lexer:

b"" ""   # Tok::String (with kind=Bytes), Tok::String (kind=String)

- kind: DuplicateArgumentName
location:
row: 1
column: 24
end_location:
row: 1
column: 30
fix: ~
parent: ~
- kind: DuplicateArgumentName
location:
row: 5
column: 27
end_location:
row: 5
column: 33
fix: ~
parent: ~
- kind: DuplicateArgumentName
location:
row: 9
column: 26
end_location:
row: 9
column: 32
fix: ~
parent: ~
[]
Copy link
Contributor Author

@harupy harupy Dec 31, 2022

Choose a reason for hiding this comment

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

Duplicate function arguments like def f(a, a): pass now yield SyntaxError ( RustPython/RustPython#4380).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

@harupy harupy Dec 31, 2022

Choose a reason for hiding this comment

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

Maybe we should remove F831 because it's never raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or we can just keep it as it's not harmful.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, we can remove in a separate PR.

(Is this true in all Python versions 3.7+? If you know.)

Copy link
Contributor Author

@harupy harupy Dec 31, 2022

Choose a reason for hiding this comment

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

@charliermarsh

> docker run python:3.7 python -c "def f(a, a): pass"
  File "<string>", line 1
SyntaxError: duplicate argument 'a' in function definition

> docker run python:3.8 python -c "def f(a, a): pass"
  File "<string>", line 1
SyntaxError: duplicate argument 'a' in function definition

> docker run python:3.9 python -c "def f(a, a): pass"
  File "<string>", line 1
SyntaxError: duplicate argument 'a' in function definition

> docker run python:3.10 python -c "def f(a, a): pass"
  File "<string>", line 1
SyntaxError: duplicate argument 'a' in function definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charliermarsh

we can remove in a separate PR.

I'll file a PR.

@charliermarsh
Copy link
Member

This also seems to be a little bit faster...

@charliermarsh
Copy link
Member

Honestly this makes me so happy. This is great. Thank you.

@harupy harupy mentioned this pull request Dec 31, 2022
renovate bot referenced this pull request in ixm-one/pytest-cmake-presets Dec 31, 2022
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.203` ->
`^0.0.204` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.204/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.204/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.204/compatibility-slim/0.0.203)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.204/confidence-slim/0.0.203)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.204`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.204)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.203...v0.0.204)

#### What's Changed

- Trim CLI help during generation by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1492](https://togithub.com/charliermarsh/ruff/pull/1492)
- Escape strings when formatting check messages by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1493](https://togithub.com/charliermarsh/ruff/pull/1493)
- Add a "fix message" to every autofix-able check by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1489](https://togithub.com/charliermarsh/ruff/pull/1489)
- Stop overriding locations for expressions within f-strings by
[@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1494](https://togithub.com/charliermarsh/ruff/pull/1494)
- Remove F831 by [@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1495](https://togithub.com/charliermarsh/ruff/pull/1495)
- Fix detection of changed imports in isort plugin by
[@&#8203;squiddy](https://togithub.com/squiddy) in
[https://github.com/charliermarsh/ruff/pull/1504](https://togithub.com/charliermarsh/ruff/pull/1504)
- Remove unused snapshots by
[@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1497](https://togithub.com/charliermarsh/ruff/pull/1497)
- Improve `T20X` ranges by [@&#8203;harupy](https://togithub.com/harupy)
in
[https://github.com/charliermarsh/ruff/pull/1502](https://togithub.com/charliermarsh/ruff/pull/1502)
- Improve F811 range for function and class definitions by
[@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1499](https://togithub.com/charliermarsh/ruff/pull/1499)
- Improve PLW0120 range by [@&#8203;harupy](https://togithub.com/harupy)
in
[https://github.com/charliermarsh/ruff/pull/1500](https://togithub.com/charliermarsh/ruff/pull/1500)
- Fix N818 range by [@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1503](https://togithub.com/charliermarsh/ruff/pull/1503)
- Include fix commit message when showing violations together with
source by [@&#8203;squiddy](https://togithub.com/squiddy) in
[https://github.com/charliermarsh/ruff/pull/1505](https://togithub.com/charliermarsh/ruff/pull/1505)
- Fix E722 and F707 ranges by
[@&#8203;harupy](https://togithub.com/harupy) in
[https://github.com/charliermarsh/ruff/pull/1508](https://togithub.com/charliermarsh/ruff/pull/1508)
- Adjust `test_path` helper to detect round-trip autofix issues by
[@&#8203;squiddy](https://togithub.com/squiddy) in
[https://github.com/charliermarsh/ruff/pull/1501](https://togithub.com/charliermarsh/ruff/pull/1501)
- Generate source code with detected line ending by
[@&#8203;squiddy](https://togithub.com/squiddy) in
[https://github.com/charliermarsh/ruff/pull/1487](https://togithub.com/charliermarsh/ruff/pull/1487)
- Check for Unsupported Files and Display Errors and Warnings by
[@&#8203;saadmk11](https://togithub.com/saadmk11) in
[https://github.com/charliermarsh/ruff/pull/1509](https://togithub.com/charliermarsh/ruff/pull/1509)

**Full Changelog**:
astral-sh/ruff@v0.0.203...v0.0.204

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMiJ9-->

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants