x/tools/gopls: parameter movement bug bash #70599
Comments
gopherbot
added
Tools
|
Change https://go.dev/cl/632115 mentions this issue: |
|
(BTW, if anyone reading this is experimenting with the new parameter movement code action, please feel free to comment with any bugs you observe). |
|
Change https://go.dev/cl/632935 mentions this issue: |
|
Change https://go.dev/cl/633096 mentions this issue: |
|
Change https://go.dev/cl/632937 mentions this issue: |
|
Change https://go.dev/cl/633256 mentions this issue: |
|
Change https://go.dev/cl/633255 mentions this issue: |
Add a marker test that reproduces bugs encountered during bug bash of parameter movement refactoring. Subsequent CLs will fix these bugs. For golang/go#70599 Change-Id: Ieb52c1c02f11fa22bc043d7793509914d76d27ca Reviewed-on: https://go-review.googlesource.com/c/tools/+/632115 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Add more precision (and hopefully clarity) to the detection of redundant conversions, by using more granular analysis of references. We now record, for each reference, (1) whether it appears in an assignment context, perhaps subject to implicit conversions, (2) whether this assignment is to an interface value, and (3) whether this assignment may affect type inference. With these conditions, we can more precisely detect scenarios where conversion is necessary, as annotated in the code. Notably, this avoids unnecessary concrete-to-concrete conversions. Also: - Fix a crash in falcon analysis for named literal types. - Handle index expression assignability (previously missing). - Avoid the print builtin as an example of a function that takes 'any', as the type checker records the effective type for print, and the spec is unclear about whether implementations are allowed to specialize for effective types. For golang/go#70599 Updates golang/go#70638 Change-Id: I9730deba54d864928a1dc02a1ab00481a2ce9998 Reviewed-on: https://go-review.googlesource.com/c/tools/+/632935 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
While we can't handle moving variadic parameters, don't offer to move them. For golang/go#70599 Change-Id: I21e78b4e2ae8c061d3858f2f981d593a6b0cf5bc Reviewed-on: https://go-review.googlesource.com/c/tools/+/633096 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Robert Findley <rfindley@google.com>
In the assign statements strategy, if there are no nontrivial result conversions, we can replace a call with its result expression regardless of the AssignStmt.Tok. For golang/go#70599 Change-Id: Ifa2b615c17d58e3bbc708b9ecf98530858915564 Reviewed-on: https://go-review.googlesource.com/c/tools/+/632937 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
In change signature refactoring, the wrapper was very likely to run into a particular unidiomatic behavior of the inliner, where arguments were detected as unsubstitutable because they have free variables shadowed by other parameters. But if those other parameters are going to be substituted, this shadowing is irrelevant. Fix this by keeping track of shadowing from other parameters in a new shadowMap type, and then analyzing these relationships during substitution to detect cases where closed subgraphs of parameters can be substituted simultaneously. Also: fix a formatting bug (golang/go#67335) that was reproduced by one of the new tests: we need to clear positions when using nodes from the callee in the caller's binding decl. For golang/go#70599 Fixes golang/go#67335 Change-Id: I08970a1cea8a82ea108084394569cffbc5975235 Reviewed-on: https://go-review.googlesource.com/c/tools/+/633256 Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Robert Findley <rfindley@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
While using the change signature refactoring, I never want it to literalize changed signatures based on its (very coarse) effects analysis. Disable this inlining analysis for the purpose of change signature refactoring. For golang/go#70599 Change-Id: I979c4f5b6be520b67a3441fa6e0c55afe1fe9196 Reviewed-on: https://go-review.googlesource.com/c/tools/+/633255 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/633711 mentions this issue: |
|
Change https://go.dev/cl/633714 mentions this issue: |
|
Change https://go.dev/cl/633710 mentions this issue: |
|
Change https://go.dev/cl/633713 mentions this issue: |
|
Change https://go.dev/cl/633719 mentions this issue: |
|
Change https://go.dev/cl/633718 mentions this issue: |
|
All done. |
This issue tracks bugs encountered while using the new change signature/parameter movement feature.
Aggregating into a single issue for convenience; I'll keep adding to this list as I encounter bugs.
type Hash [32]byteas an argument to a[32]byteparameter, we see that the inliner adds unnecessary conversions to[32]byte. I avoided unecessary interface->interface conversions, but we also don't need to worry about concrete->concrete.x, y := Foo(1, 2)), the inliner thinks it needs to introduce a binding declaration for x, y.The text was updated successfully, but these errors were encountered: