-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[syntax-errors] PEP 701 f-strings before Python 3.12 #16543
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
Merged
Merged
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
9b44a68
add variant and docs with examples
ntBre 626ce7d
add test_ok
ntBre 8336a66
add failing test_err case
ntBre 2dea97c
check for invalid components, pass tests
ntBre 990d212
add f-string kind variants and update messages
ntBre 3881490
gate whole check behind is_unsupported
ntBre 4eafe99
clippy
ntBre 532b3f9
add true positive quote and continuation cases
ntBre e8346d7
Merge branch 'main' into brent/syn-f-strings
ntBre fc97d27
set target_version for black_compatibility tests
ntBre 2bac998
add failing false positives with comments
ntBre f57ee3a
pass string comment cases
ntBre 890069c
add cases with nesting, move comment check out of loop
ntBre d488818
Merge branch 'main' into brent/syn-f-strings
ntBre d4e5498
move check_f_string_comments to helpers module
ntBre 6db743c
avoid panic on invalid f-strings
ntBre eb3551d
add a test case with escapes outside of the expression
ntBre d8d3eb1
factor out Tokens::in_range_impl and Tokens::after_impl
ntBre a07b8fe
make TokenSource::tokens private and add TokenSource::in_range
ntBre 79b88be
make check_fstring_comments a method and use in_range
ntBre 84cc975
remove f-string stack now that range is restricted
ntBre d6ec8d6
use then_some and document check_fstring_comments
ntBre 82222d5
factor out range
ntBre 2ccd4e1
Merge branch 'main' into brent/syn-f-strings
ntBre 9d393c7
add test for escaped quote outside expression portion
ntBre 61cac6f
mark the whole triple quote when reused
ntBre 6a673d0
switch to memchr searches
ntBre 3a3c107
Merge branch 'main' into brent/syn-f-strings
ntBre ffb1935
use supports_pep_701
ntBre 0b00a8a
revert Tokens changes and use handrolled approach
ntBre 45e5716
use unwrap for character indices
ntBre fe305bc
loop over memchr_iter
ntBre c093a14
add test case with multiple escapes
ntBre 779fa1a
index -> position
ntBre 682ba38
get TextSize from slash
ntBre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletions
1
crates/ruff_python_formatter/resources/test/fixtures/black/cases/fstring.options.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| {"target_version": "3.12"} |
12 changes: 12 additions & 0 deletions
12
crates/ruff_python_parser/resources/inline/err/pep701_f_string_py311.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # parse_options: {"target-version": "3.11"} | ||
| f'Magic wand: { bag['wand'] }' # nested quotes | ||
| f"{'\n'.join(a)}" # escape sequence | ||
| f'''A complex trick: { | ||
| bag['bag'] # comment | ||
| }''' | ||
| f"{f"{f"{f"{f"{f"{1+1}"}"}"}"}"}" # arbitrary nesting | ||
| f"{f'''{"nested"} inner'''} outer" # nested (triple) quotes | ||
| f"test {a \ | ||
| } more" # line continuation | ||
| f"""{f"""{x}"""}""" # mark the whole triple quote | ||
| f"{'\n'.join(['\t', '\v', '\r'])}" # multiple escape sequences, multiple errors |
7 changes: 7 additions & 0 deletions
7
crates/ruff_python_parser/resources/inline/ok/pep701_f_string_py311.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # parse_options: {"target-version": "3.11"} | ||
| f"outer {'# not a comment'}" | ||
| f'outer {x:{"# not a comment"} }' | ||
| f"""{f'''{f'{"# not a comment"}'}'''}""" | ||
| f"""{f'''# before expression {f'# aro{f"#{1+1}#"}und #'}'''} # after expression""" | ||
| f"escape outside of \t {expr}\n" | ||
| f"test\"abcd" |
10 changes: 10 additions & 0 deletions
10
crates/ruff_python_parser/resources/inline/ok/pep701_f_string_py312.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| # parse_options: {"target-version": "3.12"} | ||
| f'Magic wand: { bag['wand'] }' # nested quotes | ||
| f"{'\n'.join(a)}" # escape sequence | ||
| f'''A complex trick: { | ||
| bag['bag'] # comment | ||
| }''' | ||
| f"{f"{f"{f"{f"{f"{1+1}"}"}"}"}"}" # arbitrary nesting | ||
| f"{f'''{"nested"} inner'''} outer" # nested (triple) quotes | ||
| f"test {a \ | ||
| } more" # line continuation |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan on using this method elsewhere?
If not, we could inline the logic in
check_fstring_commentsand simplify it to avoid the iteration for theendvariable as, I think, the parser is already at that position? So, something like what Micha suggested in #16543 (comment) i.e., just iterate over the tokens in reverse order until we reach the f-string start and report an error for all theCommenttokens found.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need/want a method of some kind because
TokenSource::tokensis a private field. I could just add atokensgetter though, of course.I also tried this without
end, but cases likecaught new errors on the trailing comment. At the point we do this processing, we've bumped past the
FStringEndand any trivia tokens after it, so I think we do need to find theendpoint as well.Hmm, maybe a
tokensgetter would be nicest. Then I could do all of the processing on a single iterator incheck_fstring_commentsat least.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not use the f-string range directly? Or, is there something else I'm missing? I don't think the comment is part of the f-string range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the
node_rangecalculation avoids any trailing trivia tokens like the one that you've mentioned in the example above. This is done by keeping track of the end of the previous token which excludes some tokens like comment. Here, when you callnode_range, then it will give you the range which doesn't include the trailing comment. If it wouldn't then the f-string range would be incorrect here.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, shoot, I think the
tokensfield should still include the trailing comment. Happy to go with what you think is best here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's a good summary. We have the exact f-string range but need to match that up with the actual
Tokens in thetokensfield, which includes trailing comments.I tried the
tokensgetter and moving the logic intocheck_fstring_comments, but I do aesthetically prefer how it looked withself.tokens.in_range...even if thein_rangemethod itself looks a little weird. So I might just leave it alone for now. Thanks for double checking!