Skip to content

Fixed the same missing Err() issue in context.go#766

Open
IDisposable wants to merge 1 commit into
rs:masterfrom
IDisposable:fix/context-error-marshal
Open

Fixed the same missing Err() issue in context.go#766
IDisposable wants to merge 1 commit into
rs:masterfrom
IDisposable:fix/context-error-marshal

Conversation

@IDisposable
Copy link
Copy Markdown
Contributor

Since #763 was merged instead of #758, only event.go was fixed. Need the same fix for context.go

Copilot AI review requested due to automatic review settings April 21, 2026 21:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a regression where Context.Err() could drop the main error field when ErrorStackMarshaler(err) returns nil, aligning context.go behavior with the earlier fix applied to event.go (PR #763).

Changes:

  • Update Context.Err() to not return early when ErrorStackMarshaler returns nil, so AnErr still records the error field.
  • Update TestContext_ErrWithNilStackMarshaler to assert the error field is still emitted when stack marshaling yields nil.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
context.go Removes the early return on nil stack-marshaler output so the main error field is still logged.
context_test.go Adjusts expectations/documentation to verify error logging is preserved even without a stack trace.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread context.go
case nil:
return c // do nothing with nil errors
// ErrorStackMarshaler returned nil — the error has no stack trace to
// attach. Fall through and still log the error via AnErr below.
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

The inline comment says “Fall through” inside a Go type-switch, but there is no fallthrough statement here. This wording can be misleading since “fallthrough” has a specific meaning in Go switches; consider rephrasing to something like “do nothing here; continue after the switch and still log the error via AnErr below.”

Suggested change
// attach. Fall through and still log the error via AnErr below.
// attach. Do nothing here; continue after the switch and still log
// the error via AnErr below.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@IDisposable IDisposable Apr 21, 2026

Choose a reason for hiding this comment

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

I copied the language from the other PR's change in event.go. Consistency rules (or I could change both in this PR, what say you @rs)?

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