Skip to content

Fix min/max skipping first iterator item with default and no key #2137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
slozier merged 1 commit into from
Nov 3, 2026
Merged

Fix min/max skipping first iterator item with default and no key #2137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 77 additions & 115 deletions src/core/IronPython/Modules/Builtin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,10 +792,12 @@ public static object locals(CodeContext/*!*/ context) {

private static readonly object UndefinedKeywordArgument = new object();

public static object? max(CodeContext/*!*/ context, object? x) {
private static object? MaxWithDefault(CodeContext/*!*/ context, object? x, object? def) {
IEnumerator i = PythonOps.GetEnumerator(context, x);
if (!i.MoveNext())
if (!i.MoveNext()) {
if (def != UndefinedKeywordArgument) return def;
throw PythonOps.ValueError("max() arg is an empty sequence");
}
object? ret = i.Current;
PythonContext pc = context.LanguageContext;
while (i.MoveNext()) {
Expand All @@ -804,9 +806,11 @@ public static object locals(CodeContext/*!*/ context) {
return ret;
}

public static object? max(CodeContext/*!*/ context, object? x, object? y) {
return context.LanguageContext.GreaterThan(x, y) ? x : y;
}
public static object? max(CodeContext/*!*/ context, object? x)
=> MaxWithDefault(context, x, UndefinedKeywordArgument);

public static object? max(CodeContext/*!*/ context, object? x, object? y)
=> context.LanguageContext.GreaterThan(y, x) ? y : x;

public static object? max(CodeContext/*!*/ context, [NotNone] params object?[] args) {
if (args.Length > 0) {
Expand All @@ -829,21 +833,18 @@ public static object locals(CodeContext/*!*/ context) {
}

public static object? max(CodeContext/*!*/ context, object? x, [ParamDictionary] IDictionary<string, object?> dict) {
IEnumerator i = PythonOps.GetEnumerator(context, x);
VerifyKeys(nameof(max), dict, out object? method, out object? def);

var kwargTuple = GetMaxKwArg(dict, isDefaultAllowed: true);
object? method = kwargTuple.Item1;
object? def = kwargTuple.Item2;
if (method == UndefinedKeywordArgument || method == null) { // null key is allowed since CPython 3.8
return MaxWithDefault(context, x, def);
}

IEnumerator i = PythonOps.GetEnumerator(context, x);
if (!i.MoveNext()) {
if (def != UndefinedKeywordArgument) return def;
throw PythonOps.ValueError("max() arg is an empty sequence");
}

if (method == UndefinedKeywordArgument || method == null) { // null key is allowed since CPython 3.8
return max(context, x);
}

object? ret = i.Current;
object? retValue = PythonCalls.Call(context, method, i.Current);
PythonContext pc = context.LanguageContext;
Expand All @@ -858,57 +859,40 @@ public static object locals(CodeContext/*!*/ context) {
}

public static object? max(CodeContext/*!*/ context, object? x, object? y, [ParamDictionary] IDictionary<string, object?> dict) {
var kwargTuple = GetMaxKwArg(dict, isDefaultAllowed: false);
object? method = kwargTuple.Item1;
VerifyKeysNoDefault(nameof(max), dict, out object? method);

if (method == null) return max(context, x, y); // null key is allowed since CPython 3.8

PythonContext pc = context.LanguageContext;
return pc.GreaterThan(PythonCalls.Call(context, method, x), PythonCalls.Call(context, method, y)) ? x : y;
return pc.GreaterThan(PythonCalls.Call(context, method, y), PythonCalls.Call(context, method, x)) ? y : x;
}

public static object? max(CodeContext/*!*/ context, [ParamDictionary] IDictionary<string, object?> dict, [NotNone] params object?[] args) {
var kwargTuple = GetMaxKwArg(dict, isDefaultAllowed: false);
object? method = kwargTuple.Item1;
if (args.Length == 0) throw PythonOps.TypeError("max expected 1 arguments, got 0");

if (method == null) return max(context, args); // null key is allowed since CPython 3.8
VerifyKeysNoDefault(nameof(max), dict, out object? method);

if (args.Length > 0) {
int retIndex = 0;
if (args.Length == 1) {
return max(context, args[retIndex], dict);
}

object? retValue = PythonCalls.Call(context, method, args[retIndex]);
PythonContext pc = context.LanguageContext;
for (int i = 1; i < args.Length; i++) {
object? tmpRetValue = PythonCalls.Call(context, method, args[i]);
if (method == null) return max(context, args); // null key is allowed since CPython 3.8
if (args.Length == 1) return max(context, args[0], dict);

if (pc.GreaterThan(tmpRetValue, retValue)) {
retIndex = i;
retValue = tmpRetValue;
}
int retIndex = 0;
object? retValue = PythonCalls.Call(context, method, args[retIndex]);
PythonContext pc = context.LanguageContext;
for (int i = 1; i < args.Length; i++) {
object? tmpRetValue = PythonCalls.Call(context, method, args[i]);
if (pc.GreaterThan(tmpRetValue, retValue)) {
retIndex = i;
retValue = tmpRetValue;
}
return args[retIndex];
} else {
throw PythonOps.TypeError("max expected 1 arguments, got 0");
}
return args[retIndex];
}

private static Tuple<object?, object?> GetMaxKwArg(IDictionary<string, object?> dict, bool isDefaultAllowed) {
if (dict.Count != 1 && dict.Count != 2)
throw PythonOps.TypeError("max() should have only 2 keyword arguments, but got {0} keyword arguments", dict.Count);
if (dict.ContainsKey("default") && !isDefaultAllowed) {
throw PythonOps.TypeError("Cannot specify a default for max() with multiple positional arguments");
}

return VerifyKeys("max", dict);
}

public static object? min(CodeContext/*!*/ context, object? x) {
private static object? MinWithDefault(CodeContext/*!*/ context, object? x, object? def) {
IEnumerator i = PythonOps.GetEnumerator(context, x);
if (!i.MoveNext()) {
throw PythonOps.ValueError("empty sequence");
if (def != UndefinedKeywordArgument) return def;
throw PythonOps.ValueError("min() arg is an empty sequence");
}
object? ret = i.Current;
PythonContext pc = context.LanguageContext;
Expand All @@ -918,9 +902,11 @@ public static object locals(CodeContext/*!*/ context) {
return ret;
}

public static object? min(CodeContext/*!*/ context, object? x, object? y) {
return context.LanguageContext.LessThan(x, y) ? x : y;
}
public static object? min(CodeContext/*!*/ context, object? x)
=> MinWithDefault(context, x, UndefinedKeywordArgument);

public static object? min(CodeContext/*!*/ context, object? x, object? y)
=> context.LanguageContext.LessThan(y, x) ? y : x;

public static object? min(CodeContext/*!*/ context, [NotNone] params object?[] args) {
if (args.Length > 0) {
Expand All @@ -940,19 +926,18 @@ public static object locals(CodeContext/*!*/ context) {
}

public static object? min(CodeContext/*!*/ context, object? x, [ParamDictionary] IDictionary<string, object?> dict) {
VerifyKeys(nameof(min), dict, out object? method, out object? def);

if (method == UndefinedKeywordArgument || method == null) { // null key is allowed since CPython 3.8
return MinWithDefault(context, x, def);
}

IEnumerator i = PythonOps.GetEnumerator(context, x);
var kwargTuple = GetMinKwArg(dict, isDefaultAllowed: true);
object? method = kwargTuple.Item1;
object? def = kwargTuple.Item2;
if (!i.MoveNext()) {
if (def != UndefinedKeywordArgument) return def;
throw PythonOps.ValueError("min() arg is an empty sequence");
}

if (method == UndefinedKeywordArgument || method == null) { // null key is allowed since CPython 3.8
return min(context, x);
}

object? ret = i.Current;
object? retValue = PythonCalls.Call(context, method, i.Current);
PythonContext pc = context.LanguageContext;
Expand All @@ -967,82 +952,59 @@ public static object locals(CodeContext/*!*/ context) {
}

public static object? min(CodeContext/*!*/ context, object? x, object? y, [ParamDictionary] IDictionary<string, object?> dict) {
var kwargTuple = GetMinKwArg(dict, isDefaultAllowed: false);
object? method = kwargTuple.Item1;
VerifyKeysNoDefault(nameof(min), dict, out object? method);

if (method == null) return min(context, x, y); // null key is allowed since CPython 3.8

return context.LanguageContext.LessThan(PythonCalls.Call(context, method, x), PythonCalls.Call(context, method, y)) ? x : y;
return context.LanguageContext.LessThan(PythonCalls.Call(context, method, y), PythonCalls.Call(context, method, x)) ? y : x;
}

public static object? min(CodeContext/*!*/ context, [ParamDictionary] IDictionary<string, object?> dict, [NotNone] params object?[] args) {
var kwargTuple = GetMinKwArg(dict, isDefaultAllowed: false);
object? method = kwargTuple.Item1;
if (args.Length == 0) throw PythonOps.TypeError("min expected 1 arguments, got 0");

if (method == null) return min(context, args); // null key is allowed since CPython 3.8
VerifyKeysNoDefault(nameof(min), dict, out object? method);

if (args.Length > 0) {
int retIndex = 0;
if (args.Length == 1) {
return min(context, args[retIndex], dict);
}

object? retValue = PythonCalls.Call(context, method, args[retIndex]);
PythonContext pc = context.LanguageContext;

for (int i = 1; i < args.Length; i++) {
object? tmpRetValue = PythonCalls.Call(context, method, args[i]);
if (method == null) return min(context, args); // null key is allowed since CPython 3.8
if (args.Length == 1) return min(context, args[0], dict);

if (pc.LessThan(tmpRetValue, retValue)) {
retIndex = i;
retValue = tmpRetValue;
}
int retIndex = 0;
object? retValue = PythonCalls.Call(context, method, args[retIndex]);
PythonContext pc = context.LanguageContext;
for (int i = 1; i < args.Length; i++) {
object? tmpRetValue = PythonCalls.Call(context, method, args[i]);
if (pc.LessThan(tmpRetValue, retValue)) {
retIndex = i;
retValue = tmpRetValue;
}
return args[retIndex];
} else {
throw PythonOps.TypeError("min expected 1 arguments, got 0");
}
return args[retIndex];
}

private static Tuple<object?, object?> GetMinKwArg(IDictionary<string, object?> dict, bool isDefaultAllowed) {
if (dict.Count != 1 && dict.Count != 2)
throw PythonOps.TypeError("min() should have only 2 keyword arguments, but got {0} keyword arguments", dict.Count);

if (dict.ContainsKey("default") && !isDefaultAllowed)
throw PythonOps.TypeError("Cannot specify a default for min() with multiple positional arguments");

return VerifyKeys("min", dict);
private static void VerifyKeysNoDefault(string name, IDictionary<string, object?> dict, out object? method) {
VerifyKeys(name, dict, out method, out object? def);
if (def != UndefinedKeywordArgument)
throw PythonOps.TypeError("Cannot specify a default for {0}() with multiple positional arguments", name);
}

private static Tuple<object?, object?> VerifyKeys(string name, IDictionary<string, object?> dict) {
object? method;
object? def;

int cnt = 0;
if (dict.TryGetValue("key", out method)) {
cnt++;
} else {
method = UndefinedKeywordArgument;
}

if (dict.TryGetValue("default", out def)) {
cnt++;
} else {
def = UndefinedKeywordArgument;
}

private static void VerifyKeys(string name, IDictionary<string, object?> dict, out object? method, out object? def) {
if (dict.Count > 2) {
throw PythonOps.TypeError("{1}() takes at most 2 keyword arguments ({0} given)", dict.Count, name);
throw PythonOps.TypeError("{0}() takes at most 2 keyword arguments ({1} given)", name, dict.Count);
}

if (dict.Count > cnt) {
var key = dict.Keys.Cast<string>().Where(x => x != "default" && x != "key").First();
throw PythonOps.TypeError("{1}() got an unexpected keyword argument ({0})", key, name);
}

var result = Tuple.Create(method, def);
method = def = UndefinedKeywordArgument;

return result;
foreach (var pair in dict) {
switch (pair.Key) {
case "key":
method = pair.Value;
break;
case "default":
def = pair.Value;
break;
default:
throw PythonOps.TypeError("{1}() got an unexpected keyword argument '{0}'", pair.Key, name);
}
}
}

/// <summary>
Expand Down
47 changes: 43 additions & 4 deletions tests/suite/test_builtinfunc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,14 @@ def test_max(self):
self.assertEqual(max(1, 2.0, 3), 3)
self.assertEqual(max(1.0, 2, 3), 3)

self.assertRaises(TypeError, max)
self.assertRaisesRegex(TypeError, "expected", max)
self.assertRaisesRegex(TypeError, "expected", max, key=int)
self.assertRaisesRegex(TypeError, "expected", max, default=None)
self.assertRaisesRegex(TypeError, "expected", max, default=None, key=int)
self.assertRaises(TypeError, max, 42)
self.assertRaises(ValueError, max, ())
self.assertRaisesRegex(ValueError, "empty", max, ())
self.assertRaisesRegex(ValueError, "empty", max, iter(()))
self.assertRaisesRegex(ValueError, "empty", max, (), key=neg)
self.assertRaises(TypeError, max, (1, 2), None)
class BadSeq:
def __getitem__(self, index):
Expand Down Expand Up @@ -358,6 +363,20 @@ def __getitem__(self, index):
f = keys.__getitem__
self.assertEqual(max(data, key=f), sorted(reversed(data), key=f)[-1])

# iterator with default and no key
self.assertEqual(max(iter([2, 1, 0]), default=None), 2)
self.assertEqual(max(iter([]), default=None), None)
self.assertEqual(max([], key=None, default=1), 1)

# first max item wins
self.assertIs(type(max(1, 1.0)), int)
self.assertIs(type(max(1, 1.0, key=neg)), int)
self.assertIs(type(max([1, 1.0])), int)
self.assertIs(type(max(0, 1, 1.0)), int)
self.assertIs(type(max(0, 1, 1.0, key=abs)), int)
self.assertIs(type(max([0, 1, 1.0], key=abs)), int)
self.assertTrue(math.isnan(max(float('nan'), 1)))
self.assertEqual(max(1, float('nan')), 1)

def test_min(self):
self.assertEqual(min('123123'), '1')
Expand All @@ -369,9 +388,14 @@ def test_min(self):
self.assertEqual(min(1, 2.0, 3), 1)
self.assertEqual(min(1.0, 2, 3), 1.0)

self.assertRaises(TypeError, min)
self.assertRaisesRegex(TypeError, "expected", min)
self.assertRaisesRegex(TypeError, "expected", min, key=int)
self.assertRaisesRegex(TypeError, "expected", min, default=None)
self.assertRaisesRegex(TypeError, "expected", min, default=None, key=int)
self.assertRaises(TypeError, min, 42)
self.assertRaises(ValueError, min, ())
self.assertRaisesRegex(ValueError, "empty", min, ())
self.assertRaisesRegex(ValueError, "empty", min, iter(()))
self.assertRaisesRegex(ValueError, "empty", min, (), key=neg)
class BadSeq:
def __getitem__(self, index):
raise ValueError
Expand Down Expand Up @@ -410,6 +434,21 @@ def __getitem__(self, index):
f = keys.__getitem__
self.assertEqual(min(data, key=f), sorted(data, key=f)[0])

# iterator with default and no key
self.assertEqual(min(iter([0, 1, 2]), default=None), 0)
self.assertEqual(min(iter([]), default=None), None)
self.assertEqual(min([], key=None, default=1), 1)

# first min item wins
self.assertIs(type(min(1, 1.0)), int)
self.assertIs(type(min(1, 1.0, key=neg)), int)
self.assertIs(type(min([1, 1.0])), int)
self.assertIs(type(min(2, 1, 1.0)), int)
self.assertIs(type(min(2, 1, 1.0, key=abs)), int)
self.assertIs(type(min([2, 1, 1.0], key=abs)), int)
self.assertTrue(math.isnan(min(float('nan'), 1)))
self.assertEqual(min(1, float('nan')), 1)

def test_abs(self):
self.assertRaises(TypeError,abs,None)

Expand Down
Loading