uv-pep508: add more methods for simplifying `extra`-related expressions by BurntSushi · Pull Request #9469 · astral-sh/uv

BurntSushi · 2024-11-27T13:35:45Z

In the course of working on #9289, I've had to devise some additions to
our markers. While we are still staying strictly compatible with the
PEP 508 format, we will be abusing the extra expression to carry a
lot more information.

Specifically, we want the following additional operations:

Simplify extra != 'foo'
Remove all extra expressions
Remove everything except extra expressions

My work on #9289 requires all of these (which will be in a future in
PR).

This PR also allows the if_not_else Clippy lint. Specifically, the
lint was flagging this code:

        if !matches!(node.var, Variable::Extra(_)) {
            i = NodeId::FALSE;
            for child in node.children.nodes() {
                i = self.or(i, child.negate(parent));
            }
            if i.is_true() {
                return NodeId::TRUE;
            }
            self.only_extras(i)
        } else {
            // Restrict all nodes recursively.
            let children = node.children.map(i, |node| self.only_extras(node));
            self.create_node(node.var.clone(), children)
        }

It wants you to flip the if and else bodies and un-negate the
condition. But I wanted to keep it as-written because I find it easier
to read. And I think this applies more generally. I think there are
some cases where un-negating a condition can be easier to read (in
particular, double negatives), but I don't think it applies universally
to the point where we should lint against it.

konstin · 2024-11-27T17:22:05Z

crates/uv-pep508/src/marker/algebra.rs

+            i = NodeId::FALSE;
+            for child in node.children.nodes() {
+                i = self.or(i, child.negate(parent));
+            }


I'm trying to grok what this function does. What would it do with ((os_name == ... and extra == foo) or (sys_platform == ... and extra != foo))?

It would give you os_name == ... or sys_platform == ... back. Basically, it removes all extra nodes by assuming they are true.

I added this as an example to the docs of this function.

This lint fires when an `if` negates the condition. The lint instead suggests that the condition should be un-negated and the `if` and `else` bodies flipped. I find this to be a pretty annoying lint, because sometimes I want to keep the order of the `if` and `else` bodies as-is, and not make it subject to whether the primary condition is negated or not. Sometimes it's for linear scanning reasons (where the `if` block is smaller), and sometimes it's for "code parallelism" reasons (i.e., this block of code looks like another block of code intentionally). Moreover, I don't think the benefits of this lint are very big. I do agree that sometimes negating a conditional can make it harder to read (e.g., a double negative), but it's nowhere near a universal truth enough to ban it outright.

