Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix target version inference. #3583

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

manueljacob
Copy link

Fixes #3581.

The old algorithm checked, for each target version 3.X, whether 3.X (which is roughly the same as 3.X.0) is contained in a version specifier that was modified from the requires-python project metadata to be less strict regarding patch versions. One problem of it was that, for a requires-python value of ">3.X" (which means the same as ">3.X.0"), it concluded that target version 3.X is not supported although Python versions >= 3.X.1 are included in the version specifier. I found the old approach hard to reason about and hard to fix.

To correctly check whether a target version 3.X is supported, the algorithm must check if 3.X.* overlaps the version specifier in the requires-python project metadata. Checking only specific versions (like 3.X.0) is not sufficient in general. The packaging library, which implements the logic for (PEP440-compatible) versions and version specifiers, doesn’t implement checking for overlap of two version specifiers, however.

The new algorithm works by converting the version specifiers to interval sets, which are then checked for overlap.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

…t present.

This clarifies that it is a deliberate choice instead of implementing the “correct” semantics.
The next commit will add a lot of test cases that will use this.
Fixes psf#3581.

The old algorithm checked, for each target version 3.X, whether 3.X (which is roughly the same as 3.X.0) is contained in a version specifier that was modified from the requires-python project metadata to be less strict regarding patch versions. One problem of it was that, for a requires-python value of ">3.X" (which means the same as ">3.X.0"), it concluded that target version 3.X is not supported although Python versions >= 3.X.1 are included in the version specifier. I found the old approach hard to reason about and hard to fix.

To correctly check whether a target version 3.X is supported, the algorithm must check whether 3.X.* overlaps the version specifier in the requires-python project metadata. Checking only specific versions (like 3.X.0) is not sufficient in general. The `packaging` library, which implements the logic for (PEP440-compatible) versions and version specifiers, doesn’t implement checking for overlap of two version specifiers, however.

The new algorithm works by converting the version specifiers to interval sets, which are then checked for overlap.
@ichard26
Copy link
Collaborator

Please ignore the failing diff-shades jobs. It's a known issue and not your fault. I'll have to look into why they're failing.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Feb 27, 2023

Hm, this seems a little complicated. I wonder if this is something that could be upstreamed to packaging.

Note that if black maintainers want something closer to the old approach, here's a diff that is 98% of the way there and passes almost all of the excellent tests in this PR (it fails ==3.3.0,==3.3.1, which is invalid / quite contrived and also maybe the "fail" behaviour is desirable. I also had to disable the versions that packaging emits DeprecationWarnings on):

diff --git a/src/black/files.py b/src/black/files.py
index 8c01311..8d55479 100644
--- a/src/black/files.py
+++ b/src/black/files.py
@@ -178,32 +178,41 @@ def parse_req_python_specifier(requires_python: str) -> Optional[List[TargetVers
     if not specifier_set:
         return None
 
-    target_version_map = {f"3.{v.value}": v for v in TargetVersion}
-    compatible_versions: List[str] = list(specifier_set.filter(target_version_map))
-    if compatible_versions:
-        return [target_version_map[v] for v in compatible_versions]
-    return None
+    compatible_versions = []
+    for v in TargetVersion:
+        if f"3.{v.value}.0" in specifier_set or f"3.{v.value}.9999" in specifier_set:
+            compatible_versions.append(v)
+    if not compatible_versions:
+        return None
+    return compatible_versions
 
 
 def strip_specifier_set(specifier_set: SpecifierSet) -> SpecifierSet:
-    """Strip minor versions for some specifiers in the specifier set.
+    """Strip patch versions for some specifiers in the specifier set.
 
     For background on version specifiers, see PEP 440:
     https://peps.python.org/pep-0440/#version-specifiers
     """
     specifiers = []
     for s in specifier_set:
-        if "*" in str(s):
+        if s.version.endswith(".*"):
+            parsed_version = Version(s.version[:-2])
+        else:
+            parsed_version = Version(s.version)
+
+        if len(parsed_version.release) <= 2:
             specifiers.append(s)
-        elif s.operator in ["~=", "==", ">=", "==="]:
-            version = Version(s.version)
-            stripped = Specifier(f"{s.operator}{version.major}.{version.minor}")
-            specifiers.append(stripped)
+            continue
+
+        short_version = ".".join(map(str, parsed_version.release[:2]))
+
+        if s.operator in ["==", ">=", "==="]:
+            specifiers.append(Specifier(f"{s.operator}{short_version}"))
         elif s.operator == ">":
-            version = Version(s.version)
-            if len(version.release) > 2:
-                s = Specifier(f">={version.major}.{version.minor}")
-            specifiers.append(s)
+            specifiers.append(Specifier(f">={short_version}"))
+        elif s.operator == "~=":
+            short_version = ".".join(map(str, parsed_version.release[:3]))
+            specifiers.append(Specifier(f"~={short_version}"))
         else:
             specifiers.append(s)

@manueljacob
Copy link
Author

Hm, this seems a little complicated. I wonder if this is something that could be upstreamed to packaging.

I tried several approaches to solve the problem fully, and all other approaches were both incomplete and more complicated. Mapping the specifiers to interval sets first made it much easier to reason about, even though the total amount of lines is longer. First I used some library for the interval set, but replaced it to simplify packaging (not the library that you are referring to ☺).

I don’t know if the packaging library would accept something like this. Checking for overlap of two version specifiers or emptyness of a version specifier is probably too niche. But I guess that if it can replace the current implementation for checking whether one version is contained in the specifier by something simpler, there would be a good chance.

Note that if black maintainers want something closer to the old approach, here's a diff that is 98% of the way there and passes almost all of the excellent tests in this PR (it fails ==3.3.0,==3.3.1, which is invalid / quite contrived and also maybe the "fail" behaviour is desirable. I also had to disable the versions that packaging emits DeprecationWarnings on):

After I implemented the approach in this PR, I also implemented a similar approach as in your diff, except that it checked for 3.X.0 or 3.X.99 (that shouldn’t matter, though) and got rid of strip_specifier_set completely. But at this time, I had already implemented the “full” solution, so I submitted only that.

I think that either approaches would be fine. But if it doesn’t recognize all Python patch versions, it should be transparent in the documentation. By the way, the reason why I found this bug was that the phrase “Black will to infer this from the project metadata” didn’t assure me, so I looked in the code.

@stinodego
Copy link
Contributor

Hm, this seems a little complicated.

I had the same initial thought when looking at this PR. A fix for the concrete issue (>3.7 doesn't include py37) could be much simpler as you've noted.

In the end, it's up to the black maintainers how they decide to weigh in the complexity here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inference of target versions from requires-python config is incorrect.
4 participants