Recognise empty marker in a complex intersection

[dimbleby]

Fixes python-poetry/poetry-plugin-export#163, in which the problem turns out to be that the exporter thinks that it has found overlapping markers when it really hasn't.

I'm a bit surprised that this didn't have any effect on existing unit test, but I guess that's good...

Fix feels a bit ad-hoc, better ways welcomed!

[radoering]

Fix feels a bit ad-hoc, better ways welcomed!

PTAL #529. Although it feels more like extending the existing approach than just adding something in the end, I'm not completely sure if that's better.

[dimbleby]

#529 fails in the more complicated case that is actually python-poetry/poetry-plugin-export#163 - there the markers that should collapse on intersection are:

  m1 = parse_marker(
      'python_version >= "3.8" and python_version < "3.11" and (python_version > "3.9" or'
      ' platform_system != "Windows" or platform_machine != "x86") or python_version >='
      ' "3.11" and python_version < "3.12"'
  )
  m2 = parse_marker(
      'python_version == "3.8" and platform_system == "Windows" and platform_machine =='
      ' "x86" or python_version == "3.9" and platform_system == "Windows" and'
      ' platform_machine == "x86"'
  )

I guess I should have added another testcase!

Inasmuch as I've been thinking about this at all, I'm coming round to believing in the dnf() solution. It makes intuitive sense to me that DNF should be effective at finding - and eliminating - empty parts of an intersection. (In much the same way that CNF is a good try at union - cf #390)

I'm still more than happy to see a different approach though eg if #529 can be further improved to handle the even-more-complicated case that's good by me

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[radoering]

Actually, I think DNF is the only (or at least by far the most simple) way to detect empty markers in abitrary complex markers. I almost believe that every time we improve the alternative solution there will be a more complex marker we don't cover yet while the code becomes more and more complex. Maybe, the code becomes even simpler while recognizing arbitrary complex empty and any markers if we rely more on DNF (and CNF).

What about being a bit more radical and replacing the complete body of MultiMarker.intersect() with return dnf(MultiMarker(self, other))?

[dimbleby]

What about being a bit more radical and replacing the complete body of MultiMarker.intersect() with return dnf(MultiMarker(self, other))?

I was kinda surprised that the fix as it stands doesn't break any tests, I guess I'm more surprised that this more extreme version doesn't!

I agree that the insight about dnf being good for intersection and cnf for union suggests that there's probably opportunity for more code simplification out there.

Probably I have a mild preference for merging this as is and leaving further work for another day, but I think you have the power to make change to this MR: if you want to do that then I'm ok with it

[radoering]

OK, then let's keep the fix here as simple and risk-free as possible and do the refactoring in your follow-up PR.