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.

Sign up for GitHub

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

Shorten variable types #3535

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Shorten variable types #3535

merged 2 commits into from
Nov 9, 2023

Conversation

stefanhaller
Copy link
Contributor

The variables view in VS Code is a lot easier to read if long type names are
shortened in much the same way as we shorten them for functions in the call
stack view.

We only shorten them in the value strings; the Type field of dap.Variable is
kept as is. Since this only appears in a tooltip, it isn't a problem to have the
full type visible there.

Fixes #3516.

stefanhaller
stefanhaller commented Oct 20, 2023
View reviewed changes
Copy link
Contributor Author

@stefanhaller stefanhaller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a lot of time yet to test this thoroughly, but a cursory test makes this look very promising.

Putting it out there so that others can play with it. I'll be on vacation for two weeks now.

service/api/prettyprint.go
}

// SinglelineStringWithShortTypes returns a representation of v on a single line, with types shortened.
func (v *Variable) SinglelineStringWithShortTypes() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add a new function instead of adding a flag to SinglelineString, as that function has tons of call sites that I would have had to adapt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SinglelineStringFormatted as very few calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean I should use SinglelineStringFormatted instead of adding SinglelineStringWithShortTypes? If so, should I change its behavior to always shorten types, or add a bool parameter to it to make it configurable? Personally I'd find it preferable to add the new function, as I don't need the ability to pass a format string.

service/api/prettyprint.go Outdated
return v.Type
}

func (v *Variable) writeTo(buf io.Writer, top, newlines, includeType, shortenType bool, indent, fmtstr string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also considered turning the includeType bool into an enum with three values (off, short, full). That didn't work well though, because of the way includeType is negated sometimes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having four booleans on a row crosses a threshold. If @derekparker agrees I would rebase this on something like this #3536

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fine with me. I suppose I'd wait until that PR is merged and then rebase on master? There's no rush, I'm now on vacation for two weeks, so can only continue with this after I'm back anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having four booleans on a row crosses a threshold. If @derekparker agrees I would rebase this on something like this #3536

Agreed.

@stefanhaller stefanhaller mentioned this pull request Oct 20, 2023
Closed
aarzilli
aarzilli reviewed Oct 22, 2023
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't going to be any kind of configuration to turn this off?

service/api/prettyprint.go
}

// SinglelineStringWithShortTypes returns a representation of v on a single line, with types shortened.
func (v *Variable) SinglelineStringWithShortTypes() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SinglelineStringFormatted as very few calls.

@stefanhaller
Copy link
Contributor Author

There isn't going to be any kind of configuration to turn this off?

I don't think it needs to be configurable, I really can't imagine why anybody would want to see the long types. We didn't make #3500 configurable either, and I think this is very similar. Also, you can see the full type in a tooltip by hovering over a variable name.

@aarzilli
Copy link
Member

why anybody would want to see the long types

Because short ones could be ambiguous. But if they are visible on hover it's probably ok.

@aarzilli aarzilli mentioned this pull request Oct 24, 2023
Closed
@stefanhaller
Add ShortenType function
8a55084 
Taken from

  https://github.com/aarzilli/gdlv/blob/master/internal/prettyprint/short.go

with kind permission by @aarzilli.
@stefanhaller
Shorten type names in variable values
8fc0299 
The variables view in VS Code is a lot easier to read if long type names are
shortened in much the same way as we shorten them for functions in the call
stack view.

We only shorten them in the value strings; the Type field of dap.Variable is
kept as is. Since this only appears in a tooltip, it isn't a problem to have the
full type visible there.
@stefanhaller
Copy link
Contributor Author

Sorry for the delay, I was on vacation.

I rebased this onto master, I'm quite happy about how much cleaner the change is now. Can you please have another look?

aarzilli
aarzilli approved these changes Nov 5, 2023
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aarzilli
Copy link
Member

aarzilli commented Nov 8, 2023

cc @hyangah @suzmue you wanted to review this I think.

suzmue
suzmue approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@suzmue suzmue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @stefanhaller!!

