decimal.localcontext() works correctly with both the pure Python decimal module and the C accelerator.

decimal.localcontext(None) only works with the pure Python version. The C accelerator throws an exception instead of treating it as equivalent to omitting the argument.

Georg, a regression in the decimal API relative to 3.2 got picked up by #15136. Are you OK with cherrypicking a fix for this into rc2?

The feature is undocumented though. Is it a regression?

There are other places as well. The topic was raised in msg153447 and
msg153506, with no further comments. So I concluded that None argument
support wasn't important.

While it's undocumented in the main docs [1], help(decimal.localcontext) in 3.2 starts with:

localcontext(ctx=None)
    Return a context manager for a copy of the supplied context

We essentially have two votes in favour of "it should work" (the pure Python impl and the docstring), and two in favour of "meh" (the prose docs and the lack of a test case)

I think we should fix it, but I don't mind if Georg wants it to wait until 3.3.1

[1] http://docs.python.org/py3k/library/decimal#decimal.localcontext

Can't this be fixed in the CONTEXT_CHECK_VA macro?
With the attached patch decimal.localcontext(None) works and there aren't any failures, however I don't think there are tests that pass None explicitly, so this approach might not work.

OK, but for example:

Help on function __truediv__ in module decimal:

__truediv__(self, other, context=None)
    Return self / other.

Here I think it's undisputed that a C version should not cram a context
argument into a number method. There are many functions in decimal where
some arguments are just implementation details, or worse, leftovers
(see #10650).

So I simply don't know if localcontext(None) was intended or (once)
an implementation detail.

localcontext(None) by itself looks kind of awkward to me. The only valid
use case I can see is indeed the one from #15136.

I don't particularly mind either way for localcontext(), but in more
speed sensitive areas I would care.

> Can't this be fixed in the CONTEXT_CHECK_VA macro?

No, the macro should only allow contexts. E.g. in the case of localcontext(None)
context_copy(local, NULL) would be called on local=Py_None, which is undefined.

What would perhaps be needed for a nice C-API is a function like

   PyArg_ParseTupleSkipNone()

that treats None arguments in the same way as optional args that are
not given.

This is the complete list of context=None divergences. It would
be possible to change that, since most of the functions are
generated by macro wrappers.


# This is the only place where 'ctx' is used
localcontext(ctx=None)

canonical(self, context=None)
compare(self, other, context=None)
compare_signal(self, other, context=None)
exp(self, context=None)
fma(self, other, third, context=None)
is_normal(self, context=None)
is_subnormal(self, context=None)
ln(self, context=None)
log10(self, context=None)
logb(self, context=None)
logical_and(self, other, context=None)
logical_invert(self, context=None)
logical_or(self, other, context=None)
logical_xor(self, other, context=None)
max(self, other, context=None)
max_mag(self, other, context=None)
min(self, other, context=None)
min_mag(self, other, context=None)
next_minus(self, context=None)
next_plus(self, context=None)
normalize(self, context=None)
number_class(self, context=None)
quantize(self, exp, rounding=None, context=None, watchexp=True)
remainder_near(self, other, context=None)
rotate(self, other, context=None)
scaleb(self, other, context=None)
shift(self, other, context=None)
sqrt(self, context=None)
to_eng_string(self, context=None)
to_integral(self, rounding=None, context=None)
to_integral_value(self, rounding=None, context=None)
to_integral_exact(self, rounding=None, context=None)

context.power(self, a, b, modulo=None)

This doesn't strike me as something it's essential to resolve before 3.3.1.

And I agree that decimal.localcontext(None) looks peculiar.  I noticed this yesterday when looking at #15136 and dismissed it as an artifact of the implementation---I didn't think to check what the docs said.  I think it's a bit unfortunate that the docs specify "ctx=None".  I doubt that too much thought went into that;  without knowing the history, I suspect that at the time it was just a way to indicate that the context was an optional argument.

IOW, my vote would be not to require that None is accepted, and to treat this as an minor error in the documentation.

Adding Raymond to the nosy, since he may know more about what was intended.

Note that it's only the 3.2 interactive help that specifies "ctx=None", and it's almost certainly lifting that directly from the Python implementation.

The C implementation is definitely in compliance with the prose docs, which give the signature as localcontext([c]) and say "If no context is specified, a copy of the current context is used" without indicating whether passing in None explicitly is supposed to count as not specifying a context.

However, the specific problem that prompted this report is that you used to be able to write code like:

  def my_decimal_func(arg1, arg2, context=None):
    with decimal.localcontext(context):
        # perform decimal operations on arg1 and arg2

but with the C implementation, such code will break. You have to instead write something less idiomatic like:

  def my_decimal_func(arg1, arg2, *context):
    with decimal.localcontext(*context):
        # perform decimal operations on arg1 and arg2

Any third party Decimal manipulating function that accepts an optional context and passes it down to the standard Decimal API will be confronted with the same problem in 3.3: passing None as the context no longer means the same thing as omitting the context argument entirely.

We can either handle this as a documentation issue (recommending the *args based idiom above as an alternative to passing an explicit None), or else change the C implementation to accept context=None as equivalent to omitting the context argument entirely.

I'm inclined towards the latter, simply due to the backwards compatibility breach: explicitly calling localcontext(None) would indeed be silly, but calling localcontext(context), where context comes from a parameter with a default value of None, is perfectly normal Python code. Idiomatic code that worked correctly in the previous Python version is a reasonably strong argument in favour of retaining the feature.

Nick Coghlan <report@bugs.python.org> wrote:
> Any third party Decimal manipulating function that accepts an
> optional context and passes it down to the standard Decimal API
> will be confronted with the same problem in 3.3: passing None
> as the context no longer means the same thing as omitting the
> context argument entirely.

I agree, but for me the issue is: What is the standard API?

If you look at the Context methods in decimal.py, you see this pattern:

def add(self, a, b):
    ...
    r = a.__add__(b, context=self)


So I think it's reasonably safe to say that all Decimal methods only
take a context=None argument because it happens to be a convenient way
of implementing the Context methods.

With that reasoning, most of the list in msg169144 would be eliminated
already. I sort of regret that the Decimal methods of the C version take
a context argument at all, but the arguments are documented.

Now to localcontext(ctx=None). Yes, code might exist that does the
following:

def f(a, b, context=None):
    with localcontext(context):
        return a / b


It is, however, a strange function: If I explicitly pass a context to
a function, I'd expect that it is also used for recording any status
that accumulates in the function (or that the function actually *can*
accumulate status at all).


If I'm only interested in the precision, I'd write:

def f(a, b, prec=None):
    with localcontext() as c:
        c.prec = 9 if prec is None else prec
        return Decimal(a) / b


[This is along the lines of Raymond's original suggestion in #15136.]


But there are other examples of unexpected behavior, such as
Decimal.to_eng_string(context) taking a context purely to determine
the value of context.capitals, i.e. no status flags can possibly be
set. Here I'd also prefer:

    to_eng_string(capitals=1)

I don't want to block this, BTW. Personally I'm +-0 on the issue.

If more people think this is needed for backwards compatibility, I'll
write a patch for localcontext. But I'd rather do that *very* soon.

FWIW, none of the cdecimal users has ever complained about this.

So we have a tie here: Mark thinks it can wait, Nick thinks it's
better to do something.

I don't really feel that strongly either way, either.  I understand Nick's arguments, but can't help feeling that we're letting a doc mistake dictate the API here.  (And my inner type-system has an aversion to APIs where arguments are expected to be *either* None or of type/interface XXX.)

So colour me -0 on changing cdecimal to allow the 'None' argument.

I agree it's an acceptable risk for 3.3.0. Most third party decimal operations simply won't accept an explicit context argument at all - avoiding the need to pass the current context around explicitly is the whole point of it being stored in the thread locals.

However, I still think it would be better if consistency with the Python API could be restored for 3.3.1. If that's done via a more general "this value means the arg is actually missing" C API that can be made public in 3.4, so much the better.

Should we go ahead with this? If so, then I think it only makes sense
to change *all* undocumented cases of None default args. Here's a
case that used rounding=None in quantize():

https://bitbucket.org/simplecodes/wtforms/issue/117/test-failure-with-python-33

By "all cases" I mean the ones listed in msg169144, not the Number methods.

> Should we go ahead with this?

Sure:  if you're willing to write the patch, go for it.

OK, here's a patch. Specifics:

  o Except for the number methods, decimal.py and _decimal should behave
    identically now.

  o _decimal actually requires an additional context arg in same_quantum(),
    compare_total(), compare_total_mag() and copy_sign(). This is for raising
    InvalidOperation if the second operand is a very large integer and cannot
    be converted exactly.

    I've added the context arguments to the Python version to keep compatibility.

  o For some reason decimal.py uses a context arg in canonical(), so I've
    added it to the C version.

  o localcontext(ctx=None) is pretty ugly. Perhaps we should make the
    backwards incompatible switch to localcontext(context=None). I don't
    care a lot though.

  o I'd like to update the docstrings and the documentation in a separate
    patch.

  o The important idiom c.power(modulo=None, b=9, a=3) is now supported. :)

New patch with docs. I've eliminated the unused context arg from canonical(). I can't possibly imagine that anyone uses it (or
that anyone uses canonical() at all for that matter).

New changeset 29becac5dd9b by Stefan Krah in branch '3.3':
Issue #15783: Except for the number methods, the C version of decimal now
http://hg.python.org/cpython/rev/29becac5dd9b

Ezio, thanks for the Rietveld comments.


There are still unsupported None default values in the Context
constructor, so I'm leaving the issue open.

New changeset 907d71668d3c by Stefan Krah in branch '3.3':
Issue #15783: Support None default values in the Context() constructor.
http://hg.python.org/cpython/rev/907d71668d3c

I think I've found all of them now. Closing.