fix: Do not optimise away packages due to a requirement by a locked package that will be uninstalled

[driskell]

Fixes #10394, introduced by #9620

As explained in that issue when I did the optimisation to filter out packages from the pool that would ultimately never be installed due to a locked packages requirements, I hadn't considered that when packages are removed... they may still be in the set of locked packages.

This PR updates the optimisation to ignore any locked package which isn't required by any package in the pool - as that will, as I understand, mean the package is going to be removed and it's requirements will be moot. I have considered the case where a package IS required somewhere in the pool, or even an older version for example, and in those cases the package would have been unlocked when using -W anyway. So I believe this is the correct fix.

I tested it against the scenario in #10394 and when running the second command in the following set:

composer require --dev phpunit/phpunit:"^7.5" --no-update --ignore-platform-req=php
composer update yoast/wp-test-utils --with-dependencies --ignore-platform-req=php

You then get an error about PHPUnit is locked.

When then running the second command with -W then before this fix (2.2.1), you get the following:

Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpunit/phpunit[7.5.0, ..., 7.5.20] require sebastian/exporter ^3.1 -> found sebastian/exporter[3.1.0, ..., 3.1.4] but these were not loaded, likely because it conflicts with another require.
    - Root composer.json requires phpunit/phpunit ^7.5 -> satisfiable by phpunit/phpunit[7.5.0, ..., 7.5.20].

With this PR merged it is now successful.

I will run the same timing/memory tests I did with the original PR to make sure the update is not making the optimisation useless...

@Seldaek thanks for bringing it to my attention - this will definitely need scrutiny to make sure the approach is right!

[driskell]

Benchmarks.

Using same as #9620, starting with https://github.com/drupal/recommended-project/blob/8.9.x/composer.json

$ composer install
...
$ composer require drupal/block_permissions:1.0.0
...
$ nano composer.json # Change block_permissions requirement to ^1.0
...
$ composer -vvv update -W drupal/block_permissions --profile

I did above to warm cache and then repeated the require:1.0.0, nano and update to get results below. I am on a laptop, MacBook Pro though, so there's variances...

[57.5MiB/10.24s] Dependency resolution completed in 0.004 seconds
[56.2MiB/10.24s] Analyzed 847 packages to resolve dependencies
[56.2MiB/10.24s] Analyzed 1693 rules to resolve dependencies
...
[47.8MiB/9.92s] Memory usage: 47.76MiB (peak: 113.61MiB), time: 9.92s
[47.5MiB/9.84s] Memory usage: 47.46MiB (peak: 113.36MiB), time: 9.84s
[47.2MiB/10.35s] Memory usage: 47.22MiB (peak: 113.21MiB), time: 10.35s
[47.6MiB/9.94s] Memory usage: 47.58MiB (peak: 113.46MiB), time: 9.94s

Below is 2.2.1 stable without the fix, it looks slower but it's probably pretty much the same nearly and only minuscule change and the variance I put down to me testing it with a laptop.

[52.2MiB/9.48s] Dependency resolution completed in 0.003 seconds
[50.8MiB/9.49s] Analyzed 847 packages to resolve dependencies
[50.8MiB/9.49s] Analyzed 1693 rules to resolve dependencies
...
[47.2MiB/9.91s] Memory usage: 47.2MiB (peak: 113.18MiB), time: 9.91s
[47.6MiB/10.81s] Memory usage: 47.58MiB (peak: 113.46MiB), time: 10.81s
[47.2MiB/10.04s] Memory usage: 47.2MiB (peak: 113.18MiB), time: 10.04s
[47.2MiB/11.57s] Memory usage: 47.2MiB (peak: 113.19MiB), time: 11.57s

Finally, when running latest main and disabling the optimisation, to ensure it is still beneficial, results show hopeful still as the rule count is massively increased without this. Granted the time saving does seem to be small still but this is a test with a single drupal module only.

[73.3MiB/9.75s] Dependency resolution completed in 0.030 seconds
[55.6MiB/9.76s] Analyzed 1781 packages to resolve dependencies
[55.6MiB/9.76s] Analyzed 23527 rules to resolve dependencies
...
[47.5MiB/10.17s] Memory usage: 47.49MiB (peak: 113.36MiB), time: 10.17s
[47.6MiB/11.52s] Memory usage: 47.64MiB (peak: 113.45MiB), time: 11.52s
[47.3MiB/12.05s] Memory usage: 47.29MiB (peak: 113.21MiB), time: 12.05s
[47.3MiB/11.47s] Memory usage: 47.29MiB (peak: 113.21MiB), time: 11.47s

[driskell]

@Seldaek The current set of comments I implemented, and the fixed test is properly reproducing again - no idea how I missed that test issue 😞 I will just double confirm that the scenario in the original issue is resolved still.

[driskell]

Yes, this resolved the repository example in the original issue.

[Seldaek]

Great, thanks, will review again when I get a chance.

[naderman]

Think this may also need tests for similar situations with replace/provide to make sure it works correctly in that case?

[driskell]

Think this may also need tests for similar situations with replace/provide to make sure it works correctly in that case?

I added some tests for both those scenarios. Currently the scenarios for replace/provide are not optimised for so I tried to note that in the test. It at least means we can check that it is behaving as expected currently and if optimisation is added down the line for provide/replace the test can be updated accordingly.

It's worth noting... whilst doing these tests I noticed that "allowTransitiveDeps" doesn't work for providers. If the provider is a root requirement and is not in the "allowList", and so attempts to update a package that requires the provided package will not unlock the provider. Not sure if this is intentional...?

[Seldaek]

It's worth noting... whilst doing these tests I noticed that "allowTransitiveDeps" doesn't work for providers. If the provider is a root requirement and is not in the "allowList", and so attempts to update a package that requires the provided package will not unlock the provider. Not sure if this is intentional...?

This is intentional yes, as many things can provide a package, unlocking these was getting messy and we weren't sure if it would be expected anyway, so we decided to leave it as is.

Thanks for the updates, I'll try and review this before 2.2.3 goes out.

[Seldaek]

OK turns out the name checking logic was inverted, so it would have broken if something had a replace which was not required by anything, as it would have found name not set in $this->requireConstraintsPerPackage, and it should only ignore if none of the names are referenced, not if any of them is.

Fixed that and added an additional test which was breaking before (although it covers the opposite case, a replacer required by the name it replaces but not by its own name, but it's the same problem really).

Otherwise looks good to me, thanks!

[jrfnl]

Thank you all for all your work on this. I have tested the situations in which I ran across the issue and all look to be fixed, so 🎉