refactor: remove errors directive comparison #10502
Conversation
kien6034
requested review from
a team,
protolambda,
trianglesphere and
ajsutton
as code owners
WalkthroughWalkthroughThe recent updates across various files primarily focus on enhancing error handling by replacing direct error comparisons with the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (34)
Additional Context UsedLearnings (1)
Additional comments not posted (53)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Out of diff range and nitpick comments (2)
op-batcher/batcher/channel_builder.go (1)
332-332: Clarify error handling inoutputFrame.Consider adding a comment to explain why
io.EOFis treated differently here, as it might not be immediately clear to other developers.op-node/rollup/derive/batch_queue.go (1)
171-171: Clarify error handling inderiveNextBatch.Consider adding a comment to explain why
io.EOFis treated differently here, as it might not be immediately clear to other developers.
| @@ -304,7 +304,7 @@ func (eq *EngineQueue) Step(ctx context.Context) error { | |||
| // Trying unsafe payload should be done before safe attributes | |||
| // It allows the unsafe head can move forward while the long-range consolidation is in progress. | |||
| if eq.unsafePayloads.Len() > 0 { | |||
| if err := eq.tryNextUnsafePayload(ctx); err != io.EOF { | |||
| if err := eq.tryNextUnsafePayload(ctx); !errors.Is(err, io.EOF) { | |||
There was a problem hiding this comment.
Ensure proper error handling for non-EOF errors in tryNextUnsafePayload.
- if err := eq.tryNextUnsafePayload(ctx); !errors.Is(err, io.EOF) {
+ if err := eq.tryNextUnsafePayload(ctx); err != nil && !errors.Is(err, io.EOF) {
return err
}The current implementation only handles the case where the error is not io.EOF, potentially missing other critical errors that should also halt the execution. The suggested change ensures that any error, except io.EOF, is properly handled.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := eq.tryNextUnsafePayload(ctx); !errors.Is(err, io.EOF) { | |
| if err := eq.tryNextUnsafePayload(ctx); err != nil && !errors.Is(err, io.EOF) { | |
| return err | |
| } |
| @@ -331,7 +331,7 @@ func (eq *EngineQueue) Step(ctx context.Context) error { | |||
| if err := eq.tryFinalizePastL2Blocks(ctx); err != nil { | |||
| return err | |||
| } | |||
| if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); err == io.EOF { | |||
| if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); errors.Is(err, io.EOF) { | |||
There was a problem hiding this comment.
Handle potential io.EOF error correctly in Step method.
- if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); errors.Is(err, io.EOF) {
+ if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); err != nil {
+ if errors.Is(err, io.EOF) {
return io.EOF
+ }
+ return err
} else if err != nil {
return err
} else {The original code does not handle other errors that might occur when fetching the next attributes. This change ensures that all errors are handled appropriately, not just io.EOF.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); errors.Is(err, io.EOF) { | |
| if next, err := eq.prev.NextAttributes(ctx, eq.ec.PendingSafeL2Head()); err != nil { | |
| if errors.Is(err, io.EOF) { | |
| return io.EOF | |
| } | |
| return err | |
| } else if err != nil { | |
| return err | |
| } else { |
| @@ -260,7 +261,7 @@ func TestPlasmaDataSource(t *testing.T) { | |||
| break | |||
| } | |||
|
|
|||
| for data, err := src.Next(ctx); err != io.EOF; data, err = src.Next(ctx) { | |||
| for data, err := src.Next(ctx); !errors.Is(err, io.EOF); data, err = src.Next(ctx) { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that there are multiple instances across the codebase where error handling does not use the errors.Is() function, contrary to the best practices mentioned in the review comment. These instances are spread across various files and modules, indicating that the update to use errors.Is() for error comparison is not consistently applied throughout the project.
Locations needing attention:
proxyd/consensus_tracker.goop-ufm/pkg/provider/roundtrip.goop-ufm/pkg/provider/heartbeat.goop-node/rollup/derive/engine_queue.goop-node/rollup/derive/batch_queue_test.goop-node/rollup/driver/sequencer_test.goop-node/cmd/batch_decoder/reassemble/reassemble.goop-e2e/e2eutils/*op-conductor/conductor/service.goop-challenger/game/scheduler/coordinator.goop-chain-ops/cmd/op-simulate/main.goop-batcher/batcher/*
These files contain direct comparisons of errors without using errors.Is(), which should be refactored to align with the updated error handling strategy.
Analysis chain
Use of errors.Is() for error comparison is a best practice in Go, especially for handling wrapped errors. Good implementation.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all instances of error handling in the project have been updated to use `errors.Is()`.
# Test: Search for direct error comparisons. Expect: No occurrences of direct error comparisons.
rg --type go '!errors.Is' --glob '*.go'
Length of output: 2982
| @@ -300,7 +300,7 @@ func (c *ChannelBuilder) outputReadyFrames() error { | |||
| // When creating a frame from the ready compression data, the frame overhead | |||
| // will be added to the total output size, so we can add it in the condition. | |||
| for c.co.ReadyBytes()+derive.FrameV0OverHeadSize >= int(c.cfg.MaxFrameSize) { | |||
| if err := c.outputFrame(); err == io.EOF { | |||
| if err := c.outputFrame(); errors.Is(err, io.EOF) { | |||
There was a problem hiding this comment.
Ensure proper error handling in outputReadyFrames.
- if err := c.outputFrame(); errors.Is(err, io.EOF) {
+ if err := c.outputFrame(); err != nil && !errors.Is(err, io.EOF) {The original code does not handle errors that are not io.EOF. This change ensures that all errors are properly handled.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := c.outputFrame(); errors.Is(err, io.EOF) { | |
| if err := c.outputFrame(); err != nil && !errors.Is(err, io.EOF) { |
| @@ -315,7 +315,7 @@ func (c *ChannelBuilder) closeAndOutputAllFrames() error { | |||
| } | |||
|
|
|||
| for { | |||
| if err := c.outputFrame(); err == io.EOF { | |||
| if err := c.outputFrame(); errors.Is(err, io.EOF) { | |||
There was a problem hiding this comment.
Ensure proper error handling in closeAndOutputAllFrames.
- if err := c.outputFrame(); errors.Is(err, io.EOF) {
+ if err := c.outputFrame(); err != nil && !errors.Is(err, io.EOF) {Similar to the previous comment, this ensures that all errors are properly handled, not just io.EOF.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := c.outputFrame(); errors.Is(err, io.EOF) { | |
| if err := c.outputFrame(); err != nil && !errors.Is(err, io.EOF) { |
| @@ -148,7 +148,7 @@ func (bq *BatchQueue) NextBatch(ctx context.Context, parent eth.L2BlockRef) (*Si | |||
|
|
|||
| // Load more data into the batch queue | |||
| outOfData := false | |||
| if batch, err := bq.prev.NextBatch(ctx); err == io.EOF { | |||
| if batch, err := bq.prev.NextBatch(ctx); errors.Is(err, io.EOF) { | |||
There was a problem hiding this comment.
Ensure proper error handling in NextBatch.
- if batch, err := bq.prev.NextBatch(ctx); errors.Is(err, io.EOF) {
+ if batch, err := bq.prev.NextBatch(ctx); err != nil && !errors.Is(err, io.EOF) {This change ensures that all errors are properly handled, not just io.EOF.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if batch, err := bq.prev.NextBatch(ctx); errors.Is(err, io.EOF) { | |
| if batch, err := bq.prev.NextBatch(ctx); err != nil && !errors.Is(err, io.EOF) { |
| return nil, false, io.EOF | ||
| } else if err == io.EOF { | ||
| } else if errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
Ensure proper error handling in deriveNextBatch.
- } else if errors.Is(err, io.EOF) {
+ } else if err != nil && !errors.Is(err, io.EOF) {This ensures that all errors are properly handled, not just io.EOF.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| } else if errors.Is(err, io.EOF) { | |
| } else if err != nil && !errors.Is(err, io.EOF) { |
| @@ -324,7 +324,7 @@ func TestSequencerChaosMonkey(t *testing.T) { | |||
| // reset errors | |||
| originErr = nil | |||
| attrsErr = nil | |||
| if engControl.err != mockResetErr { // the mockResetErr requires the sequencer to Reset() to recover. | |||
| if !errors.Is(engControl.err, mockResetErr) { // the mockResetErr requires the sequencer to Reset() to recover. | |||
There was a problem hiding this comment.
Ensure consistent error handling in TestSequencerChaosMonkey.
- if !errors.Is(engControl.err, mockResetErr) {
+ if engControl.err != nil && !errors.Is(engControl.err, mockResetErr) {This change ensures that all errors are properly handled, not just mockResetErr.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if !errors.Is(engControl.err, mockResetErr) { // the mockResetErr requires the sequencer to Reset() to recover. | |
| if engControl.err != nil && !errors.Is(engControl.err, mockResetErr) { // the mockResetErr requires the sequencer to Reset() to recover. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10502 +/- ##
========================================
Coverage 42.31% 42.31%
========================================
Files 73 73
Lines 4838 4838
Branches 766 766
========================================
Hits 2047 2047
Misses 2682 2682
Partials 109 109
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
io.EOF is one of the few errors in Go where == is fine in my opinion, as it is a documented part of the API to return an EOF error, even in many standard-lib functions. It's not wrapped, since an internal function might return EOF, which means something different than the outer function returning EOF. While not pretty as-is, I don't think changing literally errors.Is improves the code, due to this difference in meaning. We should be documenting returned errors instead of slamming a errors.Is on everything blindly.
|
Ditto directly comparing errors to |
Description
This PR uses
errors.Is()function instead of directive errors comparisionTests
Please describe any tests you've added. If you've added no tests, or left important behavior untested, please explain why not.
Additional context
Add any other context about the problem you're solving.
Metadata