orchestrion icon indicating copy to clipboard operation
orchestrion copied to clipboard

Add ResultHasError Function to Function Type for Simplified Error Detection

Open korECM opened this issue 10 months ago • 3 comments

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

korECM avatar Feb 09 '25 15:02 korECM

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.

korECM avatar Feb 09 '25 15:02 korECM

Maybe here?

korECM avatar Feb 10 '25 13:02 korECM

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.

eliottness avatar Feb 11 '25 15:02 eliottness