Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixes evaluation order in expression lists #5579

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

Merged
merged 1 commit into from
Mar 31, 2025

Conversation

ivan-shrimp
Copy link
Contributor

Enforce left-to-right evaluation with list/tuple/set expressions and positional arguments in calls.

We cannot iterate through the *-unpacked iterables after evaluating all given expressions. This PR fixes the problem by temporarily collecting the unpacked iterable into a tuple before moving on to the next element.

Fixes #5566

(**-unpacking with dicts and keyword arguments seem to be more buggy; working on a fix.)

@youknowone
Copy link
Member

@ivan-shrimp Hi, thank you for contributing. Is this still a draft or done?

@ivan-shrimp
Copy link
Contributor Author

This PR is complete as is. Feel free to merge this first.

I've marked this as draft because I'd like to fix the **-unpacking case as well; however I haven't got the time to fix that recently. The crux of that problem lies in two parts:

  • We currently treat {**x} the same as dict.update:

    RustPython/vm/src/frame.rs

    Lines 754 to 762 in 2a41072

    bytecode::Instruction::DictUpdate => {
    let other = self.pop_value();
    let dict = self
    .top_value()
    .downcast_ref::<PyDict>()
    .expect("exact dict expected");
    dict.merge_object(other, vm)?;
    Ok(None)
    }

    This means we evaluate {**[[1, 2]]} to {1: 2} when we should emit an error instead.

  • We currently assume that the x in f(**x) inherits from and behaves exactly like a dict:

    RustPython/vm/src/frame.rs

    Lines 1414 to 1437 in 2a41072

    fn execute_build_map_for_call(&mut self, vm: &VirtualMachine, size: u32) -> FrameResult {
    let size = size as usize;
    let map_obj = vm.ctx.new_dict();
    for obj in self.pop_multiple(size) {
    // Take all key-value pairs from the dict:
    let dict: PyDictRef = obj.downcast().map_err(|obj| {
    vm.new_type_error(format!("'{}' object is not a mapping", obj.class().name()))
    })?;
    for (key, value) in dict {
    if map_obj.contains_key(&*key, vm) {
    let key_repr = &key.repr(vm)?;
    let msg = format!(
    "got multiple values for keyword argument {}",
    key_repr.as_str()
    );
    return Err(vm.new_type_error(msg));
    }
    map_obj.set_item(&*key, value, vm)?;
    }
    }
    self.push_value(map_obj.into());
    Ok(None)
    }

    This means custom mappings and OrderedDict (which inherits from dict but maintains a separate internal ordering) will not work. A corresponding test case is here:

    # TODO: RUSTPYTHON
    @unittest.expectedFailure
    def test_kwargs_order(self):
    # bpo-34320: **kwargs should preserve order of passed OrderedDict
    od = collections.OrderedDict([('a', 1), ('b', 2)])
    od.move_to_end('a')
    expected = list(od.items())
    def fn(**kw):
    return kw
    res = fn(**od)
    self.assertIsInstance(res, dict)
    self.assertEqual(list(res.items()), expected)

In both cases, we should instead perform some "update using mapping" operation. Also, since the unpacking process is externally observable (evaluation order; and for calls, how non-str keys are rejected, and how duplicate keys are rejected), there is almost no alternative to effectively copying from CPython.

Feel free to take this up; I won't have the time in the very near future, and for myself it's easy to work around this.

don't emit a no-op when unpacking a single element

assume positional args stored as tuple in extended call
@youknowone youknowone marked this pull request as ready for review March 31, 2025 08:32
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

The patch looks really good. Thank you so much for contributing!

@youknowone youknowone merged commit 3ad8fd7 into RustPython:main Mar 31, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect order of evaluation with unpacking in expression lists and call arguments
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.