Issue with "matches()" logic

[Toflar]

I think it was also an issue in https://github.com/composer/semver/pull/86 so pinging @jderusse here as well.

I've encountered this issue while working on https://github.com/composer/composer/pull/8850 and it's a show stopper. I'm trying to find out if other package versions need to be loaded but that doesn't work reliably. The problem is the matches() method. Checkout the following example:

<?php

require_once 'vendor/autoload.php';

use Composer\Semver\VersionParser;

$versionParser = new VersionParser();
$exactConstraint = $versionParser->parseConstraints('1.0.0');
$rangeConstraint = $versionParser->parseConstraints('1.*');

var_dump($exactConstraint->matches($rangeConstraint)); // true
var_dump($rangeConstraint->matches($exactConstraint)); // true

So if a package required a dependency in exactly 1.0.0 we would never load the other 1.* versions because it would always match :(

The question is, what does matches() really mean and how can we solve this issue?

[jderusse]

I'm not sure to understand what you mean. if a package require 1.0, why would you load the other 1.*?

[Toflar]

That‘s part of the optimization process. The poolbuilder is not responsible for any decision making. That‘s the SAT solver. But instead of loading all available versions, only the once referenced anywhere should be loaded.

[jderusse]

couldn't it merge (return a conjunctive MultiConstraint) of both constraints?

[Toflar]

If we did that, it would end up in an endless loop. I need a way to determine whether we have to trigger reloading a certian package or not. And

$newConstraint = MultiConstraint::create(array($currentConstraint, $requestedConstraint), false);
if ((string) $currentConstraint === (string) $newConstraint) {
    // no need to load again, done
}

is too fragile at the moment :)

[Seldaek]

I guess the problem is that matches kinda means A intersects B in some way, while what you'd need to know is whether A includes all of B, then B can be ignored. IMO this would require a new matchFullRange or something like that? /cc @naderman

[Seldaek]

And I say a new method because matches can not be changed IMO, it serves a purpose too, to know whether ^1.0 is satisfied by 1.0.0 or 1.0.3 for example, and even with ranges on both sides if something replaces ^1.5 then a require on ^1.0 should match and be fulfilled.

[naderman]

@Seldaek well I'm rather confused: I'd tend to agree with you that matches should simply check if the two constraints intersect, and we'd need an additional more complex function to check whether one constraint is fully contained within another.

However, this does not match the current semver test suite. If matches() was supposed to check intersction only, then $a->matches($b) would be equivalent to $b->matches($a), which the test suite says is not true. So I think even the current implementation is inconsistent and probably only mostly works by chance cause Composer hardly ever compares two multiconstraints with each other so far.

So I think the first step would be to come up with a consistent explanation for what matches currently even does, and then to see what other functions we may need? Additionally we should review where and how we use matches() in Composer and whether the current implementation or any future changes could lead to bugs.

[naderman]

In particular what I said here: https://github.com/composer/semver/pull/86#discussion_r418072560 Doesn't actually make sense:

If we require 1. and replace 1.2.3, then it should match. If we require 1.2.3 and replace 1. it should match too. So this behavior makes sense, but it also means $a->matches($b) should be the same as $b->matches($a).

However if you flip matches() in the semver test suite, this breaks it (see errors below). It appears it only breaks for constraints which are not multi constraints. So the multiconstraint seems to work correctly, but the individual constraints do not?

I think we need to fix the test suite and those constraint, to properly codify $a->matches($b) means $a and $b intersect. I believe this is currently broken, and simply not noticed much because in real life hardly anyone uses unbound constraints.

We then need a new function for @toflar to check if a constraint is a real subset or not.

There were 28 failures:

1) Composer\Semver\SemverTest::testSatisfies with data set #4 (true, '1.3.0-beta', '>1.2')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

2) Composer\Semver\SemverTest::testSatisfies with data set #5 (true, '1.2.3-beta', '<=1.2.3')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

3) Composer\Semver\SemverTest::testSatisfies with data set #11 (true, '1.0.0', '>=1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

4) Composer\Semver\SemverTest::testSatisfies with data set #12 (true, '1.0.1', '>=1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

5) Composer\Semver\SemverTest::testSatisfies with data set #13 (true, '1.1.0', '>=1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

6) Composer\Semver\SemverTest::testSatisfies with data set #14 (true, '1.0.1', '>1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

7) Composer\Semver\SemverTest::testSatisfies with data set #15 (true, '1.1.0', '>1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

8) Composer\Semver\SemverTest::testSatisfies with data set #17 (true, '1.9999.9999', '<=2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

9) Composer\Semver\SemverTest::testSatisfies with data set #18 (true, '0.2.9', '<=2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

10) Composer\Semver\SemverTest::testSatisfies with data set #19 (true, '1.9999.9999', '<2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

11) Composer\Semver\SemverTest::testSatisfies with data set #20 (true, '0.2.9', '<2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

12) Composer\Semver\SemverTest::testSatisfies with data set #21 (true, '1.0.0', '>= 1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

13) Composer\Semver\SemverTest::testSatisfies with data set #22 (true, '1.0.1', '>=  1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

14) Composer\Semver\SemverTest::testSatisfies with data set #23 (true, '1.1.0', '>=   1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

15) Composer\Semver\SemverTest::testSatisfies with data set #24 (true, '1.0.1', '> 1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

16) Composer\Semver\SemverTest::testSatisfies with data set #25 (true, '1.1.0', '>  1.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

17) Composer\Semver\SemverTest::testSatisfies with data set #27 (true, '1.9999.9999', '<= 2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

18) Composer\Semver\SemverTest::testSatisfies with data set #28 (true, '0.2.9', '<=  2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

19) Composer\Semver\SemverTest::testSatisfies with data set #29 (true, '1.9999.9999', '<    2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

20) Composer\Semver\SemverTest::testSatisfies with data set #30 (true, '0.2.9', '<      2.0.0')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

21) Composer\Semver\SemverTest::testSatisfies with data set #31 (true, 'v0.1.97', '>=0.1.97')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

22) Composer\Semver\SemverTest::testSatisfies with data set #32 (true, '0.1.97', '>=0.1.97')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

23) Composer\Semver\SemverTest::testSatisfies with data set #51 (true, '1.0.0', '>=1')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

24) Composer\Semver\SemverTest::testSatisfies with data set #52 (true, '1.0.0', '>= 1')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

25) Composer\Semver\SemverTest::testSatisfies with data set #53 (true, '1.2.8', '>1.2')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

26) Composer\Semver\SemverTest::testSatisfies with data set #54 (true, '1.1.1', '<1.2')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

27) Composer\Semver\SemverTest::testSatisfies with data set #55 (true, '1.1.1', '< 1.2')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

28) Composer\Semver\SemverTest::testSatisfies with data set #66 (true, '1.2.8', '>=1.2')
Failed asserting that false matches expected true.

/home/naderman/projects/composer/semver/tests/SemverTest.php:31

[naderman]

Added a failing test case for the reproduce case of this bug in Composer here: https://github.com/composer/composer/pull/8869

So we'll need to fix the semver test suite and this behavior and then @jderusse can even make the original optimization and flip the operands of matches() if needed.

[Seldaek]

Closing this as matches is now fixed by #95 - and https://github.com/composer/semver/pull/97 will take care of isSubsetOf