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

proposal: cmd/go: support peer dependencies #63457

Open
mitar opened this issue Oct 9, 2023 · 13 comments
Open

proposal: cmd/go: support peer dependencies #63457

mitar opened this issue Oct 9, 2023 · 13 comments

Comments

@mitar
Copy link
Contributor

mitar commented Oct 9, 2023

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

  • People are trying to remove github.com/pkg/errors dependency.
  • gitlab.com/tozd/go/errors might be a good alternative.
  • But gitlab.com/tozd/go/errors also has a dependency on github.com/pkg/errors so that it works better in codebases where github.com/pkg/errors is used as well and can be interoperable with github.com/pkg/errors.

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.

@mitar mitar added the Proposal label Oct 9, 2023
@gopherbot gopherbot added this to the Proposal milestone Oct 9, 2023
@AlexanderYastrebov
Copy link
Contributor

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

I think you meant github.com/pkg/errors

@mitar
Copy link
Contributor Author

mitar commented Oct 9, 2023

@AlexanderYastrebov Thanks, fixed.

@ianlancetaylor ianlancetaylor changed the title proposal: Support peer dependencies proposal: cmd/go: support peer dependencies Oct 9, 2023
@ianlancetaylor ianlancetaylor added the GoCommand cmd/go label Oct 9, 2023
@ianlancetaylor
Copy link
Contributor

CC @bcmills @matloob

@icholy
Copy link

icholy commented Oct 10, 2023

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

mitar commented Oct 10, 2023

@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
Copy link
Contributor

Can you use generics to solve this?

type StackTrace interface {
    ~[]uintptr
}

type StackTracer[S StackTrace] interface {
    StackTrace() S
}

@mitar
Copy link
Contributor Author

mitar commented Oct 10, 2023

@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
Copy link
Contributor

earthboundkid commented Oct 10, 2023

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

mitar commented Oct 10, 2023

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

icholy commented Oct 13, 2023

Seems like you need a v2 of your module.

@mitar
Copy link
Contributor Author

mitar commented Oct 13, 2023

Seems like you need a v2 of your module.

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

@icholy
Copy link

icholy commented Oct 14, 2023

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

@mitar
Copy link
Contributor Author

mitar commented Oct 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Incoming
Development

No branches or pull requests

7 participants