Run the following code snippet:

-----snip snip-----
import argparse

parser = argparse.ArgumentParser()
parser.add_argument("--test", dest="test", type=str,
    default=[], action='append')

args = parser.parse_args()

print args.test, repr(args.test)
-----snip snip-----

In Python 3.3 (with the appropriate syntactic changes), args.test is clearly a list.  In Python 2.7 and 3.2, args.test is the string "[]".  I can't tell from reading the docs what the expected value should be, but intuitively, the Python 3.3 behavior makes the most sense, i.e. that args.test is a list, not the string representation of a list!

Removing type=str from add_argument() "fixes" the value, but that doesn't seem right because str is the default type so that should have no effect.

We discovered this when we tried to do args.test.append('foo') and tracebacked because args.test wasn't a list.

Original bug report in Ubuntu: https://launchpad.net/bugs/1048710
but I've tested this with upstream hg head for 2.7, 3.2, and 3.3.  It works in Ubuntu's 3.3rc2 but not in upstream head 3.3rc2+, so from reading Misc/NEWS I think the fix for issue #12776 and issue #11839 might be involved.

Marking it as a release blocker and adding 3.3 since 3.3 hg trunk is affected.

I don't see how this affects 3.3: you seem to be saying the behavior is fine there.

On Sep 10, 2012, at 05:21 PM, Georg Brandl wrote:

>I don't see how this affects 3.3: you seem to be saying the behavior is fine
>there.

No, I thought it was because I tested the Ubuntu rc2 version, but the problem
exists in upstream hg 3.3rc2+ head.

But it's not a release blocker for 3.3 then.

On Sep 10, 2012, at 05:59 PM, Georg Brandl wrote:

>But it's not a release blocker for 3.3 then.

Fair enough!

Okay, this bug is clearly caused by the patch applied for issue 12776.  Here's the patch set url: http://hg.python.org/cpython/rev/74f6d87cd471

Now, if you look at this, I think this changes the semantics for non-string default with a type converter, but the question then becomes, what is the intended semantics?

The documentation at:

http://docs.python.org/py3k/library/argparse.html#the-add-argument-method

says:

type - The type to which the command-line argument should be converted.

Okay, that makes perfect sense, because the command line arguments will always be strings in need of conversion.  What *doesn't* make sense, IMHO, is that the type conversion should be applied to the default value.  This is not documented behavior, but more importantly, is unnecessary, because the .add_argument() call site can just as easily provide a default that's already been converted.  However, if you do this with the change, then the default value will get converted twice, once explicitly at the .add_argument() call site, and again, implicitly by argparse.

Also, as this bug report indicates, it then becomes impossible to provide a default that is not converted.

So I believe that the test added for issue 12776 is not correct (specifically test_type_function_call_with_non_string_default), and should be removed.  I'm not even sure the original analysis of that bug is correct, but there is a serious semantic ambiguity that needs to be resolved.  Specifically, should you support both the use case given in that bug and this bug's use case, and if so, how?  E.g.  should you even expect this to work:

.add_argument('--foo', type=open, default='/etc/passwd')

?

Maybe another way to look at it is that the conversion should only happen if the action is 'store'.  It definitely should not happen if the action is 'append'.

Let's say that the latter is the intended semantics.  Attached is a patch that implements these semantics, with a test.  If approved, I think we also need to describe the exact semantics in the documentation.  I will do that, add a NEWS entry, and back port.

I'm not sure of all the implications of this, but it seems like this is a relevant piece of information from the docs:

"type= can take any callable that takes a single string argument and returns the converted value:"

(from http://docs.python.org/dev/library/argparse.html#type )

In particular, it doesn't seem like it's meant to be supported to be calling "type" on an argument (e.g. a default argument) that's not a string.  That's what's happening in the code snippet in the first comment above, where the default argument is a list.

I haven't thought about this very long, but what would happen if the "type" conversion is only called on arguments and default arguments that are strings, and otherwise left alone?  It seems like that would at least address the use case in the first comment, and maybe the type=open one as well.

Either way, it seems like you still might need to track whether an argument has been converted (e.g. if type converts string to another string type).

Along the lines of my previous comment, I notice that the following str type check was removed in the patch for issue 12776:

-                        if isinstance(action.default, str):
-                            default = self._get_value(action, default)
-                        setattr(namespace, action.dest, default)

But it was not preserved when the call to _get_value() was added back elsewhere:

+                    if (action.default is not None and
+                        hasattr(namespace, action.dest) and
+                        action.default is getattr(namespace, action.dest)):
+                        setattr(namespace, action.dest,
+                                self._get_value(action, action.default))

Here is a type of test case that I think should be considered for addition (to confirm that the code doesn't double-convert strings in at least one case).  Maybe there is already a test case like this:

class MyString(str): pass

def convert(s):
    return MyString("*" + s)

parser = ArgumentParser()
parser.add_argument("--test", dest="test", type=convert, default="foo")
args = parser.parse_args()

print(args.test)

I believe you've identified the broken part of the change, Chris.  So to restore previous behavior we need to add that back correctly.

On Sep 11, 2012, at 04:15 PM, Chris Jerdonek wrote:

>I haven't thought about this very long, but what would happen if the "type"
>conversion is only called on arguments and default arguments that are
>strings, and otherwise left alone?

I thought about that, and actually, my first take on a fix was almost exactly
to restore the isinstance check for str-ness.  When I thought about it longer,
it occurred to me that the type conversion for default only made sense when
action was 'store'.  Still, either approach would solve the problem.

>It seems like that would at least address the use case in the first comment,
>and maybe the type=open one as well.
>
>Either way, it seems like you still might need to track whether an argument
>has been converted (e.g. if type converts string to another string type).

That would be more complicated, so we'd really have to decide whether to back
port such changes, and whether we can sneak that into 3.3 given how late in
the process we are.

I'd be in favor either of my patch, or restoring the isinstance check
(probably in that order).

To repeat: there is no change to be made for 3.3.  3.3.0 will go out the door with the pre-12776 behavior.  So any backward compatibility concerns that apply to 2.7 and 3.2 also apply to 3.3.  Thus I suggest we restore the string check, and consider an enhancement patch at more leisure for 3.4.

On Sep 11, 2012, at 07:00 PM, R. David Murray wrote:

>To repeat: there is no change to be made for 3.3.  3.3.0 will go out the door
>with the pre-12776 behavior.  So any backward compatibility concerns that
>apply to 2.7 and 3.2 also apply to 3.3.  Thus I suggest we restore the string
>check, and consider an enhancement patch at more leisure for 3.4.

Okay, I missed that Georg has a separate branch for the 3.3.0 release, so we
don't have to worry about it there.  But yes, 2.7, 3.2, and 3.3.1 must be
fixed.

New changeset 739606bdcba4 by Barry Warsaw in branch '2.7':
- Issue #15906: Fix a regression in argparse caused by the preceding change,
http://hg.python.org/cpython/rev/739606bdcba4

New changeset bc342cd7ed96 by Barry Warsaw in branch '3.2':
- Issue #15906: Fix a regression in argparse caused by the preceding change,
http://hg.python.org/cpython/rev/bc342cd7ed96

New changeset 25e41fdc4e60 by Barry Warsaw in branch 'default':
- Issue #15906: Fix a regression in argparse caused by the preceding change,
http://hg.python.org/cpython/rev/25e41fdc4e60

[From python-dev: http://mail.python.org/pipermail/python-dev/2012-September/121683.html ]

> I've tried the various suggestions out, and I think from a practical point of
view, a fix for the regression in the 2.7, 3.2 and 3.3 branches should be to
apply the one line check for action being a _StoreAction instance.  This seems
to be the easiest way to preserve the current behavior (the fix for issues
#12776 and #11839) and fix issue #15906.  I'll update the issue and apply this change to the three branches.

This change doesn't seem right to me.  It also seems like it may cause other regressions.

The argparse documentation makes it pretty clear that 'type' is meant to be applied only to strings.

Also, the parse_args() documentation says, "Convert argument strings to objects and assign them as attributes of the namespace," but it doesn't say anything about also converting non-string defaults.

Thirdly, the documentation for the "default" keyword argument says, "The default keyword argument of add_argument(), whose value defaults to None, specifies what value should be used if the command-line argument is not present."  It doesn't say that the value should be converted before being used.

Also, here is what the change does from a behavior perspective for a couple test cases (compared to some other points in time):

parser = ArgumentParser()
parser.add_argument("--test", dest="test", action='store', type=str, default=False)
args = parser.parse_args()
print(repr(args.test))

to_str = lambda s: s.lower()

parser = ArgumentParser()
parser.add_argument("--test", dest="test", action='store', type=to_str, default=False)
args = parser.parse_args()
print(repr(args.test))

2.7.3: 
False
False

Python 3.3 (815b88454e3e; before issue 12776 patch):
False
False

Python 3.3.0rc2+:
'False'
Traceback (most recent call last):
  ...
AttributeError: 'bool' object has no attribute 'lower'

Python 3.3.0rc2+15906-1.diff:
'False'
Traceback (most recent call last):
  ...
AttributeError: 'bool' object has no attribute 'lower'

So with the change, code that previously didn't raise an error will now raise an AttributeError.  In other words, it seems like the change imposes restrictions on what default values are allowed relative to the 'type' callable where such restrictions didn't exist before.

I agree with Chris here.  We also need to add the various tests he's come up with.  IMO the easiest way to restore the original behavior and the preserve the fixes is to make the following single line change:

diff --git a/Lib/argparse.py b/Lib/argparse.py
--- a/Lib/argparse.py
+++ b/Lib/argparse.py
@@ -1962,7 +1962,8 @@
                     # only if it was defined already in the namespace
                     if (action.default is not None and
                         hasattr(namespace, action.dest) and
-                        action.default is getattr(namespace, action.dest)):
+                        action.default is getattr(namespace, action.dest) and
+                        isinstance(action.default, str)):
                         setattr(namespace, action.dest,
                                 self._get_value(action, action.default))

It looks like the correct fix was already applied, but just to chime in here:

(1) Yes, the error is that the isinstance(action.default, str) check was lost

(2) Yes, it is intended that you can use a string value as your default and the type= converter will be called on that. This feature allows DRY for people that have a complex object that can be specified by a simple string.

(3) Yes, the type= converter function should be applied to the default for all action types, not just for the store action.

The correct fix has not been applied yet.

Oh, I see, you're right - the recent changes from the Roundup Robot are exactly the wrong changes - special casing _StoreAction, not string defaults.

On Sep 12, 2012, at 10:57 AM, Steven Bethard wrote:

>(1) Yes, the error is that the isinstance(action.default, str) check was lost

Except that it won't work any more in the original location.  I tried it and
it broke other tests.  Maybe I did it wrong, so please verify by actually
applying that patch, backing mine out, and running the full test suite.

On Sep 12, 2012, at 10:39 AM, R. David Murray wrote:

>diff --git a/Lib/argparse.py b/Lib/argparse.py
>--- a/Lib/argparse.py
>+++ b/Lib/argparse.py
>@@ -1962,7 +1962,8 @@
>                     # only if it was defined already in the namespace
>                     if (action.default is not None and
>                         hasattr(namespace, action.dest) and
>-                        action.default is getattr(namespace, action.dest)):
>+                        action.default is getattr(namespace, action.dest) and
>+                        isinstance(action.default, str)):
>                         setattr(namespace, action.dest,
>                                 self._get_value(action, action.default))

For me, this results in the following failure.

[307/371/1] test_argparse
NS(foo='foo_converted')
Namespace(foo=0)
NS(foo='foo_converted')
Namespace(foo=0)
test test_argparse failed -- Traceback (most recent call last):
  File "/home/barry/projects/python/cpython/Lib/test/test_argparse.py", line 4608, in test_type_function_call_with_non_string_default
    self.assertEqual(NS(foo='foo_converted'), args)
  File "/home/barry/projects/python/cpython/Lib/test/test_argparse.py", line 29, in assertEqual
    super(TestCase, self).assertEqual(obj1, obj2)
AssertionError: NS(foo='foo_converted') != Namespace(foo=0)

This test was added for the issue #12776 and #11839 fix, and it's pretty
obvious why it fails.  In the test, default=0 (i.e. a non-string).

Do you think test_type_function_call_with_non_string_default() is a valid test
of expected semantics?  If not, then the test should be removed, and the
changeset for #12667 and #11839 should be re-evaluated, at least to determine
whether accurate tests of those bugs were applied.

If that test is removed, then the above suggested change can be made to fix
#15906.

On Sep 12, 2012, at 04:22 AM, Chris Jerdonek wrote:

>The argparse documentation makes it pretty clear that 'type' is meant to be
>applied only to strings.

Then test_type_function_call_with_non_string_default() which was added to fix
#12776 and #11839 is a bogus test, because it converts default=0 to
'foo_converted'.  This is the test that fails if you restore the
isinstance(action.default, str) test.

>Also, the parse_args() documentation says, "Convert argument strings to
>objects and assign them as attributes of the namespace," but it doesn't say
>anything about also converting non-string defaults.
>
>Thirdly, the documentation for the "default" keyword argument says, "The
>default keyword argument of add_argument(), whose value defaults to None,
>specifies what value should be used if the command-line argument is not
>present."  It doesn't say that the value should be converted before being
>used.

In which case, doing *any* conversion of default seems wrong.  Meaning, you
can't expect the following to work:

p.add_argument('--file', type=open, default='/etc/passwd')
a = p.parse_args([])
a.file.read()

because no --file argument was given, and a.file will be a string.  This
implies that if the command line argument is not given, then user code must
test the type of a.file, and explicitly open it if it's a string, because it
will only be a file object if --file *was* given on the command line.

Then why use type=open at all?  You're better off always expecting a.file to
be a string and do the conversion explicitly after you've parsed the
arguments.  But maybe that's the right interpretation given the documentation.

However, the original fix for #12776 and #11839 does not follow those
semantics.

I see. So right now, both string defaults and non-string defaults are being converted with the type= function. That seems suspect to me since the documentation explicitly says "type= can take any callable that takes a single string argument and returns the converted value", so non-string defaults don't make sense to pass to that function.

I believe test_type_function_call_with_non_string_default() comes from Arnaud Fontaine. Arnaud, can you comment on the intent of that test?

My thoughts:

* We should not be converting non-string defaults, or the documentation's description of the type= argument doesn't make sense.

* For the string defaults, I can see the argument for not converting them, and the argparse docs never show them being converted from strings. I thought there was a discussion somewhere where someone had requested the current behavior, but I can't for the life of me find that discussion, so perhaps I'm imagining it...

In terms of potential for breaking code, I'm not too worried about removing type conversion for non-string defaults - this never happened before the fix for #12776 and #11839, so I doubt much code depends on it. I am more worried about removing type conversion for string defaults - this has worked for a long time, so there probably is some code that depends on it.

*No* code should depend on it: this fix is very recent and is not in any released version of Python, not even the RCs.

The bogus test should be removed.  When I committed that patch I did not understand the correct (documented) semantics of default conversion, and so did not realize the test was bogus.

Actually, no, the test should not be removed, it should be reversed so as to test the documented behavior.

On Sep 12, 2012, at 03:03 PM, R. David Murray wrote:

>Actually, no, the test should not be removed, it should be reversed so as to
>test the documented behavior.

Good point.

Ok, sounds good. Let's make the test check the documented behavior, and then add back the isinstance(action.default, str) check.

On Sep 12, 2012, at 02:48 PM, Steven Bethard wrote:

>We should not be converting non-string defaults, or the documentation's
>description of the type= argument doesn't make sense.

Agreed.  If we also take RDM's suggestion of reversing the sense of
test_type_function_call_only_once(), then patch 15906-3.diff would do this[*],
restore the isinstance test of action.default, and retain the fix for the
regression noted in this issue's description.

>For the string defaults, I can see the argument for not converting them, and
>the argparse docs never show them being converted from strings. I thought
>there was a discussion somewhere where someone had requested the current
>behavior, but I can't for the life of me find that discussion, so perhaps I'm
>imagining it...
>
>In terms of potential for breaking code, I'm not too worried about removing
>type conversion for non-string defaults - this never happened before the fix
>for #12776 and #11839, so I doubt much code depends on it. I am more worried
>about removing type conversion for string defaults - this has worked for a
>long time, so there probably is some code that depends on it.

Agreed.  Removing the conversion of non-string defaults, but retaining the
conversion of string defaults is probably the safest course of action.

Do we need a new test for conversion of string defaults?

[*] Patch is against 2.7, but if we all agree this is the appropriate fix, I
will port it to 3.2 and 3.3.

On Sep 12, 2012, at 03:40 PM, Steven Bethard wrote:

>Ok, sounds good. Let's make the test check the documented behavior, and then
>add back the isinstance(action.default, str) check.

See patch 15906-3.diff for Python 2.7.  If acceptable, I will apply and
forward port it.

Looks good to me.

Oops, one small correction for 15906-3.diff: In Python 2.7, s/str/basestring/

Obviously isinstance should just check for str-y-ness in Python 3.{2,3}

Patch, with the basestring amendment, looks good.

> Do we need a new test for conversion of string defaults?

Yeah, I guess go ahead and add one. That will at least document our intentions here, and if we decide to change that later, then it will force us to deprecate the conversion of string defaults first (which we probably should if we're going to.)

> Do we need a new test for conversion of string defaults?

I think we should also update the documentation of the "default" keyword argument (and/or the "type" argument) to remove any ambiguity and make the behavior more clear.

Maybe one way to view what is happening (and maybe one way to describe it in the documentation) is that if a default value is a string, the parser treats it as a default for the command-line *argument*, otherwise it treats it as a default for the final *attribute*.

Lastly, can we also add the test from one of my first comments above to check that "double conversion" of strings doesn't take place?

All three of those sound like good ideas (testing string conversion, clarifying docs, adding the no-double conversion test).  Do you want to prepare the patch, Chris?  Barry can apply his any time and yours can be a followup.

We should also make a doc update equivalent to that we made for optparse in issue 5088, but that should probably be a different issue, where we discuss how/whether to change that behavior.

> Do you want to prepare the patch, Chris?

Sure, I should be able to get to this today or tomorrow.

On Sep 12, 2012, at 05:19 PM, R. David Murray wrote:

>All three of those sound like good ideas (testing string conversion,
>clarifying docs, adding the no-double conversion test).  Do you want to
>prepare the patch, Chris?  Barry can apply his any time and yours can be a
>followup.

I will add the no-double-conversion test.  I think that will just leave the
doc updates for Chris once my patch lands in all three branches.  That will be
RSN, modulo local test suite runs.

New changeset 8f847f66a49f by Barry Warsaw in branch '2.7':
A follow up for issue #15906: change the test for calling the type conversion
http://hg.python.org/cpython/rev/8f847f66a49f

New changeset 088b16bd6396 by Barry Warsaw in branch '3.2':
A follow up for issue #15906: change the test for calling the type conversion
http://hg.python.org/cpython/rev/088b16bd6396

New changeset 9ae9326cd79a by Barry Warsaw in branch 'default':
Merge 3.2 fix updates and tests for issue #15906.
http://hg.python.org/cpython/rev/9ae9326cd79a

Chris, it's all yours.  I am however going to close the bug as fixed.

Thanks for committing the change.  However--

+    def test_no_double_type_conversion_of_default(self):
+        def extend(str_to_convert):
+            return str_to_convert + '*'
+
+        parser = argparse.ArgumentParser()
+        parser.add_argument('--test', type=extend, default='*')
+        args = parser.parse_args([])
+        # The test argument will be two stars, one coming from the default
+        # value and one coming from the type conversion being called exactly
+        # once.
+        self.assertEqual(NS(test='**'), args)

This was actually my concern, that the type conversion would be applied twice, and the reason for my suggesting the test.  This expected value is not the one we want, because it shows that the argument is getting "double converted," though the test is supposed to assert that double conversion doesn't take place.

Oh, never mind.  The initial default value has one star to begin with.

> I think that will just leave the doc updates for Chris once my patch lands in all three branches.

I created an issue for this here:

http://bugs.python.org/issue15935