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.

Sign up for GitHub

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

Jump to bottom

contrib/olivere/elastic: fix ineffectual assignment #1010

Merged
merged 3 commits into from
Sep 21, 2021
Merged

contrib/olivere/elastic: fix ineffectual assignment #1010

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
429af0d
Fix ineffectual assignment
mrkm4ntr Sep 16, 2021
2c48d86
Merge branch 'v1' into fix-ineffectual-assignment
knusbaum Sep 17, 2021
7cd2dc0
Merge branch 'v1' into fix-ineffectual-assignment
knusbaum Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contrib/olivere/elastic/elastictrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ func peek(rc io.ReadCloser, encoding string, max, n int) (string, io.ReadCloser,
}
if encoding == "gzip" {
// unpack the snippet
gzr, err := gzip.NewReader(bytes.NewReader(snip))
if err != nil {
gzr, err2 := gzip.NewReader(bytes.NewReader(snip))
if err2 != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@mrkm4ntr maybe I haven't had enough coffee yet, but I don't understand why the previous assignment would have been "ineffectual"? See https://play.golang.org/p/hUfSbzBrIoG

Copy link
Contributor Author

@mrkm4ntr mrkm4ntr Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actual ineffectual assignment is here.

dd-trace-go/contrib/olivere/elastic/elastictrace.go

Line 143 in f86a82b

snip, err = ioutil.ReadAll(gzr)

The scope of err is in this block. So I changed the name of err in this scope to err2.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Subtle. LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this changes the logic. Whether the variable is named err or err2, it is block-scoped and never used outside the block. Likewise, we're not using the outer err inside the block, so shadowing is not an issue.

If there is a bug here, please add a test that exposes the bug which is fixed by this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, the problematic part is here.

dd-trace-go/contrib/olivere/elastic/elastictrace.go

Lines 143 to 145 in f86a82b

snip, err = ioutil.ReadAll(gzr)
}
return string(snip), rc2, err

if ioutil.ReadAll(gzr) returns error, that error is assigned to scoped err not err that function returns. So this function cannot return error of ioutil.ReadAll(gzr).

The test is still required...? I think it is preferred to introduce linters to detect these kind of bugs automatically.
https://github.com/gordonklaus/ineffassign

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrkm4ntr Thank you for the clarification. I did not look closely enough my first time through.

A test case is always welcome, but I think we can merge this as-is.

// snip wasn't gzip; return it as is
return string(snip), rc2, nil
}
Expand Down