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

contextcheck doesn’t recognize Context implementations besides STL #20

Open
FGasper opened this issue Sep 11, 2023 · 4 comments
Open

Comments

@FGasper
Copy link

FGasper commented Sep 11, 2023

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
Copy link
Owner

kkHAIKE commented Sep 18, 2023

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
Copy link
Author

FGasper commented Sep 18, 2023

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
Copy link
Owner

kkHAIKE commented Sep 20, 2023

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)
     }

@CMajeri
Copy link

CMajeri commented Sep 29, 2024

I have an interface that embeds the standard context.Context, and the linter pretty much doesn't recognize it as such.
Are there plans to support that? Would you accept a PR?

use both ctx and the custom struct as two parameters

is super cumbersome and muddles the code imo

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

3 participants