fix(patcher): ctx into nested custom funcs and Env#883
fix(patcher): ctx into nested custom funcs and Env#883thevilledev wants to merge 1 commit intoexpr-lang:masterfrom
Conversation
|
I had an idea to maybe fix this through a checker-level fix but it turned out to be quite cumbersome due to side-effects to all the other expressions. Lemme know what you think @antonmedv 👍 |
patcher/with_context.go
Outdated
| // If callee type is interface{} (unknown), try to look up the function type | ||
| // from the Functions table. This handles cases where the call is nested | ||
| // inside another call with an unknown callee type. | ||
| if fn.Kind() == reflect.Interface && w.Functions != nil { |
There was a problem hiding this comment.
But why date2() callnode type is any?
There was a problem hiding this comment.
IIUC the checker returns early without visiting date2() because now2() initially has 0 arguments, as the context has not been injected yet. This causes checkArguments to fail. Furthermore it makes the now2() call return unknown type. Then it cascades: memberNode for .After returns unknown, so the outer callNode returns early without visiting its arguments. And it leaves date2() identifier with interface{} type when WithContext runs.
I was playing around with the Checker-level fix like:
if nt.IsUnknown(&v.config.NtCache) {
// Still visit arguments to set their types for patchers
for _, arg := range node.Arguments {
v.visit(arg)
}
return Nature{}
}But it breaks a bunch of stuff, and I went with this. :)
I'll update the comment a bit.
There was a problem hiding this comment.
Thanks for explanation, let me also see if checker solution can be added.
| type WithContext struct { | ||
| Name string | ||
| Name string | ||
| Functions conf.FunctionsTable // Optional: used to look up function types when callee type is unknown. |
There was a problem hiding this comment.
A new function can be also defined as a method of Env. Should it be checked there too?
There was a problem hiding this comment.
Thanks, good point! I'll check this out. And lets add a regression test for it too.
There was a problem hiding this comment.
Functions table and Env.MethodByName() now also included!
WithContext patcher now looks up function types from the Functions
table and Env methods when the callee type is interface{}. This fixes
context injection for custom functions and Env methods nested as
arguments inside method calls with unknown callee types
(e.g., Now2().After(Date2())).
Also improved the regression test to actually verify context is passed
to both functions, and added a test for Env methods.
Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
thevilledev
force-pushed
the
fix/issue-823-part2
branch
from
4857426 to
51c878b
Compare
thevilledev
changed the title
3 participants
WithContext patcher now looks up function types from the Functions table and Env methods when the callee type is
interface{}. This fixes context injection for custom functions and Env methods nested as arguments inside method calls with unknown callee types (e.g.,Now2().After(Date2())).Also improved the regression test to actually verify context is passed to both functions, and added a test for Env methods.
Should finally fix #823.