From 76b20d738e190801edcf5b63ac2c4ab575a19e24 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 10 Jun 2020 12:17:28 +0300
Subject: [PATCH] Deprecate requirements format "base>=1.0[extra]"

This requirements format does not conform to PEP-508. Currently the
extras specified like this work by accident (because _strip_extras()
also parses them). The version checks end up being done with a
misparsed version '1.0[extra]' -- this is not changed in this commit.

Add deprecation warning and fix the corresponding resolver test. Add a
command line test.

Note that we really only check that the Requirement has SpecifierSet
with a specifier that ends in a ']'. A valid version number cannot
contain ']' and no wheels currently on pypi have versions ending in ']'.
---
 news/8288.removal                       |  1 +
 src/pip/_internal/req/constructors.py   | 12 +++++++++++
 tests/functional/test_install_extras.py | 21 +++++++++++++++++++
 tests/functional/test_new_resolver.py   | 28 +++++++++++++++++++++++--
 4 files changed, 60 insertions(+), 2 deletions(-)
 create mode 100644 news/8288.removal

diff --git a/news/8288.removal b/news/8288.removal
new file mode 100644
index 000000000..830d91aab
--- /dev/null
+++ b/news/8288.removal
@@ -0,0 +1 @@
+Add deprecation warning for invalid requirements format "base>=1.0[extra]"
diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py
index 46b1daa90..857f7fff5 100644
--- a/src/pip/_internal/req/constructors.py
+++ b/src/pip/_internal/req/constructors.py
@@ -23,6 +23,7 @@ from pip._internal.models.link import Link
 from pip._internal.models.wheel import Wheel
 from pip._internal.pyproject import make_pyproject_path
 from pip._internal.req.req_install import InstallRequirement
+from pip._internal.utils.deprecation import deprecated
 from pip._internal.utils.filetypes import ARCHIVE_EXTENSIONS
 from pip._internal.utils.misc import is_installable_dir, splitext
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
@@ -370,6 +371,17 @@ def parse_req_from_line(name, line_source):
             if add_msg:
                 msg += '\nHint: {}'.format(add_msg)
             raise InstallationError(msg)
+        else:
+            # Deprecate extras after specifiers: "name>=1.0[extras]"
+            # This currently works by accident because _strip_extras() parses
+            # any extras in the end of the string and those are saved in
+            # RequirementParts
+            for spec in req.specifier:
+                spec_str = str(spec)
+                if spec_str.endswith(']'):
+                    msg = "Extras after version '{}'.".format(spec_str)
+                    replace = "moving the extras before version specifiers"
+                    deprecated(msg, replacement=replace, gone_in="21.0")
     else:
         req = None
 
diff --git a/tests/functional/test_install_extras.py b/tests/functional/test_install_extras.py
index ba03f1e4a..d70067b6b 100644
--- a/tests/functional/test_install_extras.py
+++ b/tests/functional/test_install_extras.py
@@ -104,6 +104,27 @@ def test_nonexistent_options_listed_in_order(script, data):
     assert matches == ['nonexistent', 'nope']
 
 
+def test_install_deprecated_extra(script, data):
+    """
+    Warn about deprecated order of specifiers and extras.
+
+    Test uses a requirements file to avoid a testing issue where
+    the specifier gets interpreted as shell redirect.
+    """
+    script.scratch_path.joinpath("requirements.txt").write_text(
+        "requires_simple_extra>=0.1[extra]"
+    )
+    simple = script.site_packages / 'simple'
+
+    result = script.pip(
+        'install', '--no-index', '--find-links=' + data.find_links,
+        '-r', script.scratch_path / 'requirements.txt', expect_stderr=True,
+    )
+
+    result.did_create(simple)
+    assert ("DEPRECATION: Extras after version" in result.stderr)
+
+
 def test_install_special_extra(script):
     # Check that uppercase letters and '-' are dealt with
     # make a dummy project
diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py
index 202a4b2b4..932918033 100644
--- a/tests/functional/test_new_resolver.py
+++ b/tests/functional/test_new_resolver.py
@@ -200,8 +200,6 @@ def test_new_resolver_ignore_dependencies(script):
     [
         "base[add]",
         "base[add] >= 0.1.0",
-        # Non-standard syntax. To deprecate, see pypa/pip#8288.
-        "base >= 0.1.0[add]",
     ],
 )
 def test_new_resolver_installs_extras(tmpdir, script, root_dep):
@@ -228,6 +226,32 @@ def test_new_resolver_installs_extras(tmpdir, script, root_dep):
     assert_installed(script, base="0.1.0", dep="0.1.0")
 
 
+def test_new_resolver_installs_extras_deprecated(tmpdir, script):
+    req_file = tmpdir.joinpath("requirements.txt")
+    req_file.write_text("base >= 0.1.0[add]")
+
+    create_basic_wheel_for_package(
+        script,
+        "base",
+        "0.1.0",
+        extras={"add": ["dep"]},
+    )
+    create_basic_wheel_for_package(
+        script,
+        "dep",
+        "0.1.0",
+    )
+    result = script.pip(
+        "install", "--use-feature=2020-resolver",
+        "--no-cache-dir", "--no-index",
+        "--find-links", script.scratch_path,
+        "-r", req_file,
+        expect_stderr=True
+    )
+    assert "DEPRECATION: Extras after version" in result.stderr
+    assert_installed(script, base="0.1.0", dep="0.1.0")
+
+
 def test_new_resolver_installs_extras_warn_missing(script):
     create_basic_wheel_for_package(
         script,