Fix reconciliation of envvar with default, flag_value and type parameters for flag options

CHANGES.rst

@@ -1,5 +1,14 @@
 .. currentmodule:: click
 
+Version 8.2.2
+-------------
+
+Unreleased
+
+-   Fix reconciliation of `default`, `flag_value` and `type` parameters for
+    flag options, as well as parsing and normalization of environment variables.
+    :issue:`2952` :pr:`2956`
+
 Version 8.2.1
 -------------
 

docs/options.md

@@ -368,6 +368,31 @@ If a list is passed to `envvar`, the first environment variable found is picked.
 
 ```
 
+Variable names are:
+ - [Case-insensitive on Windows but not on other platforms](https://github.com/python/cpython/blob/aa9eb5f757ceff461e6e996f12c89e5d9b583b01/Lib/os.py#L777-L789).
+ - Not stripped of whitespaces and should match the exact name provided to the `envvar` argument.
+
+For flag options, there is two concepts to consider: the activation of the flag driven by the environment variable, and the value of the flag if it is activated.
+
+The environment variable need to be interpreted, because values read from them are always strings. We need to transform these strings into boolean values that will determine if the flag is activated or not.
+
+Here are the rules used to parse environment variable values for flag options:
+   - `true`, `1`, `yes`, `on`, `t`, `y` are interpreted as activating the flag
+   - `false`, `0`, `no`, `off`, `f`, `n` are interpreted as deactivating the flag
+   - The presence of the environment variable without value is interpreted as deactivating the flag
+   - Empty strings are interpreted as deactivating the flag
+   - Values are case-insensitive, so the `True`, `TRUE`, `tRuE` strings are all activating the flag
+   - Values are stripped of leading and trailing whitespaces before being interpreted, so the `" True "` string is transformed to `"true"` and so activates the flag
+   - If the flag option has a `flag_value` argument, passing that value in the environment variable will activate the flag, in addition to all the cases described above
+   - Any other value is interpreted as deactivating the flag
+
+.. caution::
+    For boolean flags with a pair of values, the only recognized environment variable is the one provided to the `envvar` argument.
+
+    So an option defined as `--flag\--no-flag`, with a `envvar="FLAG"` parameter, there is no magical `NO_FLAG=<anything>` variable that is recognized. Only the `FLAG=<anything>` environment variable is recognized.
+
+Once the status of the flag has been determine to be activated or not, the `flag_value` is used as the value of the flag if it is activated. If the flag is not activated, the value of the flag is set to `None` by default.
+
 ## Multiple Options from Environment Values
 
 As options can accept multiple values, pulling in such values from

src/click/core.py

@@ -2554,7 +2554,6 @@ def __init__(
         if help:
             help = inspect.cleandoc(help)
 
-        default_is_missing = "default" not in attrs
         super().__init__(
             param_decls, type=type, multiple=multiple, deprecated=deprecated, **attrs
         )
@@ -2588,47 +2587,54 @@ def __init__(
         self._flag_needs_value = self.prompt is not None and not self.prompt_required
 
         if is_flag is None:
+            # Implicitly a flag because flag_value was set.
             if flag_value is not None:
-                # Implicitly a flag because flag_value was set.
                 is_flag = True
+            # Not a flag, but when used as a flag it shows a prompt.
             elif self._flag_needs_value:
-                # Not a flag, but when used as a flag it shows a prompt.
                 is_flag = False
-            else:
-                # Implicitly a flag because flag options were given.
-                is_flag = bool(self.secondary_opts)
+            # Implicitly a flag because flag options were given.
+            elif self.secondary_opts:
+                is_flag = True
         elif is_flag is False and not self._flag_needs_value:
             # Not a flag, and prompt is not enabled, can be used as a
             # flag if flag_value is set.
             self._flag_needs_value = flag_value is not None
 
         self.default: t.Any | t.Callable[[], t.Any]
 
-        if is_flag and default_is_missing and not self.required:
-            if multiple:
-                self.default = ()
-            else:
-                self.default = False
+        if is_flag:
+            # Set missing default for flags if not explicitly required or prompted.
+            if self.default is None and not self.required and not self.prompt:
+                if multiple:
+                    self.default = ()
+                else:
+                    self.default = False
 
-        if is_flag and flag_value is None:
-            flag_value = not self.default
+            if flag_value is None:
+                # A boolean flag presence in the command line is enough to set
+                # the value: to the default if it is not blank, or to True
+                # otherwise.
+                flag_value = self.default if self.default else True
 
         self.type: types.ParamType
         if is_flag and type is None:
             # Re-guess the type from the flag value instead of the
             # default.
             self.type = types.convert_type(None, flag_value)
 
-        self.is_flag: bool = is_flag
-        self.is_bool_flag: bool = is_flag and isinstance(self.type, types.BoolParamType)
+        self.is_flag: bool = bool(is_flag)
+        self.is_bool_flag: bool = bool(
+            is_flag and isinstance(self.type, types.BoolParamType)
+        )
         self.flag_value: t.Any = flag_value
 
         # Counting
         self.count = count
         if count:
             if type is None:
                 self.type = types.IntRange(min=0)
-            if default_is_missing:
+            if self.default is None:
                 self.default = 0
 
         self.allow_from_autoenv = allow_from_autoenv
@@ -2918,8 +2924,8 @@ def get_default(
         # value as default.
         if self.is_flag and not self.is_bool_flag:
             for param in ctx.command.params:
-                if param.name == self.name and param.default:
-                    return t.cast(Option, param).flag_value
+                if param.name == self.name and param.default is not None:
+                    return t.cast(Option, param).default
 
             return None
 
@@ -2959,11 +2965,21 @@ def prompt_for_value(self, ctx: Context) -> t.Any:
         )
 
     def resolve_envvar_value(self, ctx: Context) -> str | None:
+        """Find which environment variable to read for this option and return
+        its value.
+
+        Returns the value of the environment variable if it exists, or ``None``
+        if it does not.
+
+        .. caution::
+
+            The raw value extracted from the environment is not normalized and
+            is returned as-is. Any normalization or reconciation with the
+            option's type should happen later.
+        """
         rv = super().resolve_envvar_value(ctx)
 
         if rv is not None:
-            if self.is_flag and self.flag_value:
-                return str(self.flag_value)
             return rv
 
         if (
@@ -2980,18 +2996,32 @@ def resolve_envvar_value(self, ctx: Context) -> str | None:
         return None
 
     def value_from_envvar(self, ctx: Context) -> t.Any | None:
-        rv: t.Any | None = self.resolve_envvar_value(ctx)
+        """Normalize the value from the environment variable, if it exists."""
+        rv: str | None = self.resolve_envvar_value(ctx)
 
         if rv is None:
             return None
 
+        # Non-boolean flags are more liberal in what they accept. But a flag being a
+        # flag, its envvar value still needs to analyzed to determine if the flag is
+        # activated or not.
+        if self.is_flag and not self.is_bool_flag:
+            # If the flag_value is set and match the envvar value, return it
+            # directly.
+            if self.flag_value is not None and rv == self.flag_value:
+                return self.flag_value
+            # Analyze the envvar value as a boolean to know if the flag is
+            # activated or not.
+            return types.BoolParamType.str_to_bool(rv)
+
+        # Split the envvar value if it is allowed to be repeated.
         value_depth = (self.nargs != 1) + bool(self.multiple)
-
         if value_depth > 0:
-            rv = self.type.split_envvar_value(rv)
-
+            multi_rv = self.type.split_envvar_value(rv)
             if self.multiple and self.nargs != 1:
-                rv = batch(rv, self.nargs)
+                multi_rv = batch(multi_rv, self.nargs)  # type: ignore[assignment]
+
+            return multi_rv
 
         return rv
 
@@ -3011,6 +3041,17 @@ def consume_value(
                 value = self.flag_value
                 source = ParameterSource.COMMANDLINE
 
+        # A flag which is activated and has a flag_value set, should returns
+        # the latter, unless the value comes from the explicitly sets default.
+        elif (
+            self.is_flag
+            and value is True
+            and not self.is_bool_flag
+            and self.flag_value is not None
+            and source is not ParameterSource.DEFAULT
+        ):
+            value = self.flag_value
+
         elif (
             self.multiple
             and value is not None

src/click/types.py

@@ -661,23 +661,67 @@ def _clamp(self, bound: float, dir: t.Literal[1, -1], open: bool) -> float:
 class BoolParamType(ParamType):
     name = "boolean"
 
-    def convert(
-        self, value: t.Any, param: Parameter | None, ctx: Context | None
-    ) -> t.Any:
-        if value in {False, True}:
-            return bool(value)
+    bool_states: dict[str, bool] = {
+        "1": True,
+        "0": False,
+        "yes": True,
+        "no": False,
+        "true": True,
+        "false": False,
+        "on": True,
+        "off": False,
+        "t": True,
+        "f": False,
+        "y": True,
+        "n": False,
+        # Absence of value is considered False.
+        "": False,
+    }
+    """A mapping of string values to boolean states.
+
+    Mapping is inspired by :py:attr:`configparser.ConfigParser.BOOLEAN_STATES`
+    and extends it.
+
+    .. caution::
+        String values are lower-cased, as the ``str_to_bool`` comparison function
+        below is case-insensitive.
+
+    .. warning::
+        The mapping is not exhaustive, and does not cover all possible boolean strings
+        representations. It will remains as it is to avoid endless bikeshedding.
+
+        Future work my be considered to make this mapping user-configurable from public
+        API.
+    """
 
-        norm = value.strip().lower()
+    @staticmethod
+    def str_to_bool(value: str | bool) -> bool | None:
+        """Convert a string to a boolean value.
 
-        if norm in {"1", "true", "t", "yes", "y", "on"}:
-            return True
+        If the value is already a boolean, it is returned as-is. If the value is a
+        string, it is stripped of whitespaces and lower-cased, then checked against
+        the known boolean states pre-defined in the `BoolParamType.bool_states` mapping
+        above.
 
-        if norm in {"0", "false", "f", "no", "n", "off"}:
-            return False
+        Returns `None` if the value does not match any known boolean state.
+        """
+        if isinstance(value, bool):
+            return value
+        return BoolParamType.bool_states.get(value.strip().lower())
 
-        self.fail(
-            _("{value!r} is not a valid boolean.").format(value=value), param, ctx
-        )
+    def convert(
+        self, value: t.Any, param: Parameter | None, ctx: Context | None
+    ) -> bool:
+        normalized = self.str_to_bool(value)
+        if normalized is None:
+            self.fail(
+                _(
+                    "{value!r} is not a valid boolean. Recognized values: {states}"
+                ).format(value=value, states=", ".join(sorted(self.bool_states))),
+                param,
+                ctx,
+            )
+        return normalized
 
     def __repr__(self) -> str:
         return "BOOL"

tests/test_arguments.py

@@ -198,19 +198,6 @@ def cmd(arg):
         assert result.return_value == expect
 
 
-def test_envvar_flag_value(runner):
-    @click.command()
-    # is_flag is implicitly true
-    @click.option("--upper", flag_value="upper", envvar="UPPER")
-    def cmd(upper):
-        click.echo(upper)
-        return upper
-
-    # For whatever value of the `env` variable, if it exists, the flag should be `upper`
-    result = runner.invoke(cmd, env={"UPPER": "whatever"})
-    assert result.output.strip() == "upper"
-
-
 def test_nargs_envvar_only_if_values_empty(runner):
     @click.command()
     @click.argument("arg", envvar="X", nargs=-1)

tests/test_basic.py

@@ -222,17 +222,21 @@ def cli(on):
     assert result.return_value is expect
 
 
-@pytest.mark.parametrize("default", [True, False])
-@pytest.mark.parametrize(("args", "expect"), [(["--f"], True), ([], False)])
+@pytest.mark.parametrize(
+    ("default", "args", "expect"),
+    (
+        (True, ["--f"], True),
+        (True, [], True),
+        (False, ["--f"], True),
+        (False, [], False),
+    ),
+)
 def test_boolean_flag(runner, default, args, expect):
     @click.command()
     @click.option("--f", is_flag=True, default=default)
     def cli(f):
         return f
 
-    if default:
-        expect = not expect
-
     result = runner.invoke(cli, args, standalone_mode=False)
     assert result.return_value is expect
 

tests/test_imports.py

@@ -27,29 +27,30 @@ def tracking_import(module, locals=None, globals=None, fromlist=None,
 
 ALLOWED_IMPORTS = {
     "__future__",
-    "weakref",
-    "os",
-    "struct",
+    "codecs",
     "collections",
     "collections.abc",
-    "sys",
+    "configparser",
     "contextlib",
+    "datetime",
+    "enum",
+    "errno",
+    "fcntl",
     "functools",
-    "stat",
-    "re",
-    "codecs",
+    "gettext",
     "inspect",
-    "itertools",
     "io",
+    "itertools",
+    "os",
+    "re",
+    "shutil",
+    "stat",
+    "struct",
+    "sys",
     "threading",
-    "errno",
-    "fcntl",
-    "datetime",
-    "enum",
-    "typing",
     "types",
-    "gettext",
-    "shutil",
+    "typing",
+    "weakref",
 }
 
 if WIN: