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: add the unusedwrite analysis #44461

Closed
ainar-g opened this issue Feb 20, 2021 · 9 comments
Closed

x/tools/gopls: add the unusedwrite analysis #44461

ainar-g opened this issue Feb 20, 2021 · 9 comments
Assignees
@adonovan
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.16.0

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Feb 20, 2021

CL 287034 added the new unusedwrite analysis pass. I think it would be nice to have in gopls.

I'm not sure if it should be among the default analysers, but either way, if there are no objections to this, I am ready to send a CL.
@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 Feb 20, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 20, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Feb 20, 2021
@stamblerre
Copy link
Contributor

We'd be happy to accept a CL--if you think it should be off by default, we can start with that first, and then enable it later on?

@stamblerre
Copy link
Contributor

Ah, looks like it uses SSA, so let's keep it off by default to start.

@gopherbot
Copy link

Change https://golang.org/cl/295172 mentions this issue: internal/lsp/source: add the unusedwrite analyzer

@ainar-g
Copy link
Contributor Author

ainar-g commented Feb 23, 2021

Thanks for reviewing!

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.6.7 Feb 23, 2021
@golang golang locked and limited conversation to collaborators Feb 23, 2022
@adonovan
Copy link
Member

adonovan commented Mar 28, 2024 •
edited

SSA is now enabled by default (for nilness), so the marginal cost of enabling unusedwrite is very small. I just ran a very quick experiment and found that 10 out of 10 findings I randomly sampled in kubernetes were genuine opportunities for simplification. So let's do a proper evaluation and perhaps enable it in gopls.

@adonovan adonovan reopened this Mar 28, 2024
@adonovan adonovan modified the milestones: gopls/v0.6.7, gopls/backlog Mar 28, 2024
@adonovan
Copy link
Member

I should run a job over the Module Mirror corpus to get results that we can share, but in the meantime Googlers can inspect a list of findings of this analyzer on Google source code at go/unusedwrite-diagnostics-b1c6ea0fd8b46c. The false positive rate is zero in my manual sampling so far. I think this might be a good feature for gopls.

@adonovan adonovan modified the milestones: gopls/backlog, gopls/v0.16.0 Mar 28, 2024
@adonovan
Copy link
Member

adonovan commented Mar 29, 2024 •
edited

I ran unusedwrite over the Module Mirror corpus of 22867 modules. It reported 2976 unique diagnostics, of which 100 picked at random are shown below. I analyzed a dozen or so by hand, and all were true positives, indicating that the code was unnecessarily complicated and confusing, though potentially still correct; but in three cases, the excessive complexity was clear evidence of a bug, for example updating a field of the receiver struct before returning from a method with a non-pointer receiver.

I think we should enable this analyzer by default in gopls.

The complete list is here: unusedwrite.txt

