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

formatter panic with comment between joined f-strings #6898

Closed
davidszotten opened this issue Aug 26, 2023 · 7 comments · Fixed by #7047
Closed

formatter panic with comment between joined f-strings #6898

davidszotten opened this issue Aug 26, 2023 · 7 comments · Fixed by #7047
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@davidszotten
Copy link
Contributor

a3d4f08 moved the f-string comment handling added in f091b46 (which unfortunately didn't add enough test cases)

(
    f'{1}' # comment
    f'{2}'
)

panics with

Unexpected token between nodes: `"' "`', crates/ruff_python_formatter/src/comments/placement.rs:122:17
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Aug 26, 2023
@MichaReiser MichaReiser added this to the Formatter: Alpha milestone Aug 26, 2023
@cnpryer
Copy link
Contributor

cnpryer commented Sep 1, 2023

What's the correct way to read

`"' "`'

@cnpryer
Copy link
Contributor

cnpryer commented Sep 1, 2023

I imagine the "s are just lexing noise?

debug_assert!(
!matches!(token.kind, SimpleTokenKind::Bogus),
"Unexpected token between nodes: `{:?}`",
locator.slice(TextRange::new(preceding.end(), comment.start()),)
);

Edit: noise probably isn't a good way to describe it. I mean it's lexed as " tokens or something?

@charliermarsh
Copy link
Member

I think the SimpleTokenizer intentionally doesn't support lexing strings, so it hits the first quote and then lexes any subsequent characters as bogus. I'm not quite familiar enough with the f-string comment handling to be sure how to solve this, but I can play around with it for a sec to get an idea.

@davidszotten
Copy link
Contributor Author

i believe the issue is that the preceding node is the FormattedValue _inside the f-string

the printed slice is

 f'{1}' # comment
      ^^

@davidszotten
Copy link
Contributor Author

the previous solution was re-assigning the comment from the FormattedValue to the surrounding f-string

@charliermarsh
Copy link
Member

Yeah, makes sense. I can probably restore it.

@davidszotten
Copy link
Contributor Author

i found this when trying to test #6365 after rebasing on top of the refactor in the pr description.

@charliermarsh charliermarsh self-assigned this Sep 1, 2023
charliermarsh added a commit that referenced this issue Sep 1, 2023
## Summary

Restores the dangling comment handling for f-strings, which broke with
the parenthesized expression code.

Closes #6898.

## Test Plan

`cargo test`

No change in any of the similarity indexes or changed file counts:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1632 |
| django | 0.99957 | 2760 | 67 |
| transformers | 0.99927 | 2587 | 468 |
| twine | 0.99982 | 33 | 1 |
| typeshed | 0.99978 | 3496 | 2173 |
| warehouse | 0.99818 | 648 | 24 |
| zulip | 0.99942 | 1437 | 32 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants