-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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.
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
Core: Fix Callable.get_bound_arguments{,_count}()
return incorrect data
#98713
Core: Fix Callable.get_bound_arguments{,_count}()
return incorrect data
#98713
Conversation
e305498
to
423aca0
Compare
Thanks very much for your quick fix. BTW, I just figured out that unbind() can not only discard the arguments passed to the
which is not that accurate. |
423aca0
to
7f65c4d
Compare
I will try to give a formal proof of why this works and why we first remove unbound arguments and then add bound ones, and not vice versa. 1. Successive calls of the same name can be reduced to the form 1.1. 1.2. 2. Successive calls of different names can be reduced to the form 2.1. 2.2.
Since So, 3. Any sequence of
So, it will be either So, it will be either |
Very impresive! I hope your summarization can be added on the Godot's documentation which can help many users to better understand the design of bind and unbind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The theory checks out and the tests provided covers all the cases I think are necessary, didn't do any deeper analysis of the code at this time but it looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the current behavior of chained Callable.bind
/Callable.unbind
calls is correct / we don't want to change that, this fix looks correct to me.
The previous behavior of Callable.get_bound_arguments
/ Callable.get_bound_arguments_count
makes not much sense for chained binds/unbinds indeed.
The theory in #98713 (comment) looks good, code looks fine too. I've tested it some further in action and everything seems to work as expected. 👍
Test script
@tool
extends EditorScript
func foo(a = 0, b = 0, c = 0, d = 0, e = 0, f = 0, g = 0, h = 0):
return [a, b, c, d, e, f, g, h]
var foo_lambda: Callable = func (a = 0, b = 0, c = 0, d = 0, e = 0, f = 0, g = 0, h = 0):
return [a, b, c, d, e, f, g, h]
func test(c: Callable, repr: String = "") -> void:
print("%-18s %-22s %2d %2d %2d %-9s %s" % [
repr,
".call(5, 6, 7, 8, 9)" if c.get_unbound_arguments_count() <= 5 else "CAN'T .call(5, 6, 7, 8, 9)",
c.get_argument_count(),
c.get_unbound_arguments_count(),
c.get_bound_arguments_count(),
c.get_bound_arguments(),
c.call(5, 6, 7, 8, 9) if c.get_unbound_arguments_count() <= 5 else "N/A",
])
##4.3
#print("%-18s %-22s %2d %2s %2d %-9s %s" % [
#repr,
#".call(5, 6, 7, 8, 9)",
#c.get_argument_count(),
#"-",
#c.get_bound_arguments_count(),
#c.get_bound_arguments(),
#c.call(5, 6, 7, 8, 9),
#])
func test_subcalls(c: Callable, subcalls: Array[Subcall]) -> void:
var repr: String = c.get_method()
test(c, repr)
for i in subcalls.size():
var subcall: Subcall = subcalls[i]
c = subcall.apply_to(c)
repr = "%s%s" % [" ".repeat(i), subcall]
test(c, repr)
print_separator()
func print_separator() -> void:
print("-".repeat(90))
func Bind(args: Array) -> Subcall: return Subcall.new(Subcall.Type.BIND, args)
func Unbind(count: int) -> Subcall: return Subcall.new(Subcall.Type.UNBIND, [count])
class Subcall:
enum Type { BIND, UNBIND }
var type: Type
var args: Array
func _init(type: Type, args: Array) -> void:
self.type = type
self.args = args
func _to_string() -> String:
return ".%s(%s)" % [
"bind" if type == Type.BIND else "unbind",
", ".join(args),
]
func apply_to(c: Callable) -> Callable:
if type == Type.BIND:
return c.bindv(args)
else:
return c.unbind(args[0])
func _run() -> void:
var f: Callable = foo
#var f: Callable = foo_lambda
test_subcalls(f, [
Bind([1, 2]),
Unbind(1),
Bind([3, 4]),
Unbind(1),
])
for unbind_arg in [1, 3, 5]:
print_separator()
test_subcalls(f, [
Bind([1]),
Bind([2]),
Bind([3]),
Unbind(unbind_arg),
])
test_subcalls(f, [
Bind([1]),
Bind([2]),
Unbind(unbind_arg),
Bind([3]),
])
test_subcalls(f, [
Bind([1]),
Unbind(unbind_arg),
Bind([2]),
Bind([3]),
])
test_subcalls(f, [
Unbind(unbind_arg),
Bind([1]),
Bind([2]),
Bind([3]),
])
print_separator()
test_subcalls(f, [
Unbind(1),
Unbind(1),
Unbind(1),
Bind([1, 2, 3]),
])
test_subcalls(f, [
Unbind(1),
Unbind(1),
Bind([1, 2, 3]),
Unbind(1),
])
test_subcalls(f, [
Unbind(1),
Bind([1, 2, 3]),
Unbind(1),
Unbind(1),
])
test_subcalls(f, [
Bind([1, 2, 3]),
Unbind(1),
Unbind(1),
Unbind(1),
])
print_separator()
test_subcalls(f, [
Bind([1]),
Unbind(2),
Bind([2]),
Unbind(2),
Bind([3]),
Unbind(2),
])
Output (this PR)
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1, 2) .call(5, 6, 7, 8, 9) 6 0 2 [1, 2] [5, 6, 7, 8, 9, 1, 2, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 7 1 2 [1, 2] [5, 6, 7, 8, 1, 2, 0, 0]
.bind(3, 4) .call(5, 6, 7, 8, 9) 5 0 3 [3, 1, 2] [5, 6, 7, 8, 9, 3, 1, 2]
.unbind(1) .call(5, 6, 7, 8, 9) 6 1 3 [3, 1, 2] [5, 6, 7, 8, 3, 1, 2, 0]
------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.bind(3) .call(5, 6, 7, 8, 9) 5 0 3 [3, 2, 1] [5, 6, 7, 8, 9, 3, 2, 1]
.unbind(1) .call(5, 6, 7, 8, 9) 6 1 3 [3, 2, 1] [5, 6, 7, 8, 3, 2, 1, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 7 1 2 [2, 1] [5, 6, 7, 8, 2, 1, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 8 1 1 [1] [5, 6, 7, 8, 1, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 6 0 2 [3, 1] [5, 6, 7, 8, 9, 3, 1, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 9 1 0 [] [5, 6, 7, 8, 0, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 7 0 1 [2] [5, 6, 7, 8, 9, 2, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 6 0 2 [3, 2] [5, 6, 7, 8, 9, 3, 2, 0]
------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.bind(3) .call(5, 6, 7, 8, 9) 5 0 3 [3, 2, 1] [5, 6, 7, 8, 9, 3, 2, 1]
.unbind(3) .call(5, 6, 7, 8, 9) 8 3 3 [3, 2, 1] [5, 6, 3, 2, 1, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.unbind(3) .call(5, 6, 7, 8, 9) 9 3 2 [2, 1] [5, 6, 2, 1, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 8 2 2 [2, 1] [5, 6, 7, 2, 1, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.unbind(3) .call(5, 6, 7, 8, 9) 10 3 1 [1] [5, 6, 1, 0, 0, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 9 2 1 [1] [5, 6, 7, 1, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 8 1 1 [1] [5, 6, 7, 8, 1, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(3) .call(5, 6, 7, 8, 9) 11 3 0 [] [5, 6, 0, 0, 0, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 10 2 0 [] [5, 6, 7, 0, 0, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 9 1 0 [] [5, 6, 7, 8, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.bind(3) .call(5, 6, 7, 8, 9) 5 0 3 [3, 2, 1] [5, 6, 7, 8, 9, 3, 2, 1]
.unbind(5) .call(5, 6, 7, 8, 9) 10 5 3 [3, 2, 1] [3, 2, 1, 0, 0, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 6 0 2 [2, 1] [5, 6, 7, 8, 9, 2, 1, 0]
.unbind(5) .call(5, 6, 7, 8, 9) 11 5 2 [2, 1] [2, 1, 0, 0, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 10 4 2 [2, 1] [5, 2, 1, 0, 0, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.unbind(5) .call(5, 6, 7, 8, 9) 12 5 1 [1] [1, 0, 0, 0, 0, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 11 4 1 [1] [5, 1, 0, 0, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 10 3 1 [1] [5, 6, 1, 0, 0, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(5) .call(5, 6, 7, 8, 9) 13 5 0 [] [0, 0, 0, 0, 0, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 12 4 0 [] [5, 0, 0, 0, 0, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 11 3 0 [] [5, 6, 0, 0, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 10 2 0 [] [5, 6, 7, 0, 0, 0, 0, 0]
------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 9 1 0 [] [5, 6, 7, 8, 0, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 10 2 0 [] [5, 6, 7, 0, 0, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 11 3 0 [] [5, 6, 0, 0, 0, 0, 0, 0]
.bind(1, 2, 3) .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 9 1 0 [] [5, 6, 7, 8, 0, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 10 2 0 [] [5, 6, 7, 0, 0, 0, 0, 0]
.bind(1, 2, 3) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 8 1 1 [1] [5, 6, 7, 8, 1, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 9 1 0 [] [5, 6, 7, 8, 0, 0, 0, 0]
.bind(1, 2, 3) .call(5, 6, 7, 8, 9) 6 0 2 [1, 2] [5, 6, 7, 8, 9, 1, 2, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 7 1 2 [1, 2] [5, 6, 7, 8, 1, 2, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 8 2 2 [1, 2] [5, 6, 7, 1, 2, 0, 0, 0]
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1, 2, 3) .call(5, 6, 7, 8, 9) 5 0 3 [1, 2, 3] [5, 6, 7, 8, 9, 1, 2, 3]
.unbind(1) .call(5, 6, 7, 8, 9) 6 1 3 [1, 2, 3] [5, 6, 7, 8, 1, 2, 3, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 7 2 3 [1, 2, 3] [5, 6, 7, 1, 2, 3, 0, 0]
.unbind(1) .call(5, 6, 7, 8, 9) 8 3 3 [1, 2, 3] [5, 6, 1, 2, 3, 0, 0, 0]
------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------
foo .call(5, 6, 7, 8, 9) 8 0 0 [] [5, 6, 7, 8, 9, 0, 0, 0]
.bind(1) .call(5, 6, 7, 8, 9) 7 0 1 [1] [5, 6, 7, 8, 9, 1, 0, 0]
.unbind(2) .call(5, 6, 7, 8, 9) 9 2 1 [1] [5, 6, 7, 1, 0, 0, 0, 0]
.bind(2) .call(5, 6, 7, 8, 9) 8 1 1 [1] [5, 6, 7, 8, 1, 0, 0, 0]
.unbind(2) .call(5, 6, 7, 8, 9) 10 3 1 [1] [5, 6, 1, 0, 0, 0, 0, 0]
.bind(3) .call(5, 6, 7, 8, 9) 9 2 1 [1] [5, 6, 7, 1, 0, 0, 0, 0]
.unbind(2) .call(5, 6, 7, 8, 9) 11 4 1 [1] [5, 1, 0, 0, 0, 0, 0, 0]
------------------------------------------------------------------------------------------
Some core folks should take a look at this before merging though (I'm not sure if everything needed was changed, if it won't break extensions etc.).
I think we can evaluate down the line exactly the desired behavior, but I think maintaining the behavior of those, as it is at least useful, is more important than these methods, I would say more people would be affected by changes to the binding behavior |
7f65c4d
to
e379cc7
Compare
Thanks! |
get_bound_arguments
is wrong after usingunbind
#98695.Test script
Before:
After:
Note: This is probably unrelated and may not be a bug, but
get_argument_count()
can return negative values for variadic functions, as in the test.