https://go-mod-viewer.appspot.com/github.com/shuguocloud/go-zero@v1.3.0/rest/httpx/requests_test.go#L312: unused write to field Percent
https://go-mod-viewer.appspot.com/github.com/o1egl/govatar@v0.4.1/govatar_test.go#L182: unused write to field B
https://go-mod-viewer.appspot.com/github.com/yitter/idgenerator-go@v1.3.3/idgen/OverCostActionArg.go#L25: unused write to field TermIndex
https://go-mod-viewer.appspot.com/github.com/ergo-services/ergo@v1.999.224/etf/decode.go#L1171: unused write to field Module
https://go-mod-viewer.appspot.com/github.com/estesp/manifest-tool@v1.0.3/docker/inspect_v2.go#L66: unused write to field MediaType
https://go-mod-viewer.appspot.com/github.com/datachainlab/cross@v0.2.2/x/packets/types.go#L250: unused write to field payload
https://go-mod-viewer.appspot.com/github.com/takumakei/go-urfave-cli/clix@v0.0.0-20210528051825-f43cf228bf5d/filepath_test.go#L25: unused write to field FilePath
https://go-mod-viewer.appspot.com/github.com/abadojack/whatlanggo@v1.0.1/detect_test.go#L82: unused write to field Confidence
https://go-mod-viewer.appspot.com/github.com/nacos-group/nacos-sdk-go/v2@v2.2.5/clients/naming_client/naming_cache/subscribe_callback_test.go#L77: unused write to field LastRefTime
https://go-mod-viewer.appspot.com/github.com/perimeterx/marshmallow@v1.1.5/unmarshal_test.go#L1231: unused write to field ChildField26
https://go-mod-viewer.appspot.com/github.com/selefra/selefra-provider-sdk@v0.0.23/provider/transformer/column_value_extractor/examples/column_value_extractor_wrapper/main.go#L15: unused write to field TableName
https://go-mod-viewer.appspot.com/github.com/DoNewsCode/core@v0.12.3/codec/yaml/yaml_test.go#L88: unused write to field A
https://go-mod-viewer.appspot.com/github.com/adharshmk96/stk@v1.2.3/pkg/sqlMigrator/generator_test.go#L97: unused write to field Name
https://go-mod-viewer.appspot.com/github.com/laher/argo@v0.0.0-20140722103944-11d91c83cc0f/ar/reader_test.go#L293: unused write to field closed
https://go-mod-viewer.appspot.com/github.com/argoproj/argo-cd/v2@v2.10.5/util/db/repository_legacy.go#L285: unused write to field GithubAppInstallationId
https://go-mod-viewer.appspot.com/github.com/ti-mo/conntrack@v0.5.0/attribute_types_test.go#L297: unused write to field Type
https://go-mod-viewer.appspot.com/github.com/n0madic/twitter-scraper@v0.0.0-20231104223941-296710769dd8/tweets_test.go#L279: unused write to field Name
https://go-mod-viewer.appspot.com/github.com/dolthub/go-mysql-server@v0.18.0/sql/expression/function/string_test.go#L146: unused write to field Name
https://go-mod-viewer.appspot.com/gopkg.in/goose.v2@v2.0.1/testservices/novaservice/service_http_test.go#L1314: unused write to field Addresses
https://go-mod-viewer.appspot.com/github.com/drone/go-convert@v0.0.0-20240307072510-6bd371c65e61/convert/circle/convert.go#L132: unused write to field Inputs
https://go-mod-viewer.appspot.com/github.com/smartwalle/ncrypto@v1.0.4/pkcs12/pkcs12_test.go#L83: unused write to field Certificates
https://go-mod-viewer.appspot.com/github.com/unidoc/unidoc@v2.2.0+incompatible/pdf/model/fonts/helvetica_bold_oblique.go#L30: unused write to field encoder
https://go-mod-viewer.appspot.com/github.com/projecteru2/core@v0.0.0-20240321043226-06bcc1c23f58/cluster/calcium/wal_test.go#L134: unused write to field Engine
https://go-mod-viewer.appspot.com/github.com/ethereum-optimism/optimism@v1.7.2/op-node/rollup/derive/batches_test.go#L191: unused write to field Hash
https://go-mod-viewer.appspot.com/github.com/stafiprotocol/go-substrate-rpc-client@v1.4.7/types/multi_address.go#L111: unused write to field IsAddress32
https://go-mod-viewer.appspot.com/github.com/whatap/golib@v0.0.22/util/hmap/IntSet.go#L205: unused write to field table
https://go-mod-viewer.appspot.com/github.com/siglens/siglens@v0.0.0-20240328180423-f7ce9ae441ed/pkg/integrations/loki/loki.go#L553: unused write to field ExecTime
https://go-mod-viewer.appspot.com/github.com/minio/minio@v0.0.0-20240328213742-3f72439b8a27/cmd/erasure-multipart.go#L871: unused write to field MaxSize
https://go-mod-viewer.appspot.com/github.com/hashicorp/cap@v0.6.0/oidc/options_test.go#L34: unused write to field withNowFunc
https://go-mod-viewer.appspot.com/github.com/gocrane/crane@v0.11.0/pkg/utils/expression_prom_deafult_test.go#L163: unused write to field description
https://go-mod-viewer.appspot.com/github.com/pion/rtp@v1.8.5/packet_test.go#L901: unused write to field Payload
https://go-mod-viewer.appspot.com/github.com/ti-mo/conntrack@v0.5.0/attribute_types_test.go#L269: unused write to field Type
https://go-mod-viewer.appspot.com/github.com/ardanlabs/conf/v2@v2.2.0/conf_test.go#L495: unused write to field name
https://go-mod-viewer.appspot.com/github.com/perimeterx/marshmallow@v1.1.5/unmarshal_from_json_map_test.go#L907: unused write to field ChildField6
https://go-mod-viewer.appspot.com/github.com/nntaoli-project/goex@v1.3.3/okex/OKExSwap.go#L457: unused write to field InstrumentId
https://go-mod-viewer.appspot.com/github.com/ti-mo/conntrack@v0.5.0/attribute_types_test.go#L234: unused write to field Nested
https://go-mod-viewer.appspot.com/github.com/google/battery-historian@v0.0.0-20170519220231-d2356ba4fd5f/checkindelta/checkin_normalize.go#L49: unused write to field AggregationType
https://go-mod-viewer.appspot.com/github.com/hellobchain/newcryptosm@v0.0.0-20221019060107-edb949a317e9/sm9/sm9_test.go#L523: unused write to field t
https://go-mod-viewer.appspot.com/github.com/akamai/AkamaiOPEN-edgegrid-golang@v1.2.2/configgtm-v1_5/errors.go#L39: unused write to field err
https://go-mod-viewer.appspot.com/github.com/nntaoli-project/goex@v1.3.3/binance/SpotWs.go#L227: unused write to field Amount
https://go-mod-viewer.appspot.com/github.com/kyma-project/kyma-environment-broker@v0.0.1/internal/provisioner/client_test.go#L417: unused write to field RuntimeID
https://go-mod-viewer.appspot.com/github.com/pingcap/tiflow@v0.0.0-20240329071719-db60e9e8de74/cdc/scheduler/internal/v3/keyspan/splitter_region_count_test.go#L194: unused write to field EnableTableAcrossNodes
https://go-mod-viewer.appspot.com/github.com/aacfactory/fns-contrib/databases/sql@v1.2.84/rows.go#L660: unused write to field size
https://go-mod-viewer.appspot.com/github.com/huaweicloud/golangsdk@v0.0.0-20210831081626-d823fe11ceba/openstack/dns/v2/recordsets/testing/requests_test.go#L140: unused write to field Version
https://go-mod-viewer.appspot.com/cloud.google.com/go/storage@v1.40.0/client_test.go#L336: unused write to field Bucket
https://go-mod-viewer.appspot.com/gopkg.in/httprequest.v1@v1.2.1/bench_test.go#L425: unused write to field Field5
https://go-mod-viewer.appspot.com/github.com/alibaba/sentinel-golang@v1.0.4/core/stat/base/leap_array_test.go#L39: unused write to field BucketStart
https://go-mod-viewer.appspot.com/github.com/cozy/cozy-stack@v0.0.0-20240327093429-939e4a21320e/model/move/cursor.go#L127: unused write to field Number
https://go-mod-viewer.appspot.com/github.com/gonum/lapack@v0.0.0-20181123203213-e4cdc5a0bff9/testlapack/dgetrs.go#L102: unused write to field Cols
https://go-mod-viewer.appspot.com/github.com/yitter/idgenerator-go@v1.3.3/idgen/OverCostActionArg.go#L24: unused write to field GenCountInOneTerm
https://go-mod-viewer.appspot.com/github.com/quic-go/quic-go@v0.42.0/internal/wire/ack_frame.go#L259: unused write to field Smallest
https://go-mod-viewer.appspot.com/github.com/laher/uggo@v0.0.0-20140418102112-0ad25fe11c5b/flagsetaliasedvars.go#L91: unused write to field out
https://go-mod-viewer.appspot.com/github.com/mattermost/mattermost-plugin-api@v0.1.4/i18n/i18n_test.go#L32: unused write to field b
https://go-mod-viewer.appspot.com/github.com/nacos-group/nacos-sdk-go@v1.1.4/clients/naming_client/subscribe_callback_test.go#L78: unused write to field LastRefTime
https://go-mod-viewer.appspot.com/k8s.io/apiserver@v0.29.3/pkg/storage/value/encrypt/envelope/metrics/metrics_test.go#L345: unused write to field apiServerIDHash
https://go-mod-viewer.appspot.com/github.com/zondax/ledger-go@v0.14.3/apduWrapper_test.go#L117: unused write to field tag
https://go-mod-viewer.appspot.com/github.com/hduhelp/go-zero@v1.4.3/gateway/server.go#L172: unused write to field Name
https://go-mod-viewer.appspot.com/github.com/juju/juju@v0.0.0-20240327075706-a90865de2538/provider/maas/config.go#L73: unused write to field Config
https://go-mod-viewer.appspot.com/git.ug-wk.cc/server/log@v0.0.0-20230524075443-2e41d2d64cca/config.go#L146: unused write to field ErrorOutputPaths
https://go-mod-viewer.appspot.com/github.com/projectdiscovery/nuclei/v2@v2.9.15/pkg/protocols/dns/dns.go#L142: unused write to field Resolvers
https://go-mod-viewer.appspot.com/github.com/openfga/go-sdk@v0.3.5/client/client_test.go#L1050: unused write to field ResponseStatus
https://go-mod-viewer.appspot.com/github.com/janelia-flyem/dvid@v1.0.0/datatype/multichan16/multichan16.go#L421: unused write to field channelNum
https://go-mod-viewer.appspot.com/github.com/condensat/bank-core@v0.1.0/security/internal/edwards25519/edwards25519.go#L752: unused write to array index 31:int - t59
https://go-mod-viewer.appspot.com/github.com/PagerDuty/go-pagerduty@v1.8.0/service_test.go#L74: unused write to field More
https://go-mod-viewer.appspot.com/github.com/fluxcd/go-git-providers@v0.19.3/gitprovider/testutils/retry.go#L86: unused write to field backoff
https://go-mod-viewer.appspot.com/github.com/perimeterx/marshmallow@v1.1.5/unmarshal_test.go#L1215: unused write to field ChildField25
https://go-mod-viewer.appspot.com/gitlab.com/go-extension/tls@v0.0.0-20240304171319-e6745021905e/example_test.go#L216: unused write to field VerifyConnection
https://go-mod-viewer.appspot.com/go.temporal.io/server@v1.23.0/common/rpc/test/rpc_localstore_tls_test.go#L409: unused write to field Membership
https://go-mod-viewer.appspot.com/github.com/perimeterx/marshmallow@v1.1.5/unmarshal_test.go#L1151: unused write to field ChildField21
https://go-mod-viewer.appspot.com/github.com/weaviate/weaviate@v1.24.6/modules/text2vec-openai/clients/openai_test.go#L65: unused write to field ModelVersion
https://go-mod-viewer.appspot.com/github.com/nacos-group/nacos-sdk-go/v2@v2.2.5/clients/naming_client/naming_cache/subscribe_callback_test.go#L75: unused write to field CacheMillis
https://go-mod-viewer.appspot.com/yunion.io/x/jsonutils@v1.0.0/unmarshal.go#L81: unused write to field data
https://go-mod-viewer.appspot.com/github.com/ystia/yorc/v4@v4.3.0/storage/store_mgr.go#L123: unused write to field Types
https://go-mod-viewer.appspot.com/github.com/gravity-devs/liquidity@v1.5.3/x/liquidity/keeper/liquidity_pool_test.go#L263: unused write to field WithdrawFeeRate
https://go-mod-viewer.appspot.com/git.ug-wk.cc/server/log@v0.0.0-20230524075443-2e41d2d64cca/config.go#L144: unused write to field Encoding
https://go-mod-viewer.appspot.com/github.com/aacfactory/fns-contrib/databases/sql@v1.2.84/rows.go#L266: unused write to field Valid
https://go-mod-viewer.appspot.com/github.com/prysmaticlabs/prysm@v1.4.4/slasher/detection/detect_test.go#L539: unused write to field ctx
https://go-mod-viewer.appspot.com/github.com/prysmaticlabs/prysm@v1.4.4/slasher/detection/detect_test.go#L540: unused write to field cfg
https://go-mod-viewer.appspot.com/github.com/hyperledger/aries-framework-go@v0.3.2/pkg/didcomm/protocol/didexchange/states_test.go#L817: unused write to field outboundDispatcher
https://go-mod-viewer.appspot.com/github.com/mit-dci/lit@v0.0.0-20221102210550-8c3d3b49f2ce/wire/message.go#L279: unused write to field command
https://go-mod-viewer.appspot.com/github.com/SlyMarbo/spdy@v0.0.0-20160217225201-b5edf18e002b/spdy3/io.go#L107: unused write to field StreamID
https://go-mod-viewer.appspot.com/github.com/verrazzano/verrazzano@v1.7.0/application-operator/controllers/loggingtrait/loggingtrait_controller.go#L168: unused write to field ImagePullPolicy
https://go-mod-viewer.appspot.com/gitlab.com/SkynetLabs/skyd@v1.6.9/skymodules/renter/skylinkdatasource_test.go#L34: unused write to field staticBaseSectorDownloadStats
https://go-mod-viewer.appspot.com/github.com/projectcontour/contour@v1.28.2/cmd/contour/certgen_test.go#L190: unused write to field OutputKube
https://go-mod-viewer.appspot.com/github.com/coreos/goproxy@v0.0.0-20190513173959-f8dc2d7ba04e/websocket.go#L50: unused write to field Path
https://go-mod-viewer.appspot.com/sigs.k8s.io/cluster-api@v1.6.3/internal/controllers/machinehealthcheck/machinehealthcheck_targets_test.go#L170: unused write to field patchHelper
https://go-mod-viewer.appspot.com/github.com/gambol99/goproxy@v0.0.0-20170612135454-e713e5909438/websocket.go#L29: unused write to field Path
https://go-mod-viewer.appspot.com/github.com/KiraCore/sekai@v0.3.43/x/layer2/proposal_handler.go#L173: unused write to field UpdateTimeMax
https://go-mod-viewer.appspot.com/github.com/ChainSafe/chainbridge-core@v1.4.2/chains/evm/listener/deposit-handler_test.go#L196: unused write to field SenderAddress
https://go-mod-viewer.appspot.com/github.com/confluentinc/confluent-kafka-go@v1.9.2/kafka/producer_performance_test.go#L208: unused write to field Value
https://go-mod-viewer.appspot.com/yunion.io/x/cloudmux@v0.3.10-0-alpha.1/pkg/multicloud/huawei/obs/client.go#L86: unused write to field httpClient
https://go-mod-viewer.appspot.com/github.com/vertgenlab/gonomics@v1.0.0/genomeGraph/search.go#L124: unused write to field targetEnd
https://go-mod-viewer.appspot.com/github.com/fluxcd/go-git-providers@v0.19.3/gitprovider/testutils/retry.go#L81: unused write to field interval
https://go-mod-viewer.appspot.com/github.com/vishvananda/netlink@v1.1.0/nl/seg6_linux.go#L107: unused write to field routingType
https://go-mod-viewer.appspot.com/github.com/IBM-Cloud/bluemix-go@v0.0.0-20240314082800-4e02a69b84b2/api/iampap/iampapv1/models.go#L191: unused write to field Value
https://go-mod-viewer.appspot.com/github.com/civo/civogo@v0.3.65/firewall_test.go#L134: unused write to field InstanceCount
https://go-mod-viewer.appspot.com/github.com/vertgenlab/gonomics@v1.0.0/genomeGraph/search.go#L131: unused write to field rightScore
https://go-mod-viewer.appspot.com/github.com/btccom/go-micro/v2@v2.9.3/store/cockroach/cockroach.go#L251: unused write to field Expiry
https://go-mod-viewer.appspot.com/honnef.co/go/tools@v0.4.7/pattern/parser.go#L304: unused write to field idx
https://go-mod-viewer.appspot.com/github.com/kyma-project/control-plane/components/kyma-environment-broker@v0.0.0-20230911130701-f7d1ac254205/internal/provisioner/client_test.go#L399: unused write to field State

