Pip accepts requirements with extras that don't conform to PEP 508

[pfmoore]

Description
See #8285. The form setuptools_scm>=3.5[toml] does not conform to PEP 508. Pip accepts it, presumably for historical reasons.

We should deprecate and ultimately reject this form.

Expected behavior
Pip should only accept standard (PEP 508) formats for requirements.

How to Reproduce
See the linked issue.

[jku]

I'm trying to get familiar with pip sources and can take a look at this.

I'll probably need a bit of advice along the way though: I've looked at this enough to see that the requirements/specifier handling is a bit redundant: it gets parsed several times with slightly different results and it's not 100% clear which of these parsing passes should check for this. More importantly it looks like the non-deprecated form base[extra]>=1.0 gets mis-parsed in two places...

I'll make a potential fix for just this issue and then comment on the other weirdness once I understand it better.

[jku]

Actually commenting now on the strange behaviour I'm seeing so I remember later:

• parse_req_from_line() returns no extras for input like "base[extra]>=1.0" in the returned RequirementParts

>>> req_parts = parse_req_from_line("base[extra]>=1.0", None)
>>> print (req_parts.requirement.extras, req_parts.extras)
{'extra'} set()
>>> req_parts = parse_req_from_line("base>=1.0[extra]", None)
>>> print (req_parts.requirement.extras, req_parts.extras)
set() {'extra'}

• that RequirementParts is used to build an InstallRequirement: that will however end up having extras (even if it was explicitly given an empty extras set as input)
• parse_req_from_line() uses _strip_extras() that is clearly buggy in context of "base[extra]>=1.0" input -- maybe this shouldn't be executed for plain requirement specifiers?

[jku]

I think the major confusion with the methods in question is that there seems to be undocumented redundancy:

• what does it mean when a RequirementParts or InstallRequirement has both "extras" and "requirement.extras"
• what does it mean when a RequirementParts or InstallRequirement has both "markers" and "requirement.marker"

[jku]

So InstallRequirements does this:

    if extras:
        self.extras = extras
    elif req:
        self.extras = {
            pkg_resources.safe_extra(extra) for extra in req.extras
        }
    else:
        self.extras = set()

I still can't find an explanation -- why are there two ways to define extras?

EDIT: after lots of git blame I think this is the origin of multiple ways to give extras to InstallRequirement: #1236. Still don't understand why.

[uranusjr]

Man, I think we should just rewrite RequirementParts and get rid of parts.extras and parts.marker entirely. It is only used by parse_req_from_line and parse_req_from_editable, both are well-scoped and can be replaced safely.

I am not entirely sure, but assume most of the extras and marker cruft here are due to history baggage. InstallRequirement and its various constructors predate PEP 508 and the packaging library. pip had its own parsing logic at the time, and was refactored to use packaging afterwards, but a lot of the logic lingers to avoid breaking stuff (and because nobody really have much time to do the cleanups).

[pradyunsg]

I'm on board. We will have to go through a deprecation cycle though, since this is changing user-facing behavior.

[jku]

Right, that's the feel I got from the code as well -- although for a newcomer parse_req_from_line() is not well specified: refactoring anything seems scary because the input can be so many different things...

[uranusjr]

Trust me, I feel as uneasy as you are to the prospect. I feel less afriad of breaking things with experience, but still break as many things as when I started.

[pfmoore]

+1 from me too. And @jku don't worry, those nervous feelings are the sign of someone who knows what they are doing! The people I'd be worried about are people who look at this code and think it all seems reasonably straightforward 🙂

On a more serious note, please feel free to ask if you're unsure. And trust your instincts - they've been good so far.

[jku]

For others information: I have some ideas and might try the refactoring at some point but for now I'm not working on it -- feel free to have a go.

I'll have another go with the PR #8424 now though: getting this specific misparsing marked as deprecated will be useful in any case.