Implement CPython-like implicit string concatenation in parser by charliermarsh · Pull Request #4097 · RustPython/RustPython

charliermarsh · 2022-08-18T01:17:25Z

Summary

This PR attempts to address #4095 by modifying the parser's handling of implicit string concatenation to better match CPython's behavior. Specifically, rather than always returning a JoinedStr in the presence of multiple substrings, we return a single Constant if all the substrings are constant, and a single JoinedStr if any of the substrings are JoinedStr, but with the sub-expressions flattened as much as possible.

charliermarsh · 2022-08-18T01:18:30Z

(High- and low-level feedback welcome!)

fanninpm · 2022-08-18T01:26:53Z

@youknowone does this conflict with #4090?

youknowone

Nice work! The approach seems convincing. I left a few comments about high-level decisions.

@fanninpm conflicts happen everyday and we always do rebase. It will not be a big deal.

youknowone · 2022-08-18T08:06:33Z

+        if values
+            .iter()
+            .all(|value| matches!(value.node, ExprKind::Constant { .. }))
+        {


this condition seems to be handled by compiler instead of parser

But:

import ast expr = """( 'Hello, ' 'world!' )""" print(ast.dump(ast.parse(expr)))

Returns: Module(body=[Expr(value=Constant(value='Hello, world!', kind=None))], type_ignores=[]).

So my interpretation was that the CPython AST parser was handling this condition. Unless I'm misunderstanding.

Thank you, I was not understanding this part that well.
I think this is a special case handling to check if it contains an f-string or not.
The other logic part looks same to the general case.
How about having a flag to check it? Then we don't need to keep the same error handling logic for both branch.

charliermarsh · 2022-08-18T14:54:25Z

+                        } else {
+                            return Err(LexicalError {
+                                location,
+                                error: LexicalErrorType::OtherError(


Would it be preferable to define a custom error type here, as with f-strings?

I think this is not possible to hit if parser doesn't have a bug. How about panic?

youknowone · 2022-08-18T15:46:55Z

@fanninpm oh, if you wanted to asked to check if #4042 has string changes to conflict, the answer is still no. only ! conversions changed in the update.

charliermarsh · 2022-08-18T16:11:51Z

+                .map(|value| match value.node {
+                    ExprKind::JoinedStr { values } => Ok(values),
+                    ExprKind::Constant { .. } => Ok(vec![value]),
+                    _ => Err(LexicalError {


Considering structuring this code such that I iterate once upfront to ensure that I only have these expression types (maybe even cast them to a custom Enum that's distinct from ExprKind?), so that I don't have to handle these mismatch errors multiple times.

I suggested a way to use StringKind instead of Expr. That will be helpful if you want to go.

youknowone

Thank you! I learned more from your comments.

Here is 2 choices of suggestions. Minimal fix or more refactoring.

Because I think the string-only branch is duplicated logic, I want to remove it.

Suggestion 1. If you want to finish this PR quickly, please remove the string-only branch and use has_fstring flag I suggested to decide to create JoinedStr. That will be good enough. And we can refactor it later.
Suggestion 2. If you want to fully refactor the string processing, please move the whole string expr handling to the new function and use the StringKind instead of ast::ExprKind matches.

youknowone · 2022-08-18T16:09:55Z

+                        } else {
+                            return Err(LexicalError {
+                                location,
+                                error: LexicalErrorType::OtherError(


I think this is not possible to hit if parser doesn't have a bug. How about panic?

youknowone · 2022-08-18T16:13:37Z

-        } else {
-            values.into_iter().next().unwrap()
-        })
+        parse_implicit_concatenation(values).map_err(|e| e.into())


the similar part in cpython is _PyPegen_concatenate_strings

youknowone · 2022-08-18T16:54:07Z

+        if values
+            .iter()
+            .all(|value| matches!(value.node, ExprKind::Constant { .. }))
+        {


Thank you, I was not understanding this part that well.
I think this is a special case handling to check if it contains an f-string or not.
The other logic part looks same to the general case.
How about having a flag to check it? Then we don't need to keep the same error handling logic for both branch.

youknowone · 2022-08-18T16:58:22Z

        let values = values.collect::<Result<Vec<_>, _>>()?;
-
-        Ok(if values.len() > 1 {
-            ast::Expr::new(location, ast::ExprKind::JoinedStr { values })
-        } else {
-            values.into_iter().next().unwrap()
-        })
+        parse_implicit_concatenation(values).map_err(|e| e.into())


The whole process(L959-975) looks like to be fit in parse_implicit_concatenation.
Creating Expr or Constant here is looking too eager now - which was previously ok but we can handle it better because you enhanced it a lot.
Having f-string marker will be easier (which can be used to decide to make JoinedStr)

youknowone · 2022-08-18T17:10:19Z

+        if values
+            .iter()
+            .all(|value| matches!(value.node, ExprKind::Constant { .. }))


If you'd like to do the simpler fix,

Suggested change

if values

.iter()

.all(|value| matches!(value.node, ExprKind::Constant { .. }))

let has_fstring = values

.iter()

.any(|value| !matches!(value.node, ExprKind::Constant { .. }));

Otherwise it will be removed.

I think I still need this has_fstring flag and logic, even after the refactor, since I need this at the end:

if has_fstring { Expr::new(initial_location, ExprKind::JoinedStr { values: deduped }) } else { deduped .into_iter() .exactly_one() .expect("String must be concatenated to a single element.") }

...but correct me if I'm wrong.

(In theory, if values at the end is just a single Expr, and it's a constant, we could just return that -- but CPython seems to return a JoinedStr with one constant child, so we'd be deviating from their behavior IIUC.)

Oh, yep. Sorry for poor explanation. we need has_fstring, but not by iteration for its own. we can manage a mutable variable to update when it hits fstring branch later.

charliermarsh · 2022-08-18T18:13:25Z

Awesome, thanks tons @youknowone, I'm learning a lot from your comments. Will do another pass on this and figure out which of the two suggestions I want to pursue.

charliermarsh · 2022-08-18T23:24:47Z

@youknowone - I went for Suggestion 2, attempted to refactor to use StringKind, etc.

charliermarsh · 2022-08-18T23:26:13Z

 unicode_names2 = "0.5.0"
 phf = "0.10.1"
 ahash = "0.7.6"
+itertools = "0.10.3"


(Had to add this to the parser, assume that's ok? Also, unclear if you want to keep these sorted.)

sorting will be good. this is also ok.

youknowone · 2022-08-19T05:16:18Z

+        let mut deduped: Vec<Expr> = vec![];
+        let mut current: Vec<String> = vec![];
+        for value in values {
+            match value.node {


match here is actually matching StringKind::F or not.
The entire upper match can be moved here.

I was able to consolidate but I believe I still need an inner match?

An example would be something like:

( "foo" f"bar{1}baz" )

In this case, we want to produce JoinedStr([Constant("foobar"), FormattedValue(1), Constant("baz")]). So I have to iterate over the values of f-string within the StringKind match, IIUC.

youknowone

would you also rebase it to our main branch? parser is recently updated to 3.10 compliant version.

charliermarsh · 2022-08-19T14:02:46Z

+            StringKind::Normal => current.push(string),
+            StringKind::U => current.push(string),
+            StringKind::F => {
+                if let ExprKind::JoinedStr { values } = parse_located_fstring(&string, location)


Arguably parse_located_fstring could now return Vec<Expr>?

oh, that makes sense! we never use the returned JoinedStr

I can do this in a separate PR.

charliermarsh · 2022-08-19T14:03:15Z

(Rebased!)

youknowone · 2022-08-19T14:24:17Z

+use crate::token::StringKind;
+use itertools::Itertools;
+
+pub fn parse_implicit_concatenation(


The function is not only handling concatenation but strings itself now

Suggested change

pub fn parse_implicit_concatenation(

pub fn parse_strings(

charliermarsh · 2022-08-20T16:05:15Z

-pub fn parse_strings(
-    values: Vec<(Location, (String, StringKind))>,
-) -> Result<Expr, LexicalError> {
+pub fn parse_strings(values: Vec<(Location, (String, StringKind))>) -> Result<Expr, LexicalError> {


(Gah, thank you, forgot to cargo fmt.)

charliermarsh · 2022-08-20T16:05:49Z

Good changes, thanks @youknowone!

youknowone

Thank you! I made a few minor changes.

charliermarsh mentioned this pull request Aug 18, 2022

Implicit string concatenation produces different AST than CPython #4095

Closed

youknowone reviewed Aug 18, 2022

View reviewed changes

charliermarsh commented Aug 18, 2022

View reviewed changes

Comment thread parser/src/string.rs Resolved

charliermarsh commented Aug 18, 2022

View reviewed changes

youknowone requested changes Aug 18, 2022

View reviewed changes

charliermarsh requested a review from youknowone August 18, 2022 23:25

charliermarsh commented Aug 18, 2022

View reviewed changes

youknowone reviewed Aug 19, 2022

View reviewed changes

Charles Marsh added 7 commits August 19, 2022 09:45

Implement CPython-like implicit string concatenation in parser

8b45dee

Add three-part test

e32959a

Move more logic into parse_implicit_concatenation

9502bae

Remove extern crate

728614e

Reorder tests

2ff3d0f

Remove newline

e9907d4

Remove unused import

07a4332

charliermarsh force-pushed the charlie/joined-str branch from d3dc291 to 07a4332 Compare August 19, 2022 13:49

Charles Marsh added 2 commits August 19, 2022 09:50

Move Ok

4878848

Remove one match; sort Cargo.toml

0d0f5a2

charliermarsh commented Aug 19, 2022

View reviewed changes

Comment thread parser/Cargo.toml Resolved

charliermarsh commented Aug 19, 2022

View reviewed changes

youknowone reviewed Aug 19, 2022

View reviewed changes

Rename parse_strings

7c377ca

charliermarsh requested a review from youknowone August 19, 2022 14:45

youknowone added 2 commits August 20, 2022 23:44

fix style

853da19

refactoring

f12082c

charliermarsh commented Aug 20, 2022

View reviewed changes

youknowone approved these changes Aug 21, 2022

View reviewed changes

youknowone merged commit bea97cd into RustPython:main Aug 21, 2022

charliermarsh mentioned this pull request Aug 21, 2022

Avoid creating unused JoinedStr in FStringParser #4116

Merged

Conversation

charliermarsh commented Aug 18, 2022

Summary

Uh oh!

charliermarsh commented Aug 18, 2022

Uh oh!

fanninpm commented Aug 18, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charliermarsh commented Aug 18, 2022

Uh oh!

charliermarsh commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

youknowone Aug 18, 2022 •

edited

Loading