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

Invalid fix for PYI053 for f-strings #10307

Closed
dhruvmanila opened this issue Mar 9, 2024 · 3 comments · Fixed by #10311
Closed

Invalid fix for PYI053 for f-strings #10307

dhruvmanila opened this issue Mar 9, 2024 · 3 comments · Fixed by #10311
Labels
bug Something isn't working fixes Related to suggested fixes for violations linter Related to the linter

Comments

@dhruvmanila
Copy link
Member

Given:

def foo(
    arg2: str = f'51 character {x} stringgggggggggggggggggggggggggggggggggggggggggggg',
) -> None: ...

Here, the number of characters after the replacement field is > 50 which is what we flag and fix:

src/PYI053.pyi:3:35: PYI053 [*] String and bytes literals longer than 50 characters are not permitted
  |
1 | def foo(
2 |     arg1: str = '51 character stringggggggggggggggggggggggggggggggg',
3 |     arg2: str = f'51 character {x} stringgggggggggggggggggggggggggggggggggggggggggggg',
  |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI053
4 | ) -> None: ...
  |
  = help: Replace with `...`

ℹ Safe fix
1 1 | def foo(
2 2 |     arg1: str = '51 character stringggggggggggggggggggggggggggggggg',
3   |-    arg2: str = f'51 character {x} stringgggggggggggggggggggggggggggggggggggggggggggg',
  3 |+    arg2: str = f'51 character {x}...',
4 4 | ) -> None: ...

Found 1 error.
[*] 1 fixable with the --fix option.

This is not ideal and it raises an interesting question - how to count the characters for an f-string containing replacement fields?

  1. f"one{expr}one" (6 total or 3 and 3)
  2. "one" f"one{expr}one" (9 total or 3, 3, 3)
  3. f"one{expr}one" f"one{expr}one" (12 total or 3, 3, 3, 3)
@dhruvmanila dhruvmanila added the bug Something isn't working label Mar 9, 2024
@AlexWaygood
Copy link
Member

(I don't think there's a use case for having f-strings in a .pyi file, FWIW, so I think the correctness issue with this specific rule is unlikely to come up in real life. The broader issue you're pointing to is obviously still valid, however :)

@zanieb
Copy link
Member

zanieb commented Mar 9, 2024

This is such a weird "safe fix". Does this actually have any effect? Will an IDE hint the wrong default value now?

@dhruvmanila
Copy link
Member Author

Will an IDE hint the wrong default value now?

Yeah, Ruff will basically replace only part of a f-string. The linked PR has a solution to instead count all of the characters in a f-string and replace the entire f-string if it's > 50 instead.

@zanieb zanieb added fixes Related to suggested fixes for violations linter Related to the linter labels Mar 11, 2024
dhruvmanila added a commit that referenced this issue Mar 14, 2024
## Summary

This PR updates the `StringLike::FString` variant to use `ExprFString`
instead of `FStringLiteralElement`.

For context, the reason it used `FStringLiteralElement` is that the node
is actually the string part of an f-string ("foo" in `f"foo{x}"`). But,
this is inconsistent with other variants where the captured value is the
_entire_ string.

This is also problematic w.r.t. implicitly concatenated strings. Any
rules which work with `StringLike::FString` doesn't account for the
string part in an implicitly concatenated f-strings. For example, we
don't flag confusable character in the first part of `"𝐁ad" f"𝐁ad
string"`, but only the second part
(https://play.ruff.rs/16071c4c-a1dd-4920-b56f-e2ce2f69c843).

### Update `PYI053`

_This is included in this PR because otherwise it requires a temporary
workaround to be compatible with the old logic._

This PR also updates the `PYI053` (`string-or-bytes-too-long`) rule for
f-string to consider _all_ the visible characters in a f-string,
including the ones which are implicitly concatenated. This is consistent
with implicitly concatenated strings and bytes.

For example,

```python
def foo(
	# We count all the characters here
    arg1: str = '51 character ' 'stringgggggggggggggggggggggggggggggggg',
	# But not here because of the `{x}` replacement field which _breaks_ them up into two chunks
    arg2: str = f'51 character {x} stringgggggggggggggggggggggggggggggggggggggggggggg',
) -> None: ...
```

This PR fixes it to consider all _visible_ characters inside an f-string
which includes expressions as well.

fixes: #10310 
fixes: #10307 

## Test Plan

Add new test cases and update the snapshots.

## Review

To facilitate the review process, the change have been split into two
commits: one which has the code change while the other has the test
cases and updated snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations linter Related to the linter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants