Skip to content

Commit

Permalink
Replace naive sorted() implementation with rich comparison methods.
Browse files Browse the repository at this point in the history
This commit removes the naive ("sort { $a cmp $b }") comparison used by
sorted() and replaces it with checks against the rich comparison methods
(__lt__, __eq__, etc.) which brings us much closer to the way Python
behaves.

This also fixes a number of comparison discrepancies between Python and our impelementation
and fails hard if no comparison for two data types is implemented.
  • Loading branch information
mreitinger committed Feb 22, 2024
1 parent fa307a1 commit 15a663b
Show file tree
Hide file tree
Showing 9 changed files with 248 additions and 15 deletions.
10 changes: 9 additions & 1 deletion p5lib/Python2/Builtin/Sorted.pm
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,15 @@ sub __call__ {
} $list->ELEMENTS

: sort {
$a->__tonative__ cmp $b->__tonative__
if(${ $a->__lt__($b) }->__tonative__) {
return -1;
}
elsif (${ $a->__eq__($b) }->__tonative__) {
return 0;
}
else {
return 1;
}
} $list->ELEMENTS
;

Expand Down
53 changes: 53 additions & 0 deletions p5lib/Python2/Type/Dict.pm
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use warnings;
use strict;

use Scalar::Util qw/ refaddr /;
use List::Util qw/ min /;
use Tie::PythonDict;

sub new {
Expand Down Expand Up @@ -223,6 +224,32 @@ sub __gt__ {
return \Python2::Type::Scalar::Bool->new(1)
if ($other->__type__ eq 'int');

return \Python2::Type::Scalar::Bool->new(1)
if ($other->__type__ eq 'bool');

return \Python2::Type::Scalar::Bool->new(0)
if ($other->__type__ eq 'list');

return \Python2::Type::Scalar::Bool->new(0)
if ($other->__type__ eq 'tuple');

return \Python2::Type::Scalar::Bool->new(0)
if ($other->__type__ eq 'str');

# ref https://hg.python.org/releasing/2.7.9/file/753a8f457ddc/Objects/dictobject.c#l1792
if ($other->__type__ eq 'dict') {
if (${ $self->__len__ }->__tonative__ > ${ $other->__len__ }->__tonative__) {
return \Python2::Type::Scalar::Bool->new(1);
}

if (${ $self->__len__ }->__tonative__ < ${ $other->__len__ }->__tonative__) {
return \Python2::Type::Scalar::Bool->new(0);
}

return \Python2::Type::Scalar::Bool->new(0)
if $self->__eq__($other);
}

die Python2::Type::Exception->new('NotImplementedError', '__gt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand All @@ -232,6 +259,32 @@ sub __lt__ {
return \Python2::Type::Scalar::Bool->new(0)
if ($other->__type__ eq 'int');

return \Python2::Type::Scalar::Bool->new(0)
if ($other->__type__ eq 'bool');

return \Python2::Type::Scalar::Bool->new(1)
if ($other->__type__ eq 'list');

return \Python2::Type::Scalar::Bool->new(1)
if ($other->__type__ eq 'tuple');

return \Python2::Type::Scalar::Bool->new(1)
if ($other->__type__ eq 'str');

# ref https://hg.python.org/releasing/2.7.9/file/753a8f457ddc/Objects/dictobject.c#l1792
if ($other->__type__ eq 'dict') {
if (${ $self->__len__ }->__tonative__ < ${ $other->__len__ }->__tonative__) {
return \Python2::Type::Scalar::Bool->new(1);
}

if (${ $self->__len__ }->__tonative__ > ${ $other->__len__ }->__tonative__) {
return \Python2::Type::Scalar::Bool->new(0);
}

return \Python2::Type::Scalar::Bool->new(0)
if $self->__eq__($other);
}

die Python2::Type::Exception->new('NotImplementedError', '__lt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand Down
42 changes: 40 additions & 2 deletions p5lib/Python2/Type/List.pm
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,27 @@ sub __lt__ {

return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::Num';
return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::String';
return \Python2::Type::Scalar::Bool->new(0) if $other->__type__ eq 'list';
return \Python2::Type::Scalar::Bool->new(0) if $other->__type__ eq 'dict';
return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::Bool';
return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Tuple';

if ($other->__type__ eq 'list') {
# compare all elements we can compare
foreach (0 .. min(${ $self->__len__ }->__tonative__, ${ $other->__len__ }->__tonative__) - 1) {
return \Python2::Type::Scalar::Bool->new(1)
if ${
${ $self->__getitem__(Python2::Type::Scalar::Num->new($_) ) }
->__lt__(${ $other->__getitem__(Python2::Type::Scalar::Num->new($_)) });
}->__tonative__;
}

return \Python2::Type::Scalar::Bool->new(
${ $self->__len__ }->__tonative__ == ${ $other->__len__ }->__tonative__
? 0
: ${ $self->__len__ }->__tonative__ < ${ $other->__len__ }->__tonative__
);
}

die Python2::Type::Exception->new('NotImplementedError', '__lt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand All @@ -164,8 +183,27 @@ sub __gt__ {

return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::Num';
return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::String';
return \Python2::Type::Scalar::Bool->new(0) if $other->__type__ eq 'list';
return \Python2::Type::Scalar::Bool->new(1) if $other->__type__ eq 'dict';
return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::Bool';
return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Tuple';

if ($other->__type__ eq 'list') {
foreach (0 .. min(${ $self->__len__ }->__tonative__, ${ $other->__len__ }->__tonative__) - 1) {
return \Python2::Type::Scalar::Bool->new(1)
if ${
${ $self->__getitem__(Python2::Type::Scalar::Num->new($_) ) }
->__gt__(${ $other->__getitem__(Python2::Type::Scalar::Num->new($_)) });
}->__tonative__;
}

return \Python2::Type::Scalar::Bool->new(
${ $self->__len__ }->__tonative__ == ${ $other->__len__ }->__tonative__
? 0
: ${ $self->__len__ }->__tonative__ > ${ $other->__len__ }->__tonative__
);
}


die Python2::Type::Exception->new('NotImplementedError', '__gt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand Down
33 changes: 33 additions & 0 deletions p5lib/Python2/Type/Scalar/Num.pm
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ sub __gt__ {
return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Scalar::String';

return \Python2::Type::Scalar::Bool->new($self->__tonative__ > $other->__tonative__ // 0)
if ref($other) eq 'Python2::Type::Scalar::Bool';

return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Scalar::String';

return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::List';

return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Tuple';

return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Dict';

die Python2::Type::Exception->new('NotImplementedError', '__gt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand All @@ -64,6 +79,21 @@ sub __lt__ {
return \Python2::Type::Scalar::Bool->new($self->__tonative__ < $other->__tonative__)
if ($other->__class__ eq 'Python2::Type::Scalar::Num');

return \Python2::Type::Scalar::Bool->new($self->__tonative__ < $other->__tonative__)
if ref($other) eq 'Python2::Type::Scalar::Bool';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::Scalar::String';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::List';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::Tuple';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::Dict';

die Python2::Type::Exception->new('NotImplementedError', '__lt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand All @@ -79,6 +109,9 @@ sub __ge__ {
return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Scalar::String';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::List';

die Python2::Type::Exception->new('NotImplementedError', '__ge__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand Down
24 changes: 24 additions & 0 deletions p5lib/Python2/Type/Scalar/String.pm
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,18 @@ sub __gt__ {
return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::Scalar::Num';

return \Python2::Type::Scalar::Bool->new(1)
if $other->__type__ eq 'list';

return \Python2::Type::Scalar::Bool->new(0)
if $other->__type__ eq 'tuple';

return \Python2::Type::Scalar::Bool->new(1)
if $other->__type__ eq 'dict';

return \Python2::Type::Scalar::Bool->new(1)
if ref($other) eq 'Python2::Type::Scalar::Bool';

die Python2::Type::Exception->new('NotImplementedError', '__gt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand All @@ -486,6 +498,18 @@ sub __lt__ {
return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Scalar::Num';

return \Python2::Type::Scalar::Bool->new(0)
if $other->__type__ eq 'list';

return \Python2::Type::Scalar::Bool->new(1)
if $other->__type__ eq 'tuple';

return \Python2::Type::Scalar::Bool->new(0)
if $other->__type__ eq 'dict';

return \Python2::Type::Scalar::Bool->new(0)
if ref($other) eq 'Python2::Type::Scalar::Bool';

die Python2::Type::Exception->new('NotImplementedError', '__lt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

Expand Down
67 changes: 67 additions & 0 deletions p5lib/Python2/Type/Tuple.pm
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,73 @@ sub __tonative__ {

sub __type__ { return 'tuple'; }

sub __lt__ {
my ($self, $other) = @_;

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0] and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0]);

return \Python2::Type::Scalar::Bool->new(1)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(
${$self->[0]->__eq__($other->[0])}->__tonative__
? ${ $self->__len__ }->__tonative__ < ${ $other->__len__ }->__tonative__
: ${ $self->[0]->__lt__($other->[0]) }->__tonative__
) if ref($other) eq 'Python2::Type::Tuple';

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0] and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(1)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0]);

return \Python2::Type::Scalar::Bool->new(
${ $self->[0]->__lt__($other->[0]) }->__tonative__
) if ref($other) eq 'Python2::Type::Tuple';

return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::Num';
return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::String';
return \Python2::Type::Scalar::Bool->new(0) if $other->__type__ eq 'dict';
return \Python2::Type::Scalar::Bool->new(0) if $other->__type__ eq 'list';
return \Python2::Type::Scalar::Bool->new(0) if ref($other) eq 'Python2::Type::Scalar::Bool';

die Python2::Type::Exception->new('NotImplementedError', '__lt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

sub __gt__ {
my ($self, $other) = @_;

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0] and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(1)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $other->[0]);

return \Python2::Type::Scalar::Bool->new(0)
if (ref($other) eq 'Python2::Type::Tuple' and not exists $self->[0]);

return \Python2::Type::Scalar::Bool->new(
${$self->[0]->__eq__($other->[0])}->__tonative__
? ${ $self->__len__ }->__tonative__ > ${ $other->__len__ }->__tonative__
: ${ $self->[0]->__gt__($other->[0]) }->__tonative__
) if ref($other) eq 'Python2::Type::Tuple';

return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::Num';
return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::String';
return \Python2::Type::Scalar::Bool->new(1) if $other->__type__ eq 'dict';
return \Python2::Type::Scalar::Bool->new(1) if $other->__type__ eq 'list';
return \Python2::Type::Scalar::Bool->new(1) if ref($other) eq 'Python2::Type::Scalar::Bool';

die Python2::Type::Exception->new('NotImplementedError', '__gt__ between ' . $self->__type__ . ' and ' . $other->__type__);
}

sub __eq__ {
my ($self, $other) = @_;

Expand Down
15 changes: 9 additions & 6 deletions t/output-comparison-python-interpreter/comparisons-__gt__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
print 1 > 1
print 1 > 2
print 2 > 1
print 'a' > 'a'
print 'a' > 'b'
print 'b' > 'a'
types = [
1, 2, True, False, [], (), (1, ), {}, [1, 2], [1, 2, 3],
(1, 2), (1, 2, 3), "", "test", {1: 2}, {1: 2, 3: 4},
"a", "b"
]

for left in types:
for right in types:
print "%s > %s with '%s' > '%s': %s" % (type(left), type(right), left, right, left > right)
15 changes: 9 additions & 6 deletions t/output-comparison-python-interpreter/comparisons-__lt__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
print 1 < 1
print 1 < 2
print 2 < 1
print 'a' < 'a'
print 'a' < 'b'
print 'b' < 'a'
types = [
1, 2, True, False, [], (), (1, ), {}, [1, 2], [1, 2, 3],
(1, 2), (1, 2, 3), "", "test", {1: 2}, {1: 2, 3: 4},
"a", "b"
]

for left in types:
for right in types:
print "%s < %s with '%s' < '%s': %s" % (type(left), type(right), left, right, left < right)
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
x = [(3, 2, 6), (2, 2, 6), (4, 2, 5), (1, 2, 4), ('b', 'c', 'd'), ('a', 'b', 'c'), ('A', 'B', 'C')]

print sorted(x)
print sorted(x, key=lambda s: s[2])

0 comments on commit 15a663b

Please sign in to comment.