@adonovan adonovan self-assigned this Mar 29, 2024
@timothy-king
Copy link
Contributor

I took a quick look. Most of the reports in non-test code look really good.

https://go-mod-viewer.appspot.com/honnef.co/go/tools@v0.4.7/pattern/parser.go#L304

FYI there are some false positives due to struct literals:

  1. https://go-mod-viewer.appspot.com/github.com/takumakei/go-urfave-cli/clix@v0.0.0-20210528051825-f43cf228bf5d/filepath_test.go#L25
  2. https://go-mod-viewer.appspot.com/github.com/abadojack/whatlanggo@v1.0.1/detect_test.go#L82
  3. https://go-mod-viewer.appspot.com/github.com/nacos-group/nacos-sdk-go/v2@v2.2.5/clients/naming_client/naming_cache/subscribe_callback_test.go#L77
  4. https://go-mod-viewer.appspot.com/github.com/selefra/selefra-provider-sdk@v0.0.23/provider/transformer/column_value_extractor/examples/column_value_extractor_wrapper/main.go#L15
  5. https://go-mod-viewer.appspot.com/github.com/DoNewsCode/core@v0.12.3/codec/yaml/yaml_test.go#L88
  6. https://go-mod-viewer.appspot.com/github.com/argoproj/argo-cd/v2@v2.10.5/util/db/repository_legacy.go#L285
  7. https://go-mod-viewer.appspot.com/github.com/ti-mo/conntrack@v0.5.0/attribute_types_test.go#L297
  8. https://go-mod-viewer.appspot.com/github.com/o1egl/govatar@v0.4.1/govatar_test.go#L182
  9. https://go-mod-viewer.appspot.com/github.com/n0madic/twitter-scraper@v0.0.0-20231104223941-296710769dd8/tweets_test.go#L279
  10. https://go-mod-viewer.appspot.com/github.com/dolthub/go-mysql-server@v0.18.0/sql/expression/function/string_test.go#L146
  11. https://go-mod-viewer.appspot.com/gopkg.in/goose.v2@v2.0.1/testservices/novaservice/service_http_test.go#L1314

The common pattern is x := struct{f, g int}{f: 0, g: 1}; println(x.f). The implicit x.g = 1 write is being reported as it is never used. The solution is likely just better representation of struct literal initialization at the SSA level and not reporting these. This could be something like what staticcheck does in its ir, which is to have a struct literal Instruction, or better 'debug info' on the write instructions.

Just something to keep in mind. Unclear if it is urgent.

@adonovan
Copy link
Member

adonovan commented Apr 2, 2024

FYI there are some false positives due to struct literals:

They're not bugs, but they do indicate that a field of a struct is assigned but never used, which is a sign that the code is building a bigger data structure than it actually needs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.16.0
Development

No branches or pull requests
5 participants
@timothy-king @ainar-g @adonovan @stamblerre @gopherbot