In the course of working on #9289, I've had to devise some additions to our markers. While we are still staying strictly compatible with the PEP 508 format, we will be abusing the `extra` expression to carry a lot more information. Specifically, we want the following additional operations: * Simplify `extra != 'foo'` * Remove all extra expressions * Remove everything except extra expressions My work on #9289 requires all of these (which will be in a future in PR).

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.5` -> `0.5.6` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.6`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#056) [Compare Source](astral-sh/uv@0.5.5...0.5.6) ##### Enhancements - Add `--dry-run` to `uv pip uninstall` ([#9557](astral-sh/uv#9557)) - Allow `--constraints` and `--overrides` in `uv tool install` ([#9547](astral-sh/uv#9547)) - Display removed Python executables on uninstall ([#9459](astral-sh/uv#9459)) - Warn when keyring has no password for `uv publish` ([#8827](astral-sh/uv#8827)) - Add suggested action when `.python-version` pin is incompatible with the project ([#9590](astral-sh/uv#9590)) - Improve error messages for mismatches in `tool.uv.sources` ([#9482](astral-sh/uv#9482)) - Use constraints in trace rather than irrelevant `requires-python` ([#9529](astral-sh/uv#9529)) ##### Preview features - Add `uv python install --default` ([#8650](astral-sh/uv#8650)) - Fix Python executable installation when multiple patch versions are requested ([#9607](astral-sh/uv#9607)) - Build backend: Revamp `include` / `exclude` ([#9525](astral-sh/uv#9525)) - Build backend: Add fast path ([#9556](astral-sh/uv#9556)) - Build backend: Add functions to collect file list ([#9602](astral-sh/uv#9602)) - Build backend: Default excludes ([#9552](astral-sh/uv#9552)) - Build backend: Refactoring before list ([#9558](astral-sh/uv#9558)) - Build backend: Warn when visiting over 10k files ([#9523](astral-sh/uv#9523)) ##### Configuration - Make `check-url` available in configuration files ([#9032](astral-sh/uv#9032)) ##### Performance - Avoid adding non-extra package with extra dependencies ([#9540](astral-sh/uv#9540)) - Avoid cloning `String` in marker evaluation ([#9598](astral-sh/uv#9598)) ##### Rust API - `uv-pep508`: Add more methods for simplifying `extra`-related expressions ([#9469](astral-sh/uv#9469)) ##### Bug fixes - Allow `file:` URLs to include package names ([#9493](astral-sh/uv#9493)) - Avoid using IDs across PubGrub states ([#9538](astral-sh/uv#9538)) - Consistently enforce requested-vs.-built metadata when retrieving wheels ([#9484](astral-sh/uv#9484)) - Do not show empty version specifier in `uv tool list` ([#9605](astral-sh/uv#9605)) - Include Git member information when getting metadata from cache ([#9388](astral-sh/uv#9388)) - Include base installation directory in uv run PATH ([#9585](astral-sh/uv#9585)) - Insert backslash when appending to system drive ([#9488](astral-sh/uv#9488)) - Normalize paths when lowering Git dependencies ([#9595](astral-sh/uv#9595)) - Omit origin when comparing requirements ([#9570](astral-sh/uv#9570)) - Override `manylinux_compatible` with `--python-platform` ([#9526](astral-sh/uv#9526)) - Pass extra when evaluating lockfile markers ([#9539](astral-sh/uv#9539)) - Propagate markers for recursive extras in resolver ([#9509](astral-sh/uv#9509)) - Respect path dependencies within Git dependencies ([#9594](astral-sh/uv#9594)) - Support recursive extras with marker in `pip compile -r pyproject.toml` ([#9535](astral-sh/uv#9535)) - Don't emit unpinned warning for proxy packages ([#9497](astral-sh/uv#9497)) - Fix `--refresh-package` flag mentioned as `--refresh-dependency` ([#9486](astral-sh/uv#9486)) - Handle Windows AV/EDR file locks during script installations ([#9543](astral-sh/uv#9543)) - Re-enable conflicting extra/group tests and fix regression from [#9540](astral-sh/uv#9540) ([#9582](astral-sh/uv#9582)) ##### Documentation - Add missing word to docs for `run.md` ([#9527](astral-sh/uv#9527)) - Add policies reference section and license document ([#9367](astral-sh/uv#9367)) - Fix typo in entry point docs ([#9491](astral-sh/uv#9491)) - Fix up version in prior uninstall instructions ([#9485](astral-sh/uv#9485)) - Mention `uv pip` behavior in build system note ([#9586](astral-sh/uv#9586)) - Update build failures document ([#9584](astral-sh/uv#9584)) - Correct wording for multiple sources section ([#9504](astral-sh/uv#9504)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

BurntSushi requested review from ibraheemdev and konstin November 27, 2024 13:35

BurntSushi force-pushed the ag/pep508-additions branch from 7fd1cb9 to c43b448 Compare November 27, 2024 15:35

konstin reviewed Nov 27, 2024

View reviewed changes

BurntSushi force-pushed the ag/pep508-additions branch 2 times, most recently from c6a306f to db0cd81 Compare December 2, 2024 13:33

BurntSushi force-pushed the ag/pep508-additions branch from db0cd81 to 79414a2 Compare December 2, 2024 13:47

BurntSushi merged commit 659e86e into main Dec 2, 2024

BurntSushi deleted the ag/pep508-additions branch December 2, 2024 14:09

BrewTestBot mentioned this pull request Dec 3, 2024

uv 0.5.6 Homebrew/homebrew-core#199944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

uv-pep508: add more methods for simplifying `extra`-related expressions#9469

uv-pep508: add more methods for simplifying `extra`-related expressions#9469
BurntSushi merged 2 commits intomainfrom
ag/pep508-additions

BurntSushi commented Nov 27, 2024

Uh oh!

konstin Nov 27, 2024

Uh oh!

BurntSushi Dec 2, 2024

Uh oh!

BurntSushi Dec 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

BurntSushi commented Nov 27, 2024

Uh oh!

konstin Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

BurntSushi Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

BurntSushi Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants