Repository navigation
Avoid creating unused JoinedStr in FStringParser - #4116
Conversation
| kind: None, | ||
| })); | ||
| constant_piece.clear(); | ||
| } |
There was a problem hiding this comment.
This was another difference I noticed.
In CPython, you get:
>>> import ast
>>> print(ast.dump(ast.parse('f"{foo:{spec}}"')))
Module(body=[Expr(value=JoinedStr(values=[FormattedValue(value=Name(id='foo', ctx=Load()), conversion=-1, format_spec=JoinedStr(values=[FormattedValue(value=Name(id='spec', ctx=Load()), conversion=-1)]))]))], type_ignores=[])Prior to this change, the RustPython AST was similar, except that the inner FormattedValue was padded by an empty Constant on both sides.
|
parser/src/fstring.rs conflicts to #4101 Snapshots will make annoying conflicts. My suggestion is to remove them from the current working commit and rebase it only with fstring.rs |
|
Yup, on it. |
a6479dc to
e3f4531
Compare
|
Looking into a few of the test failures. |
645143f to
a4f46d6
Compare
| .parse()?, | ||
| ), | ||
| .parse()? | ||
| })), |
There was a problem hiding this comment.
This yields a slightly different AST than CPython, we're returning a FormattedValue(JoinedStr(FormattedValue( whereas CPython just returns a FormattedValue.
There was a problem hiding this comment.
We thought the return type can be changed, but maybe actually not? I thought The returned JoinedStr is always a vector of strings, but it seems it had a nested structure.
There was a problem hiding this comment.
Yeah, although the returned JoinedStr can either contain FormattedValue, Constant, or JoinedStr, right? I think the code as-written does work, it just includes some additional nesting (but still fewer levels than before IIUC).
There was a problem hiding this comment.
As written, this does now return format_spec: Some(JoinedStr(FormattedValue(, just like CPython. (The exactly_one enforcement / unwrapping was necessary, AFAICT.)
|
I am really sorry but I just finished massive relocation from #4119. parser directory is moved under compiler/parser. |
|
Yeah no worries! |
6d3b320 to
f37bb55
Compare
f37bb55 to
5e480b8
Compare
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
I am ok with ast incompatibility if its behavior is the same.
@coolreader18 how do you think about it?
| .parse()?; | ||
| spec_constructor.push(values | ||
| .into_iter() | ||
| .exactly_one() |
There was a problem hiding this comment.
I worried if this is possible to be broken, but ExpressionNestedTooDeeply seems to prevent it successfully.
|
As an aside, how far does RustPython get through https://github.com/python/cpython/blob/3.10/Lib/test/test_fstring.py without raising a |
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
looks great. I misunderstood the changed test. So the ast seems fully compatible now.
Summary
Per discussion in #4097, we can now avoid creating a
JoinedStrentirely when parsing f-strings, and instead return the vector of values.