Skip to content

errors: unwrap joinError to avoid deeply-nested joins #65360

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

Closed
wants to merge 1 commit into from

Conversation

sethvargo
Copy link
Contributor

@sethvargo sethvargo commented Jan 30, 2024

This PR is a quick proposal to unwrap joinError (and only joinError)
when calling errors.Join. While I don't think we should unwrap
everything, this is an internal structure that has only internally
semantic meaning. Therefore, I think it makes sense to special-case
joinError and unwrapped the contained errors on joinError.

Pros:

  • More closely mirrors how other error joining packages in the ecosystem
    behave.

  • Even though Unwrap() []error isn't a public API, this will ensure
    that the method behaves as end-users expect for errors.Join,
    returning the list of joined errors.

Cons:

  • Introduces type assertion and could result in a performance
    regression, since we're not pre-computing the slice length. This could
    be addressed if there's interest in pursuing this. I also think there's
    a reasonable argument to be made about the benefits realized from this
    change when printing the error.

  • Users could be depending on the broken behavior, but that seems
    unlikely since joinError is an internal API.

I could not find any corresponding issues, but "errors" and "unwrap" are
not particularly unique keywords, so apologies if I missed them.

@gopherbot
Copy link
Contributor

This PR (HEAD: 2186af0) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/559355.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@sethvargo sethvargo force-pushed the sethvargo/poc_joinErr branch from 2186af0 to 1da0d1a Compare January 30, 2024 02:30
@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/559355.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 1da0d1a) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/559355.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

This PR is a quick proposal to unwrap `joinError` (and only `joinError`)
when calling `errors.Join`. While I don't think we should unwrap
everything, this is an internal structure that has only internally
semantic meaning. Therefore, I think it makes sense to special-case
`joinError` and unwrapped the contained errors on `joinError`.

Pros:
- More closely mirrors how other error joining packages in the ecosystem
  behave.

- Even though `Unwrap() []error` isn't a public API, this will ensure
  that the method behaves as end-users expect for `errors.Join`,
  returning the list of joined errors.

Cons:
- Introduces type assertion and could result in a performance
  regression, since we're not pre-computing the slice length. This could
  be addressed if there's interest in pursuing this. I also think there's
  a reasonable argument to be made about the benefits realized from this
  change when printing the error.

- Users could be depending on the broken behavior, but that seems
  unlikely since `joinError` is an internal API.

I could not find any corresponding issues, but "errors" and "unwrap" are
not particularly unique keywords, so apologies if I missed them.
@sethvargo sethvargo force-pushed the sethvargo/poc_joinErr branch from 1da0d1a to d43f5a6 Compare January 30, 2024 02:39
@gopherbot
Copy link
Contributor

Message from Seth Vargo:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/559355.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: d43f5a6) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/559355.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

for _, err := range errs {
if err != nil {
n++
allErrs := make([]error, 0, len(errs))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be one of those situations where optimizing for the common use case makes sense. Given "most" error joining is of the form merr = errors.Join(merr, err), we can optimize for the single-entry error:

switch len(errs) {
  case 0:
    return nil
  case 1:
    return errs[0]
  case 2:
    if errs[0] == nil {
      return errs[1]
    } else if errs[1] == nil {
      return errs[0]
    } else {
      return &joinError{
        errs: errs, 
      }
    }
  default:
    // as below
}

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/559355.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Seth Vargo:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/559355.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Damien Neil:

Patch Set 3: Hold+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/559355.
After addressing review feedback, remember to publish your drafts!

@sethvargo sethvargo closed this Feb 16, 2024
@sethvargo sethvargo deleted the sethvargo/poc_joinErr branch February 16, 2024 03:19
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

Successfully merging this pull request may close these issues.

2 participants