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

Jump to bottom

x/tools/gopls: Adding a warning when never actually consuming the contents of a string builder #52355

Closed
m1212e opened this issue Apr 14, 2022 · 4 comments
Closed

x/tools/gopls: Adding a warning when never actually consuming the contents of a string builder #52355

m1212e opened this issue Apr 14, 2022 · 4 comments
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
gopls/unplanned

Comments

@m1212e
Copy link

m1212e commented Apr 14, 2022

When using a string builder like this:

func() {
    var data strings.Builder
    data.WriteString("something")
}

no warning will be displayed, since the declared variable is used. On other occasions, such as not passing a reference to a json unmarshal, the linter displays a warning that the user might want something different. I think it would be pretty cool to have such a warning when never calling .String() or returning the string builder.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Apr 14, 2022
@gopherbot gopherbot added this to the Unreleased milestone Apr 14, 2022
@jamalc jamalc modified the milestones: Unreleased, gopls/unplanned Apr 14, 2022
@findleyr
Copy link
Contributor

This is an interesting idea, but I am not sure it is worth pursuing. For reference, there is precedent for checking this type of correct usage, e.g. with the lostcancel analyzer.

How does this suit the vet philosophy? How correct could this analyzer be? How frequent are the bugs that it catches? How precise would it be?

I'm not sure that this type of bug is frequent enough to warrant supporting a new analyzer: unlike calling cancel(), using a string builder's output is usually critical to the program's control flow. For that reason, I suspect people are much less likely to forget to call .String(). Furthermore, I am not sure how accurate this analyzer could be without additional machinery like SSA: builders are often passed around.

Some data showing that this type of bug is common would help the discussion. For example how often does it occur in open-source codebases?

CC @timothy-king

@timothy-king
Copy link
Contributor

How precise would it be?

I think a vet checker would need to give up on most escapes for the strings.Builder, and report if there are no reads to the variable. After those restrictions, I am not too concerned about precision.

How frequent are the bugs that it catches?
For example how often does it occur in open-source codebases?

+1 more real world examples would help assess impact and frequent.

@findleyr findleyr added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 21, 2022
@guodongli-google
Copy link

This example is a good one for the UnusedResult checker in DeepGo. Basically, all the side-effects within WriteString are on the receiver data, and the function return is not used. If data is not used after the function call, then it indicates a bug.

@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2022
@golang golang locked and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
gopls/unplanned
Development

No branches or pull requests
6 participants
@timothy-king @gopherbot @m1212e @jamalc @findleyr @guodongli-google