Repository: python-poetry/poetry
Pull request: Fix the solver adding a package as a dependency of itself when extras are involved
# Pull Request Check List

Resolves: #5727 

- [x] Added **tests** for changed code.
- [ ] Updated **documentation** for changed code.


Problem statement:
poetry add "requests[security]" fails on Poetry 1.2.0b1
If I run `poetry add "requests[security]"` on Poetry 1.2.0b1, it fails with this message:

 Package 'requests' is listed as a dependency of itself.

I downgraded to Poetry 1.1.13 where it works. Thanks for all your work!

Hints from issue discussion:
Hello @thailow,

I guess your project itself is named `requests`. Have a look at your `pyproject.toml` at the keyword `name` in the `[tool.poetry]` section.

Naming the project the same as a dependency is not allowed.

fin swimmer
Hi @finswimmer, the project has a different name. I am pretty sure this is a bug as it's working fine in 1.1.13.
Hey, 

then please show your `pyproject.toml` so that one can reproduce your issue.

fin swimmer 
Sure, here it is:

```toml
[tool.poetry]
name = "cross_piano_client"
version = "0.1.27"
description = ""
authors = ["Full Name <full.name@email.com>"]

[tool.poetry.dependencies]
python = "3.9.13"
dependency-injector = "^4.38.0"
deprecated = "^1.2.13"
requests = {extras = ["security"], version = "^2.27.1"}

[tool.poetry.dev-dependencies]
pytest = "^7.1.2"

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

[[tool.poetry.source]]
name = "gitlab_shared"
url = "https://gitlab.com/api/v4/groups/1241251/-/packages/pypi/simple"
```
`poetry lock` with the above works just fine for me at all of 1.1.13, 1.2.0b1, and master
I just retested as well - you should be able to reproduce with these steps:
* `poetry install` with above `pyproject.toml`
* Remove requests from the `pyproject.toml`
* Run a `poetry update`
* Run `poetry add "requests[security]"`
* Get this error: `Package 'requests' is listed as a dependency of itself.`
The error is not thrown with 1.1.13.
Still all works fine for me...

Hard to know what to suggest in that case, maybe clear out your cache and see if that helps?  `rm -fr ~/.cache/pypoetry` or local equivalent
Strange.. I am managing the .venv in the projects (virtualenvs.in-project = true), hence, should not rely too much on any cache. Is it possible that there is some interference with pyenv? 

