Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions compiler/ast/asdl_rs.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,20 +227,25 @@ def visitConstructor(self, cons, parent, depth):
if cons.fields:
self.emit(f"{cons.name} {{", depth)
for f in cons.fields:
self.visit(f, parent, "", depth + 1)
self.visit(f, parent, "", depth + 1, cons.name)
self.emit("},", depth)
else:
self.emit(f"{cons.name},", depth)

def visitField(self, field, parent, vis, depth):
def visitField(self, field, parent, vis, depth, constructor=None):
typ = get_rust_type(field.type)
fieldtype = self.typeinfo.get(field.type)
if fieldtype and fieldtype.has_userdata:
typ = f"{typ}<U>"
# don't box if we're doing Vec<T>, but do box if we're doing Vec<Option<Box<T>>>
if fieldtype and fieldtype.boxed and (not (parent.product or field.seq) or field.opt):
typ = f"Box<{typ}>"
if field.opt:
if field.opt or (
# When a dictionary literal contains dictionary unpacking (e.g., `{**d}`),
# the expression to be unpacked goes in `values` with a `None` at the corresponding
# position in `keys`. To handle this, the type of `keys` needs to be `Option<Vec<T>>`.
constructor == "Dict" and field.name == "keys"
):
typ = f"Option<{typ}>"
if field.seq:
typ = f"Vec<{typ}>"
Expand Down
2 changes: 1 addition & 1 deletion compiler/ast/src/ast_gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ pub enum ExprKind<U = ()> {
orelse: Box<Expr<U>>,
},
Dict {
keys: Vec<Expr<U>>,
keys: Vec<Option<Expr<U>>>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found I changed it to the previous form to keep cpython compatibility in #4042.
I'd like to ask Option<Expr<U>> is fit in cpython way.
I thought this is the new cpython way when i was working on #4042 which is changed in 3.10

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@youknowone Why did you change it to the previous form?

@youknowone youknowone Jan 18, 2023 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't remember well so looking in it again.
I thought CPython's Python.asdl actually changed it to not optional here.
https://github.com/RustPython/RustPython/pull/4042/files#diff-242a44b9d53b6c6212a752e0c97d6cf2f87794f579700aa8015bb99cef63925cR63

But I still can see ast.parse generate None for empty keys in Python 3.10 as you described. I am confused.
Do you know what the '?' means in Python.asdl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ? means optional.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that was the reason I thought CPython changed it from optional key to non-optional one in 3.10. How they still have None in parse.dump?

values: Vec<Expr<U>>,
},
Set {
Expand Down
6 changes: 5 additions & 1 deletion compiler/ast/src/unparse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,11 @@ impl<'a> Unparser<'a> {
let (packed, unpacked) = values.split_at(keys.len());
for (k, v) in keys.iter().zip(packed) {
self.p_delim(&mut first, ", ")?;
write!(self, "{}: {}", *k, *v)?;
if let Some(k) = k {
write!(self, "{}: {}", *k, *v)?;
} else {
write!(self, "**{}", *v)?;
}
}
for d in unpacked {
self.p_delim(&mut first, ", ")?;
Expand Down
18 changes: 12 additions & 6 deletions compiler/codegen/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1986,18 +1986,24 @@ impl Compiler {
Ok(())
}

fn compile_dict(&mut self, keys: &[ast::Expr], values: &[ast::Expr]) -> CompileResult<()> {
fn compile_dict(
&mut self,
keys: &[Option<ast::Expr>],
values: &[ast::Expr],
) -> CompileResult<()> {
let mut size = 0;

let (packed_values, unpacked_values) = values.split_at(keys.len());
for (key, value) in keys.iter().zip(packed_values) {
self.compile_expression(key)?;
let (packed, unpacked): (Vec<_>, Vec<_>) = keys
.iter()
.zip(values.iter())
.partition(|(k, _)| k.is_some());
for (key, value) in packed {
self.compile_expression(key.as_ref().unwrap())?;
self.compile_expression(value)?;
size += 1;
}
emit!(self, Instruction::BuildMap { size });

for value in unpacked_values {
for (_, value) in unpacked {
Comment thread
harupy marked this conversation as resolved.
Resolved
self.compile_expression(value)?;
emit!(self, Instruction::DictUpdate);
}
Expand Down
11 changes: 4 additions & 7 deletions compiler/codegen/src/symboltable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -885,13 +885,10 @@ impl SymbolTableBuilder {
self.scan_expression(value, ExpressionContext::Load)?;
}
Dict { keys, values } => {
let (packed, unpacked) = values.split_at(keys.len());
for (key, value) in keys.iter().zip(packed) {
self.scan_expression(key, context)?;
self.scan_expression(value, context)?;
}
for value in unpacked {
// dict unpacking marker
for (key, value) in keys.iter().zip(values.iter()) {
if let Some(key) = key {
self.scan_expression(key, context)?;
}
self.scan_expression(value, context)?;
}
}
Expand Down
43 changes: 5 additions & 38 deletions compiler/parser/python.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,44 +1136,11 @@ Atom<Goal>: ast::Expr = {
}.into())
},
<location:@L> "{" <e:DictLiteralValues?> "}" <end_location:@R> => {
let pairs = e.unwrap_or_default();

let (keys, values) = match pairs.iter().position(|(k,_)| k.is_none()) {
Some(unpack_idx) => {
let mut pairs = pairs;
let (keys, mut values): (_, Vec<_>) = pairs.drain(..unpack_idx).map(|(k, v)| (*k.unwrap(), v)).unzip();

fn build_map(items: &mut Vec<(ast::Expr, ast::Expr)>) -> ast::Expr {
let location = items[0].0.location;
let end_location = items[0].0.end_location;
let (keys, values) = items.drain(..).unzip();
ast::Expr {
location,
end_location,
custom: (),
node: ast::ExprKind::Dict { keys, values }
}
}

let mut items = Vec::new();
for (key, value) in pairs.into_iter() {
if let Some(key) = key {
items.push((*key, value));
continue;
}
if !items.is_empty() {
values.push(build_map(&mut items));
}
values.push(value);
}
if !items.is_empty() {
values.push(build_map(&mut items));
}
(keys, values)
},
None => pairs.into_iter().map(|(k, v)| (*k.unwrap(), v)).unzip()
};

let (keys, values) = e
.unwrap_or_default()
.into_iter()
.map(|(k, v)| (k.map(|x| *x), v))
.unzip();
ast::Expr {
location,
end_location: Some(end_location),
Expand Down
6 changes: 6 additions & 0 deletions compiler/parser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,4 +309,10 @@ with (0 as a, 1 as b,): pass
assert!(parse_program(source, "<test>").is_err());
}
}

#[test]
fn test_dict_unpacking() {
let parse_ast = parse_expression(r#"{"a": "b", **c, "d": "e"}"#, "<test>").unwrap();
insta::assert_debug_snapshot!(parse_ast);
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.