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

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
Labels
FeatureRequest gopls Tools WaitingForInfo

Comments

@m1212e
Copy link

@m1212e 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 gopls labels Apr 14, 2022
@gopherbot gopherbot added this to the Unreleased milestone Apr 14, 2022
@jamalc jamalc removed this from the Unreleased milestone Apr 14, 2022
@jamalc jamalc added this to the gopls/unplanned milestone Apr 14, 2022
@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 14, 2022

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

@timothy-king timothy-king commented Apr 14, 2022

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 label Apr 21, 2022
@guodongli-google
Copy link

@guodongli-google guodongli-google commented Apr 25, 2022

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

@gopherbot gopherbot commented May 21, 2022

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls Tools WaitingForInfo
Projects
None yet
Development

No branches or pull requests

6 participants