pep508: add requires-python simplification/complexification to MarkerTree
#7134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I split this change into its own commit because I'm hoping it crystalizes what it means when we say "a `MarkerTree` has hidden state." That is, it isn't so much that there is some explicit member of a `MarkerTree` that is omitted, but rather, the lower and upper version bounds on `python_full_version` are are rewritten as "unbounded" when traversing the ADD for display. We will actually retain this functionality, but rejigger it so that it's explicit when we do this. In particular, this simplification has been problematic for us because it fundamentally changes the truth tables of a marker expression *unless* you are extremely careful to interpret it only under the original context in which it was simplified. This is quite difficult to do generally, and in prior work in #6268, we completed a refactor where we worked around this type of simplification and moved it to the edges of uv. In subsequent commits, we'll re-implement this form of simplification as a more explicit step.
This adds new routines to `MarkerTree` for "simplifying" and "complexifying" a tree with respect to lower and upper Python version bounds. In effect, "simplifying" a marker is what you do when you write it to a lock file. Namely, since `uv.lock` includes a `requires-python` bound at the top, one can say that it acts as a bound on the supported Python versions. That is, it establishes a context in which one can assume that bound is true. Therefore, the markers we write can be simplified using this assumption. The reverse is "complexifying" a marker, and it's what you do when you read a marker from the lock file. Namely, once a marker is read, it can be very difficult in code to keep the corresponding requires-python context from the lock file. If you lose track of it and decide to operate on the "simplified" marker, then it's trivial for that to produce an incorrect result.
This finally gets rid of our hack for working around "hidden" state. We no longer do a roundtrip marker serialization and deserialization just to avoid the hidden state.
BurntSushi
force-pushed
the
ag/remove-hidden-state
branch
from
a58a1a6
to
f37c755
Compare
charliermarsh
approved these changes
Sep 7, 2024
There was a problem hiding this comment.
This generally makes sense to me, though may want @ibraheemdev to look too.
|
I've been chatting loosely with @ibraheemdev about this, so I feel pretty good about this being a generally good approach. (Although I do think we discussed some alternative implementations of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #6268, we refactored how we handled
requires-python. Beforethat PR, we simplified markers inside resolution in lock-step with
requires-python version narrowing. But this turns out to be quite hard
to do correctly. After that PR, we now generally only simplify markers
when we write the lock file.
So for example, if a package has this marker:
And we have
requires-python = ">=3.8", then its simplified form is this:The problem is that if we use the simplified form in a place where we
shouldn't, perhaps in a place where we don't handle the context of
requires-pythoncorrectly or if it changes, then the marker can leadto incorrect results.
So #6268 fixed this, but there was a hiccup: previously, in order to
actually do the simplification, we did this:
MarkerTreeto "restrict" any Python version expressionssuch that they were limited to the intersection of what was in the
marker and whas was provided by
requires-python. 2. When printinga
MarkerTree, we specifically set the lower and upper bounds tonegative and positive infinity, respectively.
The end result is that we had a simplified marker, but it relied on a
subtle trick during printing to work. This made it difficult to reason
about markers since they might internally have finite lower/upper
bounds, but when printed, they had non-finite lower/upper bounds.
In order to access the simplified marker, #6268 would run the
"restrict" operation, print the marker as a string and then parse it.
This removed any "hidden" state and made the internal representation of
the marker match its printed form.
But this is bad juju to do, and having the hidden state when we don't
need it any more makes things very difficult to reason about. So in
this PR, we essentially port the old restrict+print approach to one
routine, and then also add a dedicated routine to do the reverse
operation as well (instead of relying on marker conjunction, as we did
before).