There is some other issue which I always face (and I do not know it this may be related as well). When I do a `poetry install`, it does not pick up the exact Python version specified in the `pyproject.toml` (and also not pyenv's version in .python-version). Rather, I always need to do a `poetry env use 3.9.13` or similar to enforce this.

By the way, I am on a Mac with M1 Max processor.
afaik the way that you create your virtual environment has nothing to do with poetry's cache.
I have the same problem, just with `black[jupyter]` (and also `coverage` for some reason when i change to just `black` without the extra).  Even full-clearing the cache did not fix the issue for me.

<details>
  <summary>stack trace</summary>

```bash
  Stack trace:

  17  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/application.py:330 in run
       328│
       329│             try:
     → 330│                 exit_code = self._run(io)
       331│             except Exception as e:
       332│                 if not self._catch_exceptions:

  16  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/console/application.py:174 in _run
       172│         self._load_plugins(io)
       173│
     → 174│         return super()._run(io)
       175│
       176│     def _configure_io(self, io: IO) -> None:

  15  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/application.py:425 in _run
       423│                 io.set_input(ArgvInput(argv))
       424│
     → 425│         exit_code = self._run_command(command, io)
       426│         self._running_command = None
       427│

  14  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/application.py:467 in _run_command
       465│
       466│         if error is not None:
     → 467│             raise error
       468│
       469│         return event.exit_code

  13  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/application.py:451 in _run_command
       449│
       450│             if event.command_should_run():
     → 451│                 exit_code = command.run(io)
       452│             else:
       453│                 exit_code = ConsoleCommandEvent.RETURN_CODE_DISABLED

  12  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/commands/base_command.py:118 in run
       116│         io.input.validate()
       117│
     → 118│         status_code = self.execute(io)
       119│
       120│         if status_code is None:

  11  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/cleo/commands/command.py:85 in execute
        83│
        84│         try:
     →  85│             return self.handle()
        86│         except KeyboardInterrupt:
        87│             return 1

  10  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/console/commands/update.py:51 in handle
        49│         self._installer.update(True)
        50│
     →  51│         return self._installer.run()
        52│

   9  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/installation/installer.py:114 in run
       112│         local_repo = Repository()
       113│
     → 114│         return self._do_install(local_repo)
       115│
       116│     def dry_run(self, dry_run: bool = True) -> Installer:

   8  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/installation/installer.py:325 in _do_install
       323│
       324│         with solver.use_environment(self._env):
     → 325│             ops = solver.solve(use_latest=self._whitelist).calculate_operations(
       326│                 with_uninstalls=self._requires_synchronization,
       327│                 synchronize=self._requires_synchronization,

   7  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/puzzle/solver.py:74 in solve
        72│         with self._provider.progress():
        73│             start = time.time()
     →  74│             packages, depths = self._solve(use_latest=use_latest)
        75│             end = time.time()
        76│

   6  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/puzzle/solver.py:133 in _solve
       131│
       132│         try:
     → 133│             result = resolve_version(
       134│                 self._package, self._provider, locked=locked, use_latest=use_latest
       135│             )

   5  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/mixology/__init__.py:24 in resolve_version
        22│     solver = VersionSolver(root, provider, locked=locked, use_latest=use_latest)
        23│
     →  24│     return solver.solve()
        25│

   4  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/mixology/version_solver.py:81 in solve
        79│             while next is not None:
        80│                 self._propagate(next)
     →  81│                 next = self._choose_package_version()
        82│
        83│             return self._result()

   3  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/mixology/version_solver.py:397 in _choose_package_version
       395│
       396│         conflict = False
     → 397│         for incompatibility in self._provider.incompatibilities_for(package):
       398│             self._add_incompatibility(incompatibility)
       399│

   2  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/puzzle/provider.py:433 in incompatibilities_for
       431│             dependencies.append(dep)
       432│
     → 433│         return [
       434│             Incompatibility(
       435│                 [Term(package.to_dependency(), True), Term(dep, False)],

   1  ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/puzzle/provider.py:434 in <listcomp>
       432│
       433│         return [
     → 434│             Incompatibility(
       435│                 [Term(package.to_dependency(), True), Term(dep, False)],
       436│                 DependencyCause(),

  AssertionError

  Package 'coverage' is listed as a dependency of itself.

  at ~/.local/share/pypoetry/venv/lib/python3.9/site-packages/poetry/mixology/incompatibility.py:61 in __init__
       57│                     # incompatibility irrelevant, since we already know that mutually
       58│                     # exclusive version ranges are incompatible. We should never derive
       59│                     # an irrelevant incompatibility.
       60│                     err_msg = f"Package '{ref}' is listed as a dependency of itself."
    →  61│                     assert by_ref[ref] is not None, err_msg
       62│                 else:
       63│                     by_ref[ref] = term
       64│
       65│             new_terms = []
```
</details>

Commit message: Add Failing test for #5727

Patch to review:
diff --git a/tests/puzzle/test_solver.py b/tests/puzzle/test_solver.py
index 931d56f641e..1efd5304564 100644
--- a/tests/puzzle/test_solver.py
+++ b/tests/puzzle/test_solver.py
@@ -793,6 +793,38 @@ def test_solver_finds_extras_next_to_non_extras(
     )
 
 
+def test_solver_merge_extras_into_base_package_multiple_repos_fixes_5727(
+    solver: Solver, repo: Repository, pool: Pool, package: ProjectPackage
+):
+    package.add_dependency(
+        Factory.create_dependency("A", {"version": "*", "source": "legacy"})
+    )
+    package.add_dependency(Factory.create_dependency("B", {"version": "*"}))
+
+    package_a = get_package("A", "1.0")
+    package_a.extras = {"foo": []}
+
+    repo.add_package(package_a)
+
+    package_b = Package("B", "1.0", source_type="legacy")
+    package_b.add_dependency(package_a.with_features(["foo"]).to_dependency())
+
+    package_a = Package("A", "1.0", source_type="legacy")
+    package_a.extras = {"foo": []}
+
+    repo = Repository("legacy")
+    repo.add_package(package_a)
+    repo.add_package(package_b)
+
+    pool.add_repository(repo)
+
+    transaction = solver.solve()
+
+    ops = transaction.calculate_operations(synchronize=True)
+
+    assert len(ops[0].package.requires) == 0, "a should not require itself"
+
+
 def test_solver_returns_prereleases_if_requested(
     solver: Solver, repo: Repository, package: ProjectPackage
 ):


Code review comments:
diff_hunk: @@ -164,21 +164,20 @@ def _solve(
             if package.features:
                 for _package in packages:
                     if (
-                        _package.name == package.name
-                        and not _package.is_same_package_as(package)
+                        not _package.features
+                        and package.provides(_package)
path: src/poetry/puzzle/solver.py
text: @user1:
```suggestion
                        and _package.name == package.name
```

diff_hunk: @@ -164,21 +164,20 @@ def _solve(
             if package.features:
                 for _package in packages:
                     if (
-                        _package.name == package.name
-                        and not _package.is_same_package_as(package)
+                        not _package.features
+                        and package.provides(_package)
                         and _package.version == package.version
                     ):
                         for dep in package.requires:
-                            if dep.is_same_package_as(_package):
+                            # Prevent adding base package as a dependency to itself
+                            if _package.provides(dep):
path: src/poetry/puzzle/solver.py
text: @user1:
```suggestion
                            if _package.name == dep.name:
```

diff_hunk: @@ -164,21 +164,20 @@ def _solve(
             if package.features:
                 for _package in packages:
                     if (
-                        _package.name == package.name
-                        and not _package.is_same_package_as(package)
+                        not _package.features
path: src/poetry/puzzle/solver.py
text: @user1:
Seems like something is redundant here: `package.provides()` checks the compatibility of features so either we didn't need to check `not _package.features` or we only needed to check that the names (and versions) match, right?

Similarly, since we know that `_package` has no features the later check that uses `_package.provides()` is also just a check on the names, I think?



@user2:
I interpreted the line as being an early return -- yes there's a redundant check but that's because we're re-using `package.provides()` but we only need half the functionality (name + version). That being said, I'm not nearly as much of an expert on this code so I'm going to refer to your expertise.

Do you have any reservations about the design, or is it more about clarity of intent given the overlapping checks / a desire to make explicit what we're checking by not using the function?

@user1:
I'm mostly trying to check that I understand it myself!  

I think this code is searching for "base" packages: that is feature-less packages that are otherwise the same as `package`.  If that's right then I would find it slightly clearer to go
```patch
                 for _package in packages:
                     if (
                         not _package.features
-                        and package.provides(_package)
+                        and _package.name == package.name
                         and _package.version == package.version
                     ):
                         for dep in package.requires:
                             # Prevent adding base package as a dependency to itself
-                            if _package.provides(dep):
+                            if _package.name == dep.name:
                                 continue

                             if dep not in _package.requires:
```

so that I don't have to go and check what `package.provides()` means.

But this is picking nits: the change looks plausible, it has a test, it doesn't obviously break anything, it's good for me.

@user2:
Appreciate your insight as always. While I don't mind the use of provides here, I am going to agree that your version makes clear the meaning of this code at a glance. I think that given the issues we've had with confusion over `Package.__eq__`, `Package.__hash__`, `Package.provides` and `Package.is_same_package_as`, we're better off spelling things out explicitly here to prevent regressions (or more likely confusion, given that we do have some high quality tests with this change). Would you mind submitting your suggestions as a review so that @author can apply them easily?