fix the ranges of constants inside f-strings #33

davidszotten · 2023-07-20T19:48:12Z

first stab at astral-sh/ruff/issues/5907

MichaReiser

This is heading in the right direction, but there are some remaining incorrect ranges. We'll have to review every single StringParser::range() call and replace it with the correct range.

The Cursor (used for lexing in the Ruff repository) has this concept where you can start a token. What it does is that the Cursor remembers where the start of the current token is. I think something similar would be helpful. The parser could define a start function that records the current location, and we introduce a new current_range() (and current_range_and_start_new() method, which returns the range of the current value.

MichaReiser · 2023-07-21T12:20:48Z

parser/src/snapshots/rustpython_parser__parser__tests__try.snap

@@ -167,7 +167,7 @@ expression: parse_ast
                                                        values: [
                                                            Constant(
                                                                ExprConstant {
-                                                                    range: 114..133,
+                                                                    range: 116..123,


The range of the following FormattedValue must be incorrect because it overlaps with this Constant.

MichaReiser · 2023-07-21T12:23:34Z

parser/src/string.rs

@@ -516,7 +522,7 @@ impl<'a> StringParser<'a> {
                    ast::ExprConstant {
                        value: content.into(),
                        kind: None,
-                        range: self.range(),
+                        range: TextRange::new(content_start, self.location),


Nit: The code so far consistently uses self.get_pos() over location

Suggested change

range: TextRange::new(content_start, self.location),

range: TextRange::new(content_start, self.get_pos()),

davidszotten · 2023-07-21T20:40:20Z

nice catch. will keep investigating

~~i think there might be other bugs here too, e.g. f'cc{dd=}' doesn't look right (the constant becomes ccdd )~~ update: maybe this is expected

davidszotten · 2023-07-22T07:30:49Z

f'cc{dd=}'

no i'm back to thinking this isn't what we want

i think this is currently following python's ast

>>> ast.dump(ast.parse("f'b{a=}'"))
"Module(body=[Expr(value=JoinedStr(values=[Constant(value='ba='), FormattedValue(value=Name(id='a', ctx=Load()), conversion=114)]))], type_ignores=[])"

which is fine for parsing but not for preserving the source

(f'{a=}' is short hand for f'a={a}')

probably makes sense to extract this into a separate issue

davidszotten · 2023-07-22T07:40:11Z

raised astral-sh/ruff#5970

self-documenting f-strings (`f"{foo=}"`) are still outstanding see astral-sh/ruff#5970

davidszotten · 2023-07-22T17:02:50Z

updated. afict we can't store the position on self (like cursor) because the ranges are nested

MichaReiser

This looks good overall. The complexity of this code is rather high. I hope that we could rewrite it at some point to use a Cursor based lexer (or use a dedicated lexer even).

There are a few tests that have, what seems to me, invalid ranges. I assume that's because of the TODO. Do you think it would be reasonable to fix these ranges as part of this PR as well without fixing the representation?

parser/src/snapshots/rustpython_parser__string__tests__fstring_parse_self_documenting_base.snap

...rc/snapshots/rustpython_parser__string__tests__fstring_parse_self_documenting_base_more.snap

parser/src/snapshots/rustpython_parser__string__tests__parse_fstring_self_doc_prec_space.snap

MichaReiser · 2023-07-24T12:26:33Z

parser/src/string.rs

@@ -191,7 +183,7 @@ impl<'a> StringParser<'a> {
        let mut conversion = ConversionFlag::None;
        let mut self_documenting = false;
        let mut trailing_seq = String::new();
-        let location = self.get_pos();
+        let location = self.get_pos() - TextSize::from(1);


Is it guarnanteed that the character one position before is a ASCII character? I think it is because it is always the } token. Would it make sense to instead pass the start location as an argument?

thanks for prodding me on these. think i have a better solution (though it does add a new dependency. happy to discuss)

parser/src/string.rs

MichaReiser · 2023-07-24T12:28:59Z

parser/src/string.rs

@@ -472,12 +470,13 @@ impl<'a> StringParser<'a> {
                        }
                    }
                    if !content.is_empty() {
+                        let range = self.current_range(location).sub_end(1.into());


What's the reason for substracting one character here? Is it safe to assume that the preceding character is ASCII? Should we instead pass the fstring_start_location as a function argument?

MichaReiser · 2023-07-24T12:30:27Z

parser/src/string.rs

@@ -528,6 +528,7 @@ impl<'a> StringParser<'a> {

    fn parse_bytes(&mut self) -> Result<Expr, LexicalError> {
        let mut content = String::new();
+        let location = self.get_pos();


Nit: What would you think of renaming location to start_location (same in other places)? I would find it useful because it encodes the kind of location.

MichaReiser · 2023-07-24T12:30:46Z

parser/src/string.rs

@@ -583,25 +585,23 @@ impl<'a> StringParser<'a> {
        } else if self.kind.is_any_bytes() {
            self.parse_bytes().map(|expr| vec![expr])
        } else {
-            self.parse_string().map(|expr| vec![expr])
+            dbg!(self.parse_string().map(|expr| vec![expr]))


Suggested change

dbg!(self.parse_string().map(|expr| vec![expr]))

self.parse_string().map(|expr| vec![expr])

davidszotten · 2023-07-24T13:58:42Z

I hope that we could rewrite it at some point to use a Cursor based lexer (or use a dedicated lexer even).

i saw you have a lexer rewrite pr open (but not read it yet). does that make this redundant?

MichaReiser · 2023-07-24T14:29:28Z

i saw you have a lexer rewrite pr open (but not read it yet). does that make this redundant?

I don't think so. It brings in the Cursor abstraction that could help to simplify some of the code here.

MichaReiser · 2023-07-25T09:14:35Z

This is ready to merge after we've resolved the remaining nits (either close as won't do or apply the change)

davidszotten · 2023-07-25T09:40:52Z

thanks. will take a view on (probably fix) the comments not related to ranges. just wanted to resolve that first.

use multipeek so we can check for `{{` without having to consume the first `{` and can instead leave that to the sub-parsers. this also means we can get the location for the previous (now ending) range before advancing the cursor

MichaReiser · 2023-07-25T16:09:49Z

parser/src/string.rs

@@ -21,7 +22,7 @@ use rustpython_parser_core::{
 const MAX_UNICODE_NAME: usize = 88;

 struct StringParser<'a> {
-    chars: std::iter::Peekable<std::str::Chars<'a>>,
+    chars: MultiPeek<std::str::Chars<'a>>,


I would prefer if we don't have to use MultiPeek. We can instead clone the chars iterator. Cloning is cheap because it only requires coping the start end end slice pointers (and I'm somewhat certain that LLVM can figure out that it only needs to copy the start pointer).

Using clone would allow us to remove the Peekable altogether, similar to what we do in the Lexer's Cursor:

RustPython-Parser/parser/src/lexer/cursor.rs

Lines 30 to 42 in 7cbec09

/// Peeks the next character from the input stream without consuming it.

/// Returns [EOF_CHAR] if the file is at the end of the file.

pub(super) fn first(&self) -> char {

self.chars.clone().next().unwrap_or(EOF_CHAR)

}

/// Peeks the second character from the input stream without consuming it.

/// Returns [EOF_CHAR] if the position is past the end of the file.

pub(super) fn second(&self) -> char {

let mut chars = self.chars.clone();

chars.next();

chars.next().unwrap_or(EOF_CHAR)

}

Note: It does make sense to use MultiPeek if the Iterable is not cloneable or cloning is expensive. I think this doesn't apply here because storing an Option<char> is about the same size as copying the slice start position.

MichaReiser · 2023-07-25T17:17:03Z

Excellent. Thank you so much.

fix the ranges of constants inside f-strings

472f0d4

MichaReiser mentioned this pull request Jul 21, 2023

incorrect ranges for constants in joined strings? astral-sh/ruff#5907

Closed

MichaReiser suggested changes Jul 21, 2023

View reviewed changes

check all ranges

7b97cd3

self-documenting f-strings (`f"{foo=}"`) are still outstanding see astral-sh/ruff#5970

davidszotten requested a review from MichaReiser July 24, 2023 08:42

MichaReiser approved these changes Jul 24, 2023

View reviewed changes

davidszotten added 4 commits July 25, 2023 14:13

remove stray dbg

f73f1d4

use multipeek to avoid consuming single brackets

13c68ba

use multipeek so we can check for `{{` without having to consume the first `{` and can instead leave that to the sub-parsers. this also means we can get the location for the previous (now ending) range before advancing the cursor

location -> start_location

b937fe6

current_range -> range

db221e0

MichaReiser reviewed Jul 25, 2023

View reviewed changes

peek by cloning self.chars

dd1b11a

MichaReiser merged commit 13196fc into astral-sh:main Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the ranges of constants inside f-strings #33

fix the ranges of constants inside f-strings #33

davidszotten commented Jul 20, 2023

MichaReiser left a comment •

edited by konstin

Loading

MichaReiser Jul 21, 2023

MichaReiser Jul 21, 2023

davidszotten commented Jul 21, 2023 •

edited

Loading

davidszotten commented Jul 22, 2023 •

edited

Loading

davidszotten commented Jul 22, 2023

davidszotten commented Jul 22, 2023

MichaReiser left a comment •

edited

Loading

MichaReiser Jul 24, 2023

davidszotten Jul 25, 2023

MichaReiser Jul 24, 2023

MichaReiser Jul 24, 2023

MichaReiser Jul 24, 2023

davidszotten commented Jul 24, 2023

MichaReiser commented Jul 24, 2023

MichaReiser commented Jul 25, 2023

davidszotten commented Jul 25, 2023

MichaReiser Jul 25, 2023 •

edited

Loading

MichaReiser commented Jul 25, 2023

	range: TextRange::new(content_start, self.location),
	range: TextRange::new(content_start, self.get_pos()),

	dbg!(self.parse_string().map(\|expr\| vec![expr]))
	self.parse_string().map(\|expr\| vec![expr])

	/// Peeks the next character from the input stream without consuming it.
	/// Returns [EOF_CHAR] if the file is at the end of the file.
	pub(super) fn first(&self) -> char {
	self.chars.clone().next().unwrap_or(EOF_CHAR)
	}

	/// Peeks the second character from the input stream without consuming it.
	/// Returns [EOF_CHAR] if the position is past the end of the file.
	pub(super) fn second(&self) -> char {
	let mut chars = self.chars.clone();
	chars.next();
	chars.next().unwrap_or(EOF_CHAR)
	}

fix the ranges of constants inside f-strings #33

fix the ranges of constants inside f-strings #33

Conversation

davidszotten commented Jul 20, 2023

MichaReiser left a comment • edited by konstin Loading

Choose a reason for hiding this comment

MichaReiser Jul 21, 2023

Choose a reason for hiding this comment

MichaReiser Jul 21, 2023

Choose a reason for hiding this comment

davidszotten commented Jul 21, 2023 • edited Loading

davidszotten commented Jul 22, 2023 • edited Loading

davidszotten commented Jul 22, 2023

davidszotten commented Jul 22, 2023

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

MichaReiser Jul 24, 2023

Choose a reason for hiding this comment

davidszotten Jul 25, 2023

Choose a reason for hiding this comment

MichaReiser Jul 24, 2023

Choose a reason for hiding this comment

MichaReiser Jul 24, 2023

Choose a reason for hiding this comment

MichaReiser Jul 24, 2023

Choose a reason for hiding this comment

davidszotten commented Jul 24, 2023

MichaReiser commented Jul 24, 2023

MichaReiser commented Jul 25, 2023

davidszotten commented Jul 25, 2023

MichaReiser Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

MichaReiser commented Jul 25, 2023

MichaReiser left a comment •

edited by konstin

Loading

davidszotten commented Jul 21, 2023 •

edited

Loading

davidszotten commented Jul 22, 2023 •

edited

Loading

MichaReiser left a comment •

edited

Loading

MichaReiser Jul 25, 2023 •

edited

Loading