Skip to content
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

[BUG] - false positive errors on verify types of paramters #798

Open
c14h3 opened this issue Nov 11, 2024 · 1 comment
Open

[BUG] - false positive errors on verify types of paramters #798

c14h3 opened this issue Nov 11, 2024 · 1 comment

Comments

@c14h3
Copy link

c14h3 commented Nov 11, 2024

gocron/scheduler.go

Lines 577 to 616 in 31de1e8

func (s *scheduler) verifyVariadic(taskFunc reflect.Value, tsk task, variadicStart int) error {
if err := s.verifyNonVariadic(taskFunc, tsk, variadicStart); err != nil {
return err
}
parameterType := taskFunc.Type().In(variadicStart).Elem().Kind()
if parameterType == reflect.Interface {
return s.verifyInterfaceVariadic(taskFunc, tsk, variadicStart)
}
if parameterType == reflect.Pointer {
parameterType = reflect.Indirect(reflect.ValueOf(taskFunc.Type().In(variadicStart))).Kind()
}
for i := variadicStart; i < len(tsk.parameters); i++ {
argumentType := reflect.TypeOf(tsk.parameters[i]).Kind()
if argumentType == reflect.Interface || argumentType == reflect.Pointer {
argumentType = reflect.TypeOf(tsk.parameters[i]).Elem().Kind()
}
if argumentType != parameterType {
return ErrNewJobWrongTypeOfParameters
}
}
return nil
}
func (s *scheduler) verifyNonVariadic(taskFunc reflect.Value, tsk task, length int) error {
for i := 0; i < length; i++ {
t1 := reflect.TypeOf(tsk.parameters[i]).Kind()
if t1 == reflect.Interface || t1 == reflect.Pointer {
t1 = reflect.TypeOf(tsk.parameters[i]).Elem().Kind()
}
t2 := reflect.New(taskFunc.Type().In(i)).Elem().Kind()
if t2 == reflect.Interface || t2 == reflect.Pointer {
t2 = reflect.Indirect(reflect.ValueOf(taskFunc.Type().In(i))).Kind()
}
if t1 != t2 {
return ErrNewJobWrongTypeOfParameters
}
}
return nil
}

Issue 1: providing a function like Foo(bar any) t2 will be of type interface and its indirect type is struct, so if t1 is not of type struct it will cause an error but bar can handle everything, which leads into false positive error
Issue 2: providing a function like Foo(bar **int) t2 will be of type pointer and its indirect type is also pointer, so t1 may not point to the correct type of t2, which leads into false positive no error
Issue 3: providing a function like Foo(bar struct{}) t2 will be of type struct but struct != struct, so t1 must match the syntax of t2 struct. just checking struct must be struct may lead into false positive no error

I first considered open a pull request, but solving the issues is not trivial, so I would prefer to just omit the verify check and let the implementation raise panics if devs define task values wrong.

@c14h3 c14h3 changed the title false positive errors on verify types of paramters [BUG] - false positive errors on verify types of paramters Nov 11, 2024
@JohnRoesler
Copy link
Contributor

JohnRoesler commented Nov 20, 2024

hey @c14h3

Could you help me with debugging these issue by providing code examples?

  • Issue 1: https://github.com/go-co-op/gocron/blob/v2/scheduler_test.go#L948-L952, is this specifically an issue with variadic any? Or something else?

  • Issue 2: **int as a function type isn't allowed. Could you help with an example here?

  • Issue 3: agree, this is a current issue that if the struct isn't the same struct it will still pass. And then, fail silently, as in the job just won't run. I haven't pinpointed the issue here yet, but very interesting...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants