kkHAIKE / contextcheck

Analyzer: check whether a function uses a non-inherited context
Apache License 2.0
43 stars 3 forks source link

contextcheck doesn’t recognize Context implementations besides STL #20

Open FGasper opened 1 year ago

FGasper commented 1 year ago

The following fails when I plug it into contextcheck’s test suite:

package customcontext

import (
    "context"
    "time"
)

var _ context.Context = &MyContext{}

func f1(ctx context.Context) {
    passesContextTODO()   // want "Function `passesContextTODO` should pass the context parameter"
    passesMyContextTODO() // want "Function `passesMyContextTODO` should pass the context parameter"
}

func needsSTLCtx(ctx context.Context) {}
func needsCustomCtx(ctx *MyContext)   {}

func passesContextTODO() {
    needsSTLCtx(context.TODO())
}

func passesMyContextTODO() {
    needsCustomCtx(&MyContext{})
}

// ------------------------------

type MyContext struct {
}

func (ctx *MyContext) Deadline() (time.Time, bool) {
    return time.Now(), true
}

func (ctx *MyContext) Done() <-chan struct{} {
    return make(chan struct{})
}

func (ctx *MyContext) Err() error {
    return nil
}

func (ctx *MyContext) Value(_ any) any {
    return nil
}
kkHAIKE commented 1 year ago

This case was considered before, but later we decided on the standard for using custom context: Instead of directly using this custom struct, we should pass it through context.Context and retrieve the custom structure using ctx.Value, or use both ctx and the custom struct as two parameters. Using the custom struct to pass context is not very intuitive in the code.

FGasper commented 1 year ago

If I’m understanding you correctly, you’re saying that only the standard library should really implement the context.Context interface. The standard library’s own documentation makes no such claim, so I’m a bit surprised at the notion.

For better or for worse, my own project has a custom implementation, and we can’t easily change that.

Would you accept a PR to add this flexibility, or do you prefer only to support the standard library’s Context?

kkHAIKE commented 1 year ago

I must have missed one scenario. 😭

  1. func Do(ctx context.Context, args int) {
        customCtx := ctx.Value(customKey{})
    }
    func Use() {
        ctx := context.WithValue(context.Background(), customKey{}, new(customCtxType))
        Do(ctx, 1)
    }
  2. var _ context.Context = new(customCtxType)
    func Do(ctx context.Context, args int) {
        customCtx := ctx.(*customCtxType)
    }
    func Use() {
        customCtx := new(customCtxType)
        Do(customCtx, 1)
    }
  3. var _ context.Context = new(customCtxType)
    func Do(ctx context.Context, customCtx *customCtxType, args int) {
    }
    func Use() {
         customCtx := new(customCtxType)
         Do(customCtx, customCtx, 1)
    }