Dual-option value arbitrage: first vs. last vs. most explicit policy

[kdeldycke]

WIP PR to address value arbitrage regressions from the 8.3.x series, as reported in #3403.

Explanation

Since Click inception, and up to the 8.2.x series, we did ctx.params[self.name] = value. Options sharing the same parameter ID were processed sequentially, and always overwriting each other. Which means the last write was always winning. This is an accident: a policy that was never enforced, explained or checked against.

In #3030, I changed consume_value/get_default so that the absence of a value became a real manageable UNSET state.

But then the naive dual-options tests started failing: #3030 (comment) . The less intrusive fix for these tests was implemented and made the first of a dual option win. Which changed the implicit behavior.

The underlying policy or principle governing the dual options arbitrage was never noticed, debated or documented. Because it's been a silent side-effect of the code for 12 years.

The need for arbitraging dual-options were more or less known since the unit tests that were flapping in #3030 existed. But they were never deep enough to highlight and enforce the implicit last write wins policy. The regression escaped us because the test were not covering boolean flag_values.

Solution: enforce policy

Re-introduce the last write win policy from pre-8.3.x series.

On tie, the most explicit source wins. For competing defaults, an explicit default beats an auto-derived one.

For the implementation, we can not rely on the natural processing order of dual-options. We need to actively manage and track their state so we have a place to explicitly implement the policies.

Plan

1. Add a shit-load of tests along several combinations of variations:
   • boolean and non-boolean flag_values
   • explicit and non-explicit default
   • explicit and auto-derived defaults
   • competing user-provided dual-options in different orders
   • duplicate competing options provided
   • 2+ competing options sharing the same parameter ID
2. Expect the tests to conform to the policy
3. Make all the tests happy
4. Document, explain and illustrate the policy in the public Sphinx documentation

Context

The lack of distinction between the absence of value and value of absence, that was fixed by #3030, also add semantic implication for defaults on boolean flags.

It was anticipated by @janluke in #1992 (comment) which proposed stricter validation.

Even @ldionne independently proposed to raise an error on the nonsensical flag_value=False + default=True combinations in its original #3111 report that started the chain.

In both cases I deferred these aggressive changes to a future 9.x release to not change Click internals too much.

[kdeldycke]

This PR is ready to be reviewed.

[davidism]

All this seems very thorough, I'm ok with it if you're sure it represents what you want to implement/maintain with this behavior. Once we document it more strongly like this, we're going to have a harder time changing it (which is fine, just be sure).

[kdeldycke]

All this seems very thorough, I'm ok with it if you're sure it represents what you want to implement/maintain with this behavior. Once we document it more strongly like this, we're going to have a harder time changing it (which is fine, just be sure).

Honestly I don't like the Click's API and behavior around options, flags, default and boolean flags. It was full of surprises and inconsistencies. I was a bit naive a year ago thinking I could fix it in a couple of PRs. But a year in and I'm still here discovering edge-cases and fixing them! 😅 But that's OK, I take full responsibility and will not abandon the project.

During that journey I updated my target: now my goal is to keep collecting edge-cases and real work examples, lockdown the use-cases in unit-tests, and implement work-around and special cases in the code. For me the value of Click is in the unit-tests. Which materialize the contract. Changing the contract and expectations is what cost us a lot in term of maintainance. So I will continue this kind of work for the whole 8.x series.

Then, once there is no more surprises, I will start a discussion with the community to change that contract. And clean up the public API. But that's for 9.x or later (or never, depending on the motivation, feedbacks and possible solutions).

As for that particular PR, the dual flag case is not new to me. I already had some suspicion from last year that their behavior was not deterministic and needed some active state management. It's just that I could not find a proper reproducer or see clearly the problem. #3403 report was a welcome reminder and an opportunity to think about this with a fresh mind.

So, to play it safe, I prefer to not changes the expectations of users for the 8.x series. Even at the price of complexity. And document all the subtleties. Click is way too entrenched in the Python eco-system and we still have a lot of work to do.