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/go/analysis/passes/lostcancel: add new context functions and make it configurable #70185

Open
sirzooro opened this issue Nov 4, 2024 · 8 comments
Assignees
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@sirzooro
Copy link

sirzooro commented Nov 4, 2024

Go version

go version go1.23.2 linux/amd64

Output of go env in your module/workspace:

-

What did you do?

run nogo

What did you see happen?

Analyzer lostcancel does not check if cancel returned by new context functions (WithCancelCause, WithDeadlineCause, WithTimeoutCause) is called. It is also not possible to add new import paths to check to the analyzer,

What did you expect to see?

Analyzer lostcancel checks for these new functions too. It also has configuration flag to specify additional import paths for context-like packages. You may also add another flag to specify more functions to check but this is more tricky in general, as cancel function may not be returned as a second value as for context functions.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 4, 2024
@gopherbot gopherbot added this to the Unreleased milestone Nov 4, 2024
@gabyhelp
Copy link

gabyhelp commented Nov 4, 2024

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor
Copy link
Member

CC @golang/tools-team

@adonovan adonovan self-assigned this Nov 4, 2024
@adonovan
Copy link
Member

adonovan commented Nov 4, 2024

Thanks for the report. Adding the three new context functions is trivial; I'll do that presently.

Making it configurable is more work, and in our experience no-one ever configures analyzers, so they need smart defaults or heuristics to infer the relevant patterns. A good heuristic here might be "returns a context.CancelFunc". A generalization of this might be "returns any func value named cancel, release, or stop" or "returns a func of a named type whose type name includes those words", but both of those heuristics would need an empirical evaluation.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/625115 mentions this issue: go/analysis/passes/lostcancel: add WithCancelCause et al

@sirzooro
Copy link
Author

sirzooro commented Nov 4, 2024

Thanks for quick fix for first issue.

In my case I need to check my multicontext type, which has the same With* functions as context, so ability to add new import path would be enough for me.

We already use configuration option provided by printf analyzer to check if our logging printf-like functions are used correctly. I would like to do something similar with lostcancel.

gopherbot pushed a commit to golang/tools that referenced this issue Nov 4, 2024
The *Cause variants behave in a similar manner.

Updates golang/go#70185

Change-Id: Ia7eea7a5be8878930505b63fa8222060fef47079
Reviewed-on: https://go-review.googlesource.com/c/tools/+/625115
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@adonovan
Copy link
Member

adonovan commented Nov 4, 2024

In my case I need to check my multicontext type, which has the same With* functions as context, so ability to add new import path would be enough for me.

We already use configuration option provided by printf analyzer to check if our logging printf-like functions are used correctly. I would like to do something similar with lostcancel.

Interestingly, the printf analyzer is able to infer whether a function is printf-like based on whether it delegates (perhaps indirectly) to fmt.Sprintf; this is an example of how it is simpler and more robust to get the tool to recognize the pattern than to use an allowlist. (I wonder why your logging printf-like functions need the configuration option. Is it because dynamic calls are involved, so the analyzer can't observe the delegation? See #58340.)

@sirzooro
Copy link
Author

sirzooro commented Nov 4, 2024

Interestingly, the printf analyzer is able to infer whether a function is printf-like based on whether it delegates (perhaps indirectly) to fmt.Sprintf; this is an example of how it is simpler and more robust to get the tool to recognize the pattern than to use an allowlist. (I wonder why your logging printf-like functions need the configuration option. Is it because dynamic calls are involved, so the analyzer can't observe the delegation? See #58340.)

Yes, we have logger interface like in linked issue.

@timothy-king
Copy link
Contributor

Thoughts:

  1. My intuition is that we should prefer to either infer the delegation or require some kind of source annotation within the With* functions. I think this matches what @adonovan said.
  2. allowlists end up kinda hacky. They can quickly fix specific problem for motivated users (like those that post issues to the go tracker). But they scale kinda poorly as all users need to know to reach for them and remember to set the desired knobs, and they constrain the API going forward.
  3. The different configuration solutions are better matches for a proposal IMO. I kinda think this issue should be closed (the new functions were added) and a proposal should be filed for configuration.
  4. How frequently do we think users need configuration for lostcancel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants