gh-141647: fix IP address/network comparison methods by picnixz · Pull Request #141842 · python/cpython

picnixz · 2025-11-22T15:24:09Z

I think this patch should cover the cases. I'm unsure whether this will break more stuff or not but I do know that using < and __lt__ return different boolean results which can break many things.

@serhiy-storchaka I really want a second eye here so PTAL whenever you've got time. TiA!

Issue: Use of .version attribute in ipaddress classes break many methods #141647

serhiy-storchaka

Please make benchmarks for equality and order comparison.

picnixz · 2025-11-29T10:27:14Z

So I'm a bit surprised but here are the numbers:

+---------------------------------------------+---------------+-----------------------+
| Benchmark                                   | ipaddress-ref | ipaddress-new         |
+=============================================+===============+=======================+
| address.__eq__[1.2.3.4,5.6.7.8]             | 39.7 ns       | 41.0 ns: 1.03x slower |
+---------------------------------------------+---------------+-----------------------+
| interface.__eq__[192.0.2.0/32,192.0.2.0/32] | 242 ns        | 264 ns: 1.09x slower  |
+---------------------------------------------+---------------+-----------------------+
| interface.__lt__[192.0.2.0/32,192.0.8.0/32] | 344 ns        | 317 ns: 1.09x faster  |
+---------------------------------------------+---------------+-----------------------+
| interface.__eq__[192.0.2.0/32,192.0.2.0/24] | 253 ns        | 245 ns: 1.03x faster  |
+---------------------------------------------+---------------+-----------------------+
| interface.__lt__[192.0.8.0/32,192.0.8.0/32] | 389 ns        | 362 ns: 1.07x faster  |
+---------------------------------------------+---------------+-----------------------+
| network.__lt__[192.0.2.0/32,192.0.8.0/32]   | 160 ns        | 167 ns: 1.04x slower  |
+---------------------------------------------+---------------+-----------------------+
| network.__eq__[192.0.8.0/32,192.0.2.0/24]   | 80.4 ns       | 76.4 ns: 1.05x faster |
+---------------------------------------------+---------------+-----------------------+
| network.__eq__[192.0.2.0/24,192.0.2.0/24]   | 140 ns        | 134 ns: 1.05x faster  |
+---------------------------------------------+---------------+-----------------------+
| network.__lt__[192.0.2.0/24,192.0.2.0/24]   | 173 ns        | 166 ns: 1.04x faster  |
+---------------------------------------------+---------------+-----------------------+
| mixed.__lt__[5.6.7.8,1.2.3.4/32]            | 416 ns        | 387 ns: 1.07x faster  |
+---------------------------------------------+---------------+-----------------------+
| mixed.__lt__[5.6.7.8/32,5.6.7.8/32]         | 372 ns        | 398 ns: 1.07x slower  |
+---------------------------------------------+---------------+-----------------------+
| Geometric mean                              | (ref)         | 1.00x faster          |
+---------------------------------------------+---------------+-----------------------+

This benchmark uses the _check_ip_version additional call and Python 3.14 from uv (since the changes are only in pure Python, it's fine to switch branches). I've also tried those "slower" cases but it's really within the real of noise IMO.

Benchmark script

import ipaddress
import itertools
import operator

import pyperf

seen = set()

def bench_eq(runner, label, x, y, seen):
    name = f"{label}.__eq__[{x},{y}]"
    if name not in seen:
        runner.bench_func(name, operator.__eq__, x, y)
        seen.add(name)

def bench_lt(runner, label, x, y, seen):
    name = f"{label}.__lt__[{x},{y}]"
    if name not in seen:
        runner.bench_func(name, operator.__lt__, x, y)
        seen.add(name)


if __name__ == "__main__":
    runner = pyperf.Runner()
    args = runner.parse_args()
    seen = set()

    for x, y in itertools.combinations_with_replacement(
        [
            ipaddress.IPv4Address("1.2.3.4"),
            ipaddress.IPv4Address("5.6.7.8"),
        ], 2
    ):
        bench_eq(runner, "address", x, y, seen)
        bench_lt(runner, "address", x, y, seen)

    for x, y in itertools.combinations_with_replacement(
        [
            ipaddress.IPv4Interface("192.0.2.0"),
            ipaddress.IPv4Interface("192.0.8.0"),
            ipaddress.IPv4Interface("192.0.2.0/24"),
            ipaddress.IPv4Interface("192.0.8.0/32"),
        ], 2
    ):
        bench_eq(runner, "interface", x, y, seen)
        bench_lt(runner, "interface", x, y, seen)

    for x, y in itertools.combinations_with_replacement(
        [
            ipaddress.IPv4Network("192.0.2.0"),
            ipaddress.IPv4Network("192.0.8.0"),
            ipaddress.IPv4Network("192.0.2.0/24"),
            ipaddress.IPv4Network("192.0.8.0/32"),
        ], 2
    ):
        bench_eq(runner, "network", x, y, seen)
        bench_lt(runner, "network", x, y, seen)

    for x, y in itertools.combinations_with_replacement(
        [
            ipaddress.IPv4Address("1.2.3.4"),
            ipaddress.IPv4Address("5.6.7.8"),
            ipaddress.IPv4Interface("1.2.3.4"),
            ipaddress.IPv4Interface("5.6.7.8"),
            ipaddress.IPv4Interface("1.2.3.4/24"),
            ipaddress.IPv4Interface("5.6.7.8/32"),
        ], 2
    ):
        bench_lt(runner, "mixed", x, y, seen)

serhiy-storchaka

I think there is a documentation and an implementation bugs in collapse_addresses(). They cab considered a separate issue(s).

serhiy-storchaka · 2025-12-22T18:01:57Z

    nets = []

-    # split IP addresses and networks
+    # split IP addresses/interfaces and networks


According to the documentation, all input objects should be networks. The code supports addresses (this is undocumented behavior). Support of interfaces is a new feature.

I think there is an online docs issue as well. We allow ordering with interfaces and adresses (or some weird mixed comparisons that I do not remember)

serhiy-storchaka · 2025-12-22T18:05:09Z

+                if nets:
+                    _check_ip_version(nets[-1], ip)


What will happen if we have separate networks and addresses of different versions? There is a bug here, this is a separate issue.

AFAIR different versions are not allowed with comparisons. So I guess Inshould consider the first item being compared to as the one who gives the version to use.

serhiy-storchaka · 2025-12-22T18:28:16Z

+            # compare interfaces by their network first
+            return (self.network < other.network
+                    or (self.network == other.network and address_less))
+        return address_less


Should be False if unassociated address is considered less than all interfaces.

serhiy-storchaka · 2025-12-22T18:31:17Z

+            # should __contains__ actually implement subnet_of()
+            # and supernet_of() instead?


A subset is usually not considered to be an element of the superset.

There is a weird behavior of strings, but it is an exception.

Yeah but since there was a branch with "dealing with other networks" I wondered why we even supported that check. Asking whether network A is contained in another network B rings like "A is a subnet of B" to me.

This branch is just to prevent interpretation of a network as an address, since they share common attributes and base classes. It is a guard against incorrect application of the Liskov substitution principle.

Oh I see! ok I will remove that comment then now that I know why we did this check.

If _BaseNetwork is not a subclass of _BaseAddress, we can remove this check.

_BaseNetwork is a subclass of _IPAddressBase. However, people doing net_a in net_b will now get a AttributeError (probably) instead of "False". So maybe it's better to keep the check.

serhiy-storchaka · 2025-12-24T12:08:43Z

+            # should __contains__ actually implement subnet_of()
+            # and supernet_of() instead?


If _BaseNetwork is not a subclass of _BaseAddress, we can remove this check.

serhiy-storchaka · 2025-12-24T12:10:07Z

+                self.version == other.version
+                and (other._ip & self.netmask._ip) == self.network_address._ip
+            )
+        return NotImplemented


Is NotImplemented supported for __contains__?

Mmh, I was under the impression that dunders needed to raise NotImplemented for the generic unsupported case but maybe a TypeError should be raised directly?

bedevere-app Bot added the awaiting core review label Nov 22, 2025

bedevere-app Bot mentioned this pull request Nov 22, 2025

Use of .version attribute in ipaddress classes break many methods #141647

Open

picnixz requested a review from serhiy-storchaka November 22, 2025 15:24

picnixz force-pushed the fix/ipaddress/instance-checks-141647 branch from e88a7c0 to bd5eeda Compare November 22, 2025 15:25

fix IP address/network comparison methods

ca36ea4

picnixz force-pushed the fix/ipaddress/instance-checks-141647 branch from bd5eeda to ca36ea4 Compare November 22, 2025 15:34

serhiy-storchaka reviewed Nov 22, 2025

View reviewed changes

Comment thread Lib/ipaddress.py

Comment thread Lib/ipaddress.py Outdated

Comment thread Lib/ipaddress.py Outdated

Comment thread Lib/ipaddress.py Outdated

picnixz added 3 commits November 29, 2025 10:50

revert PEP-8

b20b15a

remove redundant isinstance()

a0d7181

improve error messages for invalid objects

100dad9

picnixz requested a review from serhiy-storchaka November 29, 2025 10:30

serhiy-storchaka reviewed Dec 22, 2025

View reviewed changes

serhiy-storchaka reviewed Dec 24, 2025

View reviewed changes

		# should __contains__ actually implement subnet_of()
		# and supernet_of() instead?

Uh oh!

Conversation

picnixz commented Nov 22, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz commented Nov 29, 2025

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

picnixz commented Nov 22, 2025 •

edited by bedevere-app Bot

Loading

picnixz Dec 24, 2025 •

edited

Loading

picnixz Dec 27, 2025 •

edited

Loading