Add ResultHasError Function to Function Type for Simplified Error Detection
As discussed in https://github.com/DataDog/orchestrion/discussions/537, to implement this functionality we need to add a ResultHasError function to the Function type in the orchestrion project and modify the dd-trace-go repository to make use of it.
I'm opening this issue to propose the ResultHasError function.
If there are no objections, I'd like to work on this.
Implementation
func (f *Function) ResultHasError() string {
// First, check for the most common case: a result of type "error"
for _, result := range f.Results {
if result.Type.String() == "error" {
return result.Name
}
}
// For other error types, only then check with types.Implements
for _, result := range f.Results {
if types.Implements(result.Type, errorInterface) {
return result.Name
}
}
return ""
}
I haven't run any benchmarks, but I believe a simple string comparison is much faster than using the types.Implements method, and since most users are likely using the native error type, checking for that first before falling back to types.Implements seems like a good approach.
Would it be acceptable to proceed with a PR using code similar to the above?
Additionally, the corresponding Issue for dd-trace-go can be found here: https://github.com/DataDog/dd-trace-go/issues/3168
However, I haven't been able to locate exactly where this function should be implemented yet, so I'd appreciate any guidance on this matter.
Maybe here?
You guessed right, adding this new method on the signature type should be the right move here. From there, we also need to add the method signature to the function interface but this is overall the easy part. The hard part is getting all the arguments to actually do a real call to go/types.Implements().
For that you need to move the data from the go/types package which is here: https://github.com/DataDog/orchestrion/blob/020b6fef48811e384992a8286c6c0c405d46fc27/internal/injector/injector.go#L183
All the way to the signature struct in dot_function.go. For this, we have the AdviceContext interface that is available in the dot struct which is the top-level template helper where adding a function ResolveType(dst.Expr) types.Type should be enough to get all the data you need.
Maybe making a more general function like ResultImplements that would take as parameter the string of the type in question could also be good. I will let you decide if you believe this is worth it.