@aarzilli aarzilli merged commit aec354c into go-delve:master Nov 9, 2023
1 of 2 checks passed
@stefanhaller stefanhaller deleted the shorten-variable-types branch November 9, 2023 12:46
kakkoyun added a commit to parca-dev/parca-agent that referenced this pull request Jan 8, 2024
@kakkoyun
chore(deps): update module github.com/go-delve/delve to v1.22.0 (#2404)
87751d9 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/go-delve/delve](https://togithub.com/go-delve/delve) |
`v1.21.2` -> `v1.22.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgo-delve%2fdelve/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgo-delve%2fdelve/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgo-delve%2fdelve/v1.21.2/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgo-delve%2fdelve/v1.21.2/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>go-delve/delve (github.com/go-delve/delve)</summary>

###
[`v1.22.0`](https://togithub.com/go-delve/delve/releases/tag/v1.22.0)

[Compare
Source](https://togithub.com/go-delve/delve/compare/v1.21.2...v1.22.0)

#### What's Changed

- all: remove obsolete build tags by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3513
- service/dap: add the concrete type to interface type by
[@&#8203;suzmue](https://togithub.com/suzmue) in
[go-delve/delve#3510
- service/dap: fix bugs in stdout/stderr handling by
[@&#8203;hyangah](https://togithub.com/hyangah) in
[go-delve/delve#3522
- pkg/terminal: support vscode in edit command by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3524
- Use a valid timezone in tested binary by
[@&#8203;upils](https://togithub.com/upils) in
[go-delve/delve#3527
- proc: implement follow exec mode on Windows by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3507
- service/test: disable TestClientServer_chanGoroutines with rr backend
by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3492
- proc/native: use cgo instead of C for freebsd by
[@&#8203;4a6f656c](https://togithub.com/4a6f656c) in
[go-delve/delve#3529
- proc: use DW_AT_trampoline to detect auto-generated code by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3528
- proc: use stack machine to evaluate expressions by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3508
- proc: fix comment typos by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3531
- proc: add min and max builtins by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3530
- CI: update staticcheck version by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3533
- proc: remove expr evaluation goroutine from EvalExpressionWithCalls by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3532
- service/api: use bitfield for prettyprint flags by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3536
- proc: allow evaluator to reference previous frames by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3534
- \*: Add explicit code of conduct by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3540
- proc,service/dap,proc/gdbserial: fixes for debugserver
--unmask-signals by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3541
- \*: release 1.21.2 (for master) by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3546
- proc/native: wherever we check for exited we should also check
detached by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3547
- pkg/proc: add inline function support for stripped binaries by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3549
- cmd: fix a bunch of linter warnings from GoLand by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3550
- proc: add regression test for issue
[#&#8203;3548](https://togithub.com/go-delve/delve/issues/3548) by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3553
- \*: update dependencies by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3552
- service: fix a bunch of linter warnings from GoLand by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3551
- \*: Correct spelling mistakes by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3555
- pkg,service: Remove redundant build constraints by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3556
- Shorten variable types by
[@&#8203;stefanhaller](https://togithub.com/stefanhaller) in
[go-delve/delve#3535
- TeamCity: reupgrade linux/386 builder to Go 1.21 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3560
- pkg/proc: improve support unwinding from sigpanic by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3559
- pkg/proc: unskip passing tests and reorganize by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3561
- chore: use strings.Contains instead by
[@&#8203;testwill](https://togithub.com/testwill) in
[go-delve/delve#3562
- pkg,service: remove unnecessary convertions by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3564
- \*: remove checks for TRAVIS env variable by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3568
- terminal: clear substitute path rules cache when config is used by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3567
- service: fix typo in variable name by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3575
- TeamCity: remove windows/arm64 builders from chain by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3572
- proc: simplify and generalize runtime.mallocgc workaround by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3571
- proc/gdbserial: refactor parsing of key-value pairs from gdb protocol
by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3574
- Extract tip builds into a separate sub-project by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3578
- pkg,service/dap: use switch instead of if-else-if by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3576
- CI: update teamcity settings by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3579
- pkg/proc: use gore to obtain info from stripped binaries by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3577
- add missing import in TeamCity DSL by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3580
- update TeamCity DSL version to 2023.05 and remove tip configurations
from Aggregator by [@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3581
- proc: fix TestIssue1101 flake by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3585
- proc: skip trapthread for harcoded breakpoints after manual stop by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3582
- tests: fix tests in go1.22 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3583
- all: run go mod tidy by
[@&#8203;prattmic](https://togithub.com/prattmic) in
[go-delve/delve#3589
- add all branches but PRs to filter by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3590
- service/dap: fix close on closed channel panic by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3573
- Show pprof labels in thread names by
[@&#8203;stefanhaller](https://togithub.com/stefanhaller) in
[go-delve/delve#3501
- \*: Use forked goretk/gore module by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3597
- proc: make some type casts less counterintuitive by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3596
- teamcity,version: add 1.22 to supported versions and CI matrix by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3601
- proc: fix ppc64 arch name check by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3608
- goversion: include pre-releases in VersionAfterOrEqual check by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3607
- service/dap: fix close on closed channel by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3609
- proc: fix TestPackageRenames on go1.22 on linux/386 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3610
- \*: Update gore dep for 1.22 by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3611
- \*: release version 1.22.0 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3606

#### New Contributors

- [@&#8203;upils](https://togithub.com/upils) made their first
contribution in
[go-delve/delve#3527
- [@&#8203;testwill](https://togithub.com/testwill) made their first
contribution in
[go-delve/delve#3562
- [@&#8203;prattmic](https://togithub.com/prattmic) made their first
contribution in
[go-delve/delve#3589

**Full Changelog**:
go-delve/delve@v1.21.1...v1.22.0
**Curated Changelog**:
https://github.com/go-delve/delve/blob/master/CHANGELOG.md#1220-2023-12-29

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/parca-dev/parca-agent).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekparker derekparker derekparker left review comments

@aarzilli aarzilli aarzilli approved these changes

@suzmue suzmue suzmue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dap: strip package paths from variable types
4 participants
@stefanhaller @aarzilli @derekparker @suzmue