6b5d14c3 breaks resolution of runtime error locations in macros

[efd6]

Describe the bug

We use the NodeID() value from runtime errors to be able to locate runtime errors in programs. The change in 6b5d14c (identified by git bisect) appears to have broken this for nodes that exist within the AST in macro expansions. After the change, errors are noted as existing at the site of the unexpanded macro, rather than at the site within the macro's body.

For example with the following program:

get(state.url).Body.decode_json().records.map(r,
	get(state.url+'/'+r.id).Body.decode_json()).as(events, {
//                    ^~~~ r.id not converted to string: can't add integer to string.
		"events": events,
})

note that in this context, state.url is a dyn that is not known until runtime.

(before):

failed eval: ERROR: <input>:3:26: no such overload
 |   get(state.url+'/'+r.id).Body.decode_json()).as(events, {
 | .........................^

(after):

failed eval: ERROR: <input>:5:14: no such overload
 |    "events": events,
 | .............^

Neither is perfect, but the one after the change is worse.

To Reproduce
Check which components this affects:

• parser
• checker
• interpreter

Test setup:

Best done by reference to github.com/elastic/mito@48813c5bbc46326feb15c8aab98cb08bae2c1fa6 and the corresponding test adjustment in github.com/elastic/mito@6bd3ded032c5cd2bb44ca0aad50a14a80e736857

I can put together a smaller reproducer, but I'd like to get an idea of whether this is something that we just have to live with before I do that>

Expected behavior

Something more similar to the "before" case.

[TristonianJones]

@efd6 it's hard to know why the commit would impact the error id you're seeing. The change in question simply made sure that if errors were provided as input, they were identified as errors and returned up the stack so they could be properly identified. It sounds as though the input error to the activation somehow contains an error result from a previous expression which causes the loss of id ... is that accurate?

[efd6]

My bisection used go.mod replace directives, so I can investigate that, I'll report back.

[efd6]

OK, so yes, the issue is that the ID is dropped due to the unwrapping, although this is not the full story; losing the resolution means that the caller is never handed back the object indicating the location which it is presumably expecting.

[TristonianJones]

@efd6 Can I ask why the evaluation continues with an error as input? CEL isn't usually expecting an error to be provided as input, but rather emitted as a by-product of evaluation. If this is an expression composition case, perhaps it would be better to use unknowns as inputs so you can capture that the particular variable provided was the result of a failed computation? If you're trying to coordinate error messages across expressions, the local IDs of each expression are likely to be problematic

[efd6]

The code there is a test case, but the real world thing it is reflecting is that users may provide invalid programs or invalid data to valid programs. We would like to be able to pin-point where the errors occur to make it easier to debug these cases. We are not correlating IDs across expressions, instead in the case of errors, the host has the AST and source and makes use of that, with the expr ID to render the runtime error.