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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUF001 isn't highlighted for implicitly concatenated f-string #10310

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

RUF001 isn't highlighted for implicitly concatenated f-string #10310

dhruvmanila opened this issue Mar 9, 2024 · 0 comments 路 Fixed by #10311
Labels
bug Something isn't working linter Related to the linter

Comments

@dhruvmanila
Copy link
Member

Given:

x = '饾悂ad' f'饾悂ad string'
#           ^
#           Ruff only highlights this

Ruff only highlights the bad character from the second part of the implicitly concatenated string.

https://play.ruff.rs/16071c4c-a1dd-4920-b56f-e2ce2f69c843

@dhruvmanila dhruvmanila added the bug Something isn't working label Mar 9, 2024
@dhruvmanila dhruvmanila linked a pull request Mar 9, 2024 that will close this issue
@zanieb zanieb added the linter Related to the linter label 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 linter Related to the linter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants