golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.04k stars 17.54k forks source link

proposal: cmd/go: support peer dependencies #63457

Open mitar opened 11 months ago

mitar commented 11 months ago

Background

I am the author of the gitlab.com/tozd/go/errors, a modern drop-in replacement for github.com/pkg/errors which enables errors to be annotated with stack traces and structured details. It is a new package with fresh implementation and is not a fork of github.com/pkg/errors, but it does attempt to keep compatibility with it by reusing its API and by supporting reusing stack traces made by github.com/pkg/errors. The latter is important because in large codebases it can happen that both github.com/pkg/errors and gitlab.com/tozd/go/errors are used.

github.com/pkg/errors exposes its stack trace using the following interface:

type stackTracer interface {
        StackTrace() errors.StackTrace
}

errors.StackTrace is a type (underlying type is []uintptr) exported by github.com/pkg/errors. So to be able to access the stack trace, one has to import github.com/pkg/errors to be able to define the interface. To extract the stack trace (just the underlying []uintptr data), I do something (approximately) like:

switch e := err.(type) {
case interface {StackTrace() errors.StackTrace}:
    st := e.StackTrace()
    return *(*[]uintptr)(unsafe.Pointer(&st))
}

github.com/pkg/errors has been archived in 2021 and is not maintained anymore. Because of this people are searching for maintained alternatives. It is not that github.com/pkg/errors has any known security issues, but it does lack newer features (like errors.Join) and in general people seem to not enjoy having an unmaintained dependency. From what I have seen, even to a degree that they blocklist the dependency using linter tools.

The issue

The proposal

NPM has peer dependencies so solve exactly this problem. I think it could be useful for Go as well. The idea would be that a separate list of dependencies is made in go.mod. So you would have direct, indirect, and peer dependencies. And if that peer dependency is satisfied, an automatic build flag would enable, including the file which uses that peer dependency. (I am open to how that build flag format would look like.)

Alternatives

Build tags seems to be one alternative. I could define a build tag one could use to enable/disable checking for github.com/pkg/errors stack trace on errors (and importing the StackTrace symbol). But the issue is that dependency in go.mod still stays which can trick tools into thinking that the package still uses github.com/pkg/errors.

AlexanderYastrebov commented 11 months ago

gitlab.com/tozd/go/errors has been archived in 2021 and is not maintained anymore.

I think you meant github.com/pkg/errors

mitar commented 11 months ago

@AlexanderYastrebov Thanks, fixed.

ianlancetaylor commented 11 months ago

CC @bcmills @matloob

icholy commented 11 months ago

Peer Dependencies in Node exist to prevent multiple copies of the same package. If you don't explicitly specify a dependency on a transitive peer dependency, the version (of the peer dependency) specified by the direct dependency is used. This is very different from the behaviour you're proposing.

mitar commented 11 months ago

@icholy OK, then maybe my terminology is wrong here? Should it be called "optional dependency"?

I am not married to the concrete proposal here. But the use case is pretty real.

In addition: my package does not really care about the version of the github.com/pkg/errors (or the version it supports is pretty wide) so it also does not make much sense to force the version. (In github.com/pkg/errors case though, probably everyone is using the latest available version anyway.)

earthboundkid commented 11 months ago

Can you use generics to solve this?

type StackTrace interface {
    ~[]uintptr
}

type StackTracer[S StackTrace] interface {
    StackTrace() S
}
mitar commented 11 months ago

@carlmjohnson Hm, can you share play link how would this work? My understanding is that I would still have to make pass errors.StackTrace to S type parameter?

earthboundkid commented 11 months ago

I was thinking if you had a top level function that accepted a StackTracer, it could work with a tozd StackTrace or a pkg StackTrace.

Looking at the code for getExistingStackTrace, I would rewrite it to use reflect.

// Does it have a StackTrace method?
m, ok := reflect.TypeOf(err).MethodByName("StackTrace")
// Does StackTrace have no input and one output? 
if ok && m.Type.NumIn() == 0 && m.Type.NumOut() == 1 {
  outType := m.Type.Out(0)
  // Is the output a []uintptr?
  if outType.Kind == reflect.Slice && outType.Elem().Kind() == reflect.Uintptr {
     // Do something with m
  }
}
mitar commented 11 months ago

@carlmjohnson: getExistingStackTrace is used quite a lot (every time you make an error), so I would prefer if it does not use reflect to be more performant. But maybe that could be build tag opt-in feature. But I do suspect that many users will be OK having a type-only dependency on github.com/pkg/errors if that means better performance. So an optional dependency might still be a way to go.

icholy commented 11 months ago

Seems like you need a v2 of your module.

mitar commented 11 months ago

Seems like you need a v2 of your module.

Of which module? github.com/pkg/errors is archived and cannot be changed.

icholy commented 11 months ago

@mitar I mean a v2 of your module which doesn't integrate with pkg/errors

mitar commented 11 months ago

Sure. But the idea is that users should be able to decide if they want or not compatibility with pkg/errors. This is a feature I would like to have.