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

Spurious PLE1300 error with 0.0.283 #6442

Closed
DimitriPapadopoulos opened this issue Aug 9, 2023 · 10 comments · Fixed by #6616
Closed

Spurious PLE1300 error with 0.0.283 #6442

DimitriPapadopoulos opened this issue Aug 9, 2023 · 10 comments · Fixed by #6616
Assignees
Labels
bug Something isn't working

Comments

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Aug 9, 2023

After upgrading v0.0.281 → v0.0.282 (codespell-project/codespell#3011) we see what we think is a spurious PLE1300 error:

  • Minimal code snippet:
    s = "{0}{1:{width}}".format("a", "1", width=15)
  • The command invoked:
    $ ruff --select PLE file.py 
    file.py:1:5: PLE1300 Unsupported format character '}'
    Found 1 error.
    $ 
    
  • The current Ruff settings: no pyproject.toml file
  • The current Ruff version:
    $ ruff --version
    ruff 0.0.283
    $ 
    

The Format String Syntax documentation states:

A format_spec field can also include nested replacement fields within it. These nested replacement fields may contain a field name, conversion flag and format specification, but deeper nesting is not allowed.

@MichaReiser
Copy link
Member

This is probably related to #6171

@MichaReiser MichaReiser added the bug Something isn't working label Aug 9, 2023
@intgr
Copy link
Contributor

intgr commented Aug 9, 2023

I believe this occurred with the 0.0.282 -> 0.0.283 update, at least that's what caused it for me, not 0.0.281 → 0.0.282.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Aug 9, 2023

Perhaps I don't understand how pre-commit checks work. Our .pre-commit-config.yaml contains:

  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.0.282
    hooks:
      - id: ruff

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Aug 9, 2023

But yes, you're right about it, it's specific to 0.0.283:

$ pip install --force-reinstall --user ruff==0.0.281
Collecting ruff==0.0.281
  Downloading ruff-0.0.281-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (5.6 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 5.6/5.6 MB 11.0 MB/s eta 0:00:00
Installing collected packages: ruff
  Attempting uninstall: ruff
    Found existing installation: ruff 0.0.283
    Uninstalling ruff-0.0.283:
      Successfully uninstalled ruff-0.0.283
Successfully installed ruff-0.0.281

[notice] A new release of pip is available: 23.1.2 -> 23.2.1
[notice] To update, run: python3 -m pip install --upgrade pip
$ 
$ ruff --select PLE file.py 
$ 
$ pip install --force-reinstall --user ruff==0.0.282
Collecting ruff==0.0.282
  Downloading ruff-0.0.282-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (5.6 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 5.6/5.6 MB 12.6 MB/s eta 0:00:00
Installing collected packages: ruff
  Attempting uninstall: ruff
    Found existing installation: ruff 0.0.281
    Uninstalling ruff-0.0.281:
      Successfully uninstalled ruff-0.0.281
Successfully installed ruff-0.0.282

[notice] A new release of pip is available: 23.1.2 -> 23.2.1
[notice] To update, run: python3 -m pip install --upgrade pip
$ 
$ ruff --select PLE file.py 
$ 
$ pip install --force-reinstall --user ruff==0.0.283
Collecting ruff==0.0.283
  Using cached ruff-0.0.283-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (5.7 MB)
Installing collected packages: ruff
  Attempting uninstall: ruff
    Found existing installation: ruff 0.0.282
    Uninstalling ruff-0.0.282:
      Successfully uninstalled ruff-0.0.282
Successfully installed ruff-0.0.283

[notice] A new release of pip is available: 23.1.2 -> 23.2.1
[notice] To update, run: python3 -m pip install --upgrade pip
$ 
$ ruff --select PLE file.py 
file.py:1:5: PLE1300 Unsupported format character '}'
Found 1 error.
$ 

I'm not a fan or pre-commit, it keeps annoying me, whether in CI jobs or on my own computer.

@DimitriPapadopoulos DimitriPapadopoulos changed the title Spurious PLE1300 error with 0.0.282 Spurious PLE1300 error with 0.0.283 Aug 9, 2023
@kurtmckee
Copy link

pre-commit

ruff v0.0.283 is getting picked up by an unpinned ruff dev dependency running in a separate CI job. pre-commit is running v0.0.282 as specified.

@charliermarsh
Copy link
Member

Thanks! I'll take a look at the format string parser. I wonder if we were silently parsing this incorrectly the whole time, and the rule just brought it to light, or if the changes to the parser from #6171 were erroneous.

@charliermarsh charliermarsh self-assigned this Aug 9, 2023
@charliermarsh
Copy link
Member

I think the parser doesn't quite support this, it'll need to be improved. (The rule was new in v0.0.283.)

@zanieb zanieb self-assigned this Aug 14, 2023
@charliermarsh charliermarsh removed their assignment Aug 14, 2023
zanieb added a commit that referenced this issue Aug 17, 2023
Closes #6442

Python string formatting like `"hello {place}".format(place="world")`
supports format specifications for replaced content such as `"hello
{place:>10}".format(place="world")` which will align the text to the
right in a container filled up to ten characters.

Ruff parses formatted strings into `FormatPart`s each of which is either
a `Field` (content in `{...}`) or a `Literal` (the normal content).
Fields are parsed into name and format specifier sections (we'll ignore
conversion specifiers for now).

There are a myriad of specifiers that can be used in a `FormatSpec`.
Unfortunately for linters, the specifier values can be dynamically set.
For example, `"hello {place:{align}{width}}".format(place="world",
align=">", width=10)` and `"hello {place:{fmt}}".format(place="world",
fmt=">10")` will yield the same string as before but variables can be
used to determine the formatting. In this case, when parsing the format
specifier we can't know what _kind_ of specifier is being used as their
meaning is determined by both position and value.

Ruff does not support nested replacements and our current data model
does not support the concept. Here the data model is updated to support
this concept, although linting of specifications with replacements will
be inherently limited. We could split format specifications into two
types, one without any replacements that we can perform lints with and
one with replacements that we cannot inspect. However, it seems
excessive to drop all parsing of format specifiers due to the presence
of a replacement. Instead, I've opted to parse replacements eagerly and
ignore their possible effect on other format specifiers. This will allow
us to retain a simple interface for `FormatSpec` and most syntax checks.
We may need to add some handling to relax errors if a replacement was
seen previously.

It's worth noting that the nested replacement _can_ also include a
format specification although it may fail at runtime if you produce an
invalid outer format specification. For example, `"hello
{place:{fmt:<2}}".format(place="world", fmt=">10")` is valid so we need
to represent each nested replacement as a full `FormatPart`.

## Test plan

Adding unit tests for `FormatSpec` parsing and snapshots for PLE1300
@tdulcet
Copy link

tdulcet commented Aug 21, 2023

This is still broken unfortunately. test.py:

import sys

number = 1234.56
print("{0:.{prec}g}".format(number, prec=sys.float_info.dig))

Ruff output:

$ ruff --select PLE1300 --isolated test.py
test.py:4:7: PLE1300 Unsupported format character 'g'
Found 1 error.

Ruff version:

$ ruff --version
ruff 0.0.285

There is a full example here that triggers two erroneous errors.

@DimitriPapadopoulos
Copy link
Contributor Author

@tdulcet Perhaps you should open a new issue, since this one has been automatically closed.

@DimitriPapadopoulos
Copy link
Contributor Author

I have opened #6767 as a follow-up to this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants