Complex exclusion not parsed correctly

[bendavies]

ref: composer/composer#5617

Given

{
    "require": {
        "jms/serializer": "~0.13|~1.0 !=1.3.1"
    }
}

version 1.3.1 is not excluded.

[alcohol]

@Seldaek I am still uncertain about how this should be parsed (e.g. is it a matter of order or operator precedence?). It seems like a bug to me, but want to check with you first.

Outcome of the above constraint is:

class Composer\Semver\Constraint\MultiConstraint#10 (3) {
  protected $constraints =>
  array(2) {
    [0] =>
    class Composer\Semver\Constraint\Constraint#11 (3) {
      protected $operator =>
      int(4)
      protected $version =>
      string(12) "0.13.0.0-dev"
      protected $prettyString =>
      NULL
    }
    [1] =>
    class Composer\Semver\Constraint\Constraint#12 (3) {
      protected $operator =>
      int(1)
      protected $version =>
      string(22) "2.0.0.0-dev != 1.3.1.0"
      protected $prettyString =>
      NULL
    }
  }
  protected $prettyString =>
  string(18) "~0.13|~1.0 !=1.3.1"
  protected $conjunctive =>
  bool(true)
}

[bendavies]

it's parsed like that because it's collapsed
https://github.com/composer/semver/blob/master/src/VersionParser.php#L263-L277
That constraint still looks ok though? so maybe a solver bug?

[stof]

the issue here is that the upper bound is parsed as being 2.0.0.0-dev != 1.3.1.0, instead of using 2 different constraints here.

Can you try to add a comma between ~1.0 and != 1.3.1 ?

[bendavies]

doesn't help unfortunately stof.

[stof]

Looks like the regex splitting the AND constraints does not play well with the != operator

[bendavies]

looks ok to me...

Composer\Semver\Constraint\MultiConstraint::__set_state(array(
   'constraints' => 
  array (
    0 => 
    Composer\Semver\Constraint\Constraint::__set_state(array(
       'operator' => 4,
       'version' => '1.0.0.0-dev',
       'prettyString' => NULL,
    )),
    1 => 
    Composer\Semver\Constraint\Constraint::__set_state(array(
       'operator' => 1,
       'version' => '2.0.0.0-dev',
       'prettyString' => NULL,
    )),
    2 => 
    Composer\Semver\Constraint\Constraint::__set_state(array(
       'operator' => 5,
       'version' => '1.3.1.0',
       'prettyString' => NULL,
    )),
  ),
   'prettyString' => NULL,
   'conjunctive' => true,
))

[>= 1.0.0.0-dev < 2.0.0.0-dev != 1.3.1.0]

[stof]

well, in this case, yes. But try with ~1.0 !=1.3.1, which is what was reported here

[bendavies]

that is for ~1.0 !=1.3.1,

[alcohol]

I suspect it has to do with attempting to collapse the contiguous range (since ~0.x || ~1.x is a contiguous range) not properly taking into account the exclude.

[bendavies]

that's what i said 😉

[alcohol]

I am confused. I wrote some tests, but they all work as expected..

[alcohol]

Never mind. Added some more assertions. The string representation is identical. The actual count of constraints inside the multiconstraint is not.

[alcohol]

Here is a simple debugging session against the given constraint in the crucial part of the code.

[alcohol]

Due to the simple string comparison in there things do not end up as expected. Not sure what the most elegant solution would be here?

[Seldaek]

I don't think it's an issue here tbh, the parsing looks fine, and it should work.. collapsing that range isn't wrong AFAICT, as >=0.13 AND <2.0 AND not 1.3.1 should result in the correct stuff being picked. I am wondering if it's not instead something going of in the solver or the evaluation of the constraint.. But it's kinda late here so maybe I'm missing something :)

[alcohol]

The problem is that the upper bound + the exclude are merged into one constraint, while they should be a multiconstraint (or separate constraints in the multiconstraint being generated). You cannot have a single constraint that represents both the upper bound and the exclusion because a constraint only has one operator.

[alcohol]

Now it actually translates into (>="0.13") AND (<"2.0 AND NOT 1.3.1") (quotes and brackets added for clarity). So it simply acts as an upper bound only, the "NOT" gets lost in conversion through collapsing.

[Seldaek]

In #42 (comment) the operator => 5 contains the NOT, so I think that's fine.

[bendavies]

feel free to reopen the original composer/composer#5617 if you think it's a solver issue.

[alcohol]

@Seldaek that is proper parsing yes. But that only happens when there is not a contiguous range (the constraint used there was ~1.0 !=1.3.1, not ~0.13 || ~1.0 !=1.3.1).

[alcohol]

As you can see in my debug session, it initially creates two multi constraints, the first containing lower and upper bound, and the second containing lower and upper bound and the not constraint. When it merges them it ends up with only a lower and upper bound. See #42 (comment) for details.