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

Do not include newline for unterminated string range #12017

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jun 25, 2024

Summary

This PR updates the unterminated string error range to not include the final newline character.

This is a follow-up to #12016 and required for #12019

This is not done for when the unterminated string goes till the end of file (not a newline character). The unterminated f-string range is correct.

Why is this required for #12019 ?

Because otherwise the token ranges will overlap. For example:

f"{"
f"{foo!r"

Here, the re-lexing logic recovers from an unterminated f-string and thus emitting a Newline token for the one at the end of the first line. But, currently the Unknown and the Newline token would overlap because the Unknown token (unterminated string literal) range would include the newline character.

Test Plan

Update and validate the snapshot.

@dhruvmanila dhruvmanila added the parser Related to the parser label Jun 25, 2024
@dhruvmanila dhruvmanila requested a review from MichaReiser as a code owner June 25, 2024 03:19
@dhruvmanila dhruvmanila changed the title Use correct range to highlight line continuation error Do not include newline for unterminated string range Jun 25, 2024
Copy link
Contributor

github-actions bot commented Jun 25, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, even without the requirement. The string literal would also end before the newline if it is correctly quoted.

Base automatically changed from dhruv/unknown-range to main June 25, 2024 08:05
@dhruvmanila dhruvmanila force-pushed the dhruv/unterminated-string-range branch from e7cb731 to a92f5e0 Compare June 25, 2024 08:06
@dhruvmanila dhruvmanila enabled auto-merge (squash) June 25, 2024 08:06
@dhruvmanila dhruvmanila merged commit d930e97 into main Jun 25, 2024
17 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/unterminated-string-range branch June 25, 2024 08:10
dhruvmanila added a commit that referenced this pull request Jun 25, 2024
## Summary

This PR updates the parser test infrastructure to validate the token
ranges.

From the code documentation:
```
/// Verifies that:
/// * the ranges are strictly increasing when loop the tokens in insertion order
/// * all ranges are within the length of the source code
```

Follow-up from #12016 and #12017
resolves: #11938

## Test Plan

Make sure that there are no failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Related to the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants