From 43f6631dfe19fc54fc005b480e99cd7aca377c41 Mon Sep 17 00:00:00 2001 From: Aryan Sachin Date: Wed, 11 Nov 2026 00:33:33 +0530 Subject: [PATCH] fix: preserve string-hinted argument values without numeric coercion (#733) When a function parameter is explicitly annotated with str or an optional string type (Optional[str] / str | None), prevent DefaultParseValue from coercing version strings, floats, and numeric representations (e.g. 1.10) into loat(1.1) or int. - Add IsStringAnnotation and AllowsNone helpers in inspectutils.py to identify string annotations across direct types, typing constructs, and union expressions. - Pass argspec nnotations into _ParseArgs and _ParseValue in core.py, preserving the original string when literal evaluation would otherwise coerce the value away from str. - Preserve custom parse function decorator precedence and standard literal parsing for untyped or non-string arguments. - Add regression test coverage in inspectutils_test.py and ire_test.py. Signed-off-by: Aryan Sachin --- fire/core.py | 32 ++++++++++++++++----- fire/fire_test.py | 21 ++++++++++++++ fire/inspectutils.py | 58 +++++++++++++++++++++++++++++++++++++++ fire/inspectutils_test.py | 18 ++++++++++++ 4 files changed, 122 insertions(+), 7 deletions(-) diff --git a/fire/core.py b/fire/core.py index 8e23e76b6..b68e2190a 100644 --- a/fire/core.py +++ b/fire/core.py @@ -715,6 +715,7 @@ def _MakeParseFn(fn, metadata): the leftover args from the arguments to the parse function. """ fn_spec = inspectutils.GetFullArgSpec(fn) + annotations = fn_spec.annotations # Note: num_required_args is the number of positional arguments without # default values. All of these arguments are required. @@ -728,7 +729,7 @@ def _ParseFn(args): # Note: _ParseArgs modifies kwargs. parsed_args, kwargs, remaining_args, capacity = _ParseArgs( fn_spec.args, fn_spec.defaults, num_required_args, kwargs, - remaining_args, metadata) + remaining_args, metadata, annotations) if fn_spec.varargs or fn_spec.varkw: # If we're allowed *varargs or **kwargs, there's always capacity. @@ -749,7 +750,8 @@ def _ParseFn(args): varargs = [] for index, value in enumerate(varargs): - varargs[index] = _ParseValue(value, None, None, metadata) + varargs[index] = _ParseValue(value, None, fn_spec.varargs, metadata, + annotations) varargs = parsed_args + varargs remaining_args += remaining_kwargs @@ -761,7 +763,7 @@ def _ParseFn(args): def _ParseArgs(fn_args, fn_defaults, num_required_args, kwargs, - remaining_args, metadata): + remaining_args, metadata, annotations=None): """Parses the positional and named arguments from the available supplied args. Modifies kwargs, removing args as they are used. @@ -777,6 +779,7 @@ def _ParseArgs(fn_args, fn_defaults, num_required_args, kwargs, remaining_args: The remaining command line arguments, which may still be used as positional arguments. metadata: Metadata about the function, typically from Fire decorators. + annotations: The type annotations dict from the function's argspec. Returns: parsed_args: A list of values to be used as positional arguments for calling the target function. @@ -795,13 +798,13 @@ def _ParseArgs(fn_args, fn_defaults, num_required_args, kwargs, for index, arg in enumerate(fn_args): value = kwargs.pop(arg, None) if value is not None: # A value is specified at the command line. - value = _ParseValue(value, index, arg, metadata) + value = _ParseValue(value, index, arg, metadata, annotations) parsed_args.append(value) else: # No value has been explicitly specified. if remaining_args and accepts_positional_args: # Use a positional arg. value = remaining_args.pop(0) - value = _ParseValue(value, index, arg, metadata) + value = _ParseValue(value, index, arg, metadata, annotations) parsed_args.append(value) elif index < num_required_args: raise FireError( @@ -814,7 +817,7 @@ def _ParseArgs(fn_args, fn_defaults, num_required_args, kwargs, parsed_args.append(fn_defaults[default_index]) for key, value in kwargs.items(): - kwargs[key] = _ParseValue(value, None, key, metadata) + kwargs[key] = _ParseValue(value, None, key, metadata, annotations) return parsed_args, kwargs, remaining_args, capacity @@ -962,7 +965,7 @@ def _IsMultiCharFlag(argument): return argument.startswith('--') or re.match('^-[a-zA-Z]', argument) -def _ParseValue(value, index, arg, metadata): +def _ParseValue(value, index, arg, metadata, annotations=None): """Parses value, a string, into the appropriate type. The function used to parse value is determined by the remaining arguments. @@ -972,10 +975,12 @@ def _ParseValue(value, index, arg, metadata): index: The index of the value in the function's argspec. arg: The name of the argument the value is being parsed for. metadata: Metadata about the function, typically from Fire decorators. + annotations: The type annotations dict from the function's argspec. Returns: value, parsed into the appropriate type for calling a function. """ parse_fn = parser.DefaultParseValue + has_custom_parse_fn = False # We check to see if any parse function from the fn metadata applies here. parse_fns = metadata.get(decorators.FIRE_PARSE_FNS) @@ -986,9 +991,22 @@ def _ParseValue(value, index, arg, metadata): if index is not None and 0 <= index < len(positional): parse_fn = positional[index] + has_custom_parse_fn = True elif arg in named: parse_fn = named[arg] + has_custom_parse_fn = True elif default is not None: parse_fn = default + has_custom_parse_fn = True + + if not has_custom_parse_fn and annotations and arg: + annotation = annotations.get(arg) + if inspectutils.IsStringAnnotation(annotation): + parsed = parse_fn(value) + if isinstance(parsed, str): + return parsed + if parsed is None and inspectutils.AllowsNone(annotation): + return None + return value return parse_fn(value) diff --git a/fire/fire_test.py b/fire/fire_test.py index 99b4a7c6f..27a606180 100644 --- a/fire/fire_test.py +++ b/fire/fire_test.py @@ -16,6 +16,7 @@ import os import sys +import typing from unittest import mock import fire @@ -178,6 +179,26 @@ def testFireAnnotatedArgs(self): self.assertEqual(fire.Fire(tc.Annotations, command=['double', '5']), 10) self.assertEqual(fire.Fire(tc.Annotations, command=['triple', '5']), 15) + def testFireStringHintedArgs(self): + def echo_version(ver: str): + return ver + + def echo_optional(ver: typing.Optional[str] = None): + return ver + + # Strings containing numeric or float representations are preserved. + self.assertEqual(fire.Fire(echo_version, command=['1.10']), '1.10') + self.assertEqual( + fire.Fire(echo_version, command=['--ver=1.10']), '1.10') + self.assertEqual(fire.Fire(echo_version, command=['123']), '123') + self.assertEqual(fire.Fire(echo_version, command=['True']), 'True') + + # Optional string annotations allow None while preserving strings. + self.assertIsNone(fire.Fire(echo_optional, command=['None'])) + self.assertEqual(fire.Fire(echo_optional, command=['1.10']), '1.10') + self.assertEqual( + fire.Fire(echo_optional, command=['--ver=1.10']), '1.10') + def testFireKeywordOnlyArgs(self): with self.assertRaisesFireExit(2): # Keyword arguments must be passed with flag syntax. diff --git a/fire/inspectutils.py b/fire/inspectutils.py index 17508e305..d0f016174 100644 --- a/fire/inspectutils.py +++ b/fire/inspectutils.py @@ -347,3 +347,61 @@ def IsCoroutineFunction(fn): return inspect.iscoroutinefunction(fn) except: # pylint: disable=bare-except return False + + +def IsStringAnnotation(annotation): + """Returns True if the annotation indicates a string type.""" + if annotation is None: + return False + if annotation is str or annotation == 'str': + return True + if isinstance(annotation, type) and issubclass(annotation, str): + return True + if getattr(annotation, '__name__', None) == 'Text': + return True + + origin = getattr(annotation, '__origin__', None) + if origin is not None: + args = getattr(annotation, '__args__', ()) + non_none_args = [a for a in args if a is not type(None)] + if len(non_none_args) == 1 and IsStringAnnotation(non_none_args[0]): + return True + + union_type = getattr(types, 'UnionType', None) + if union_type is not None and isinstance(annotation, union_type): + args = getattr(annotation, '__args__', ()) + non_none_args = [a for a in args if a is not type(None)] + if len(non_none_args) == 1 and IsStringAnnotation(non_none_args[0]): + return True + + if isinstance(annotation, str): + cleaned = annotation.replace(' ', '') + if cleaned in ('str', 'Optional[str]', 'Union[str,None]', 'Union[None,str]', + 'str|None', 'None|str', 'Text', 'Optional[Text]'): + return True + + return False + + +def AllowsNone(annotation): + """Returns True if the annotation permits None.""" + if annotation is None: + return True + origin = getattr(annotation, '__origin__', None) + if origin is not None: + args = getattr(annotation, '__args__', ()) + if type(None) in args: + return True + + union_type = getattr(types, 'UnionType', None) + if union_type is not None and isinstance(annotation, union_type): + args = getattr(annotation, '__args__', ()) + if type(None) in args: + return True + + if isinstance(annotation, str): + cleaned = annotation.replace(' ', '') + if any(pattern in cleaned for pattern in ('None', 'Optional')): + return True + return False + diff --git a/fire/inspectutils_test.py b/fire/inspectutils_test.py index 47de7e72e..53be60378 100644 --- a/fire/inspectutils_test.py +++ b/fire/inspectutils_test.py @@ -15,6 +15,7 @@ """Tests for the inspectutils module.""" import os +import typing from fire import inspectutils from fire import test_components as tc @@ -125,6 +126,23 @@ def testInfoNoDocstring(self): info = inspectutils.Info(tc.NoDefaults) self.assertEqual(info['docstring'], None, 'Docstring should be None') + def testIsStringAnnotation(self): + self.assertTrue(inspectutils.IsStringAnnotation(str)) + self.assertTrue(inspectutils.IsStringAnnotation('str')) + self.assertTrue(inspectutils.IsStringAnnotation(typing.Optional[str])) + self.assertTrue(inspectutils.IsStringAnnotation('Optional[str]')) + self.assertTrue(inspectutils.IsStringAnnotation('str | None')) + self.assertFalse(inspectutils.IsStringAnnotation(int)) + self.assertFalse(inspectutils.IsStringAnnotation('int')) + self.assertFalse(inspectutils.IsStringAnnotation(None)) + + def testAllowsNone(self): + self.assertFalse(inspectutils.AllowsNone(str)) + self.assertTrue(inspectutils.AllowsNone(typing.Optional[str])) + self.assertTrue(inspectutils.AllowsNone('Optional[str]')) + self.assertTrue(inspectutils.AllowsNone('str | None')) + self.assertTrue(inspectutils.AllowsNone(None)) + if __name__ == '__main__': testutils.main()