if I read this correctly, we effectively previously ran this with globals=locals(), locals=globals()?

You do. But we have no reason to pass locals() anyway, and we don’t pass locals() in the normal non-interactive mode, nor do we specify globals= as a keyword argument to exec().

I just meant explicitly setting globals by kwarg makes this a bit less error-prone.

I understand, but when passing one argument, there’s no error to be prone to; the same argument is used as both globals and locals.

huh, that's not really obvious. Then maybe a comment explaining what we do and why?

If you expand a bit more of the diff context, you’ll see we already do exec(tests, globals()) 6 lines above this; exec has clearer documentation but works exactly the same way. I don’t see any other way a reader might think it would work? What comment do you suggest?

I just think we already mistakenly confused locals and globals (by relying on the number of arguments), and being a bit more descriptive in what we actually pass here as what wouldn't hurt. How things are handled if only one argument is passed isn't described anywhere in the function docstring either. Just using a subtle globals= makes it clear.

Remember, the main bug wasn’t that the arguments were provided in the wrong order—it’s that two arguments were provided at all. The bug would still have been there, just with different variables, if the two arguments had been provided in the right order:

>>> import ptpython
>>> a = 12
>>> def f(b):
...     ptpython.repl.embed(globals(), locals())
... 
>>> f(34)
>>> a
12
>>> (lambda: a)()
12
>>> b
34
>>> (lambda: b)()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in <lambda>
NameError: name 'b' is not defined

name 'b' is not defined

It might have been less visible since we don’t actually need any of the locals from inside the function that called embed. But that’s just another way of saying that the correct solution is to avoid providing the caller’s locals() at all.

Now, if, in some hypothetical alternate reality, the locals and globals parameters of embed were swapped in ptpython, then in order for it to continue supporting one-argument calls, the swap would necessarily need to preserve the property that the second parameter defaults to the first parameter, not the property that the locals parameter defaults to the globals parameter.

-def embed(globals=None, locals=None, …):
+def embed(locals=None, globals=None, …):
     ...
-    locals = locals or globals
+    globals = globals or locals
     …

So embed(globals()) would still do the right thing, while embed(globals=globals()) would not. Therefore, even if we’re trying to be robust against counterfactual multiverse translocation, we really do want to provide the first argument here, not the globals argument.

This may explain why exec(code, globals()) is idiomatic (see 6 lines above, and 2to3’s translation of exec code in globals()) and exec(code, globals=globals()) is not.

I understand the desire to find a subtle way to prevent this bug from happening in the future, but the best way to do that is to provide a correct call that can be referenced rather than an incorrect one. I’ll add a comment linking back here if it will help get this PR merged.