refactor: replace golang.org/x/exp with stdlib
#3881
Conversation
Juneezee
requested review from
denis256,
levkohimins and
yhakbar
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes update numerous import statements across the project to replace experimental packages (such as Changes
Possibly Related PRs
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/log/format/options/common.go (1)
114-114: Consider usingslices.Collectfor better performanceWhile
slices.Sorted(maps.Values())works, it creates an intermediate slice. Usingslices.Collect(maps.Values())would be more efficient as it directly collects the iterator values into a sorted slice.-list := slices.Sorted(maps.Values(val.list)) +list := slices.Collect(maps.Values(val.list)) +slices.Sort(list)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cli/app.go(1 hunks)cli/commands/run/creds/getter.go(1 hunks)cli/commands/run/creds/providers/externalcmd/provider.go(1 hunks)cli/flags/flag_opts.go(1 hunks)go.mod(7 hunks)internal/cli/bool_flag_test.go(1 hunks)internal/cli/help.go(1 hunks)internal/experiment/experiment.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/controls/deprecated_env_var.go(1 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)internal/strict/status.go(1 hunks)pkg/log/format/format.go(2 hunks)pkg/log/format/options/color.go(2 hunks)pkg/log/format/options/common.go(2 hunks)pkg/log/format/options/time_format.go(2 hunks)tf/getproviders/lock.go(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- internal/strict/status.go
- internal/experiment/experiment.go
- cli/commands/run/creds/providers/externalcmd/provider.go
- cli/commands/run/creds/getter.go
- internal/cli/help.go
- internal/strict/control.go
- internal/strict/controls/deprecated_flag_name.go
- internal/cli/bool_flag_test.go
- cli/app.go
- cli/flags/flag_opts.go
- tf/getproviders/lock.go
- internal/strict/controls/deprecated_env_var.go
🔇 Additional comments (8)
internal/strict/category.go (1)
3-7: Clean import update! 👍The switch from
golang.org/x/exp/slicesto stdlibslicesis spot on. The functionality remains identical, and we're now using the stable version.pkg/log/format/options/time_format.go (1)
82-88: Nice use ofslices.SortedFunc! 🎯Great job using
slices.SortedFuncwithstrings.Compare. This is the most idiomatic way to sort slices in Go 1.21+, and it makes the code more readable and maintainable.pkg/log/format/format.go (1)
152-152: Perfect use ofslices.Collect! ✨Great job using
slices.Collectto handle the iterator frommaps.Keys. This is exactly how we should handle the transition from experimental packages to stdlib.pkg/log/format/options/color.go (2)
4-5: LGTM! Good move to stdlib imports.The switch from experimental packages to standard library imports is a solid improvement. This aligns perfectly with Go's stability guarantees.
111-111: LGTM! Nice use ofslices.Collect.The change to use
slices.Collect(maps.Values())is the right way to handle the iterator-to-slice conversion in Go 1.23.go.mod (3)
266-281: LGTM! Well-documented replace directives.The replace directives are clearly documented with the rationale for each replacement. This is super helpful for future maintenance.
3-3:✅ Verification successful
Verify Go 1.23 compatibility.
The upgrade to Go 1.23 is needed for the stdlib
mapsandslicespackages, but let's make sure it's a released version.
🌐 Web query:
What is the latest version of Go?💡 Result:
The latest stable version of Go is 1.23, with the most recent minor revision being 1.23.6 (released February 4, 2025)[4]. This release follows Go's six-month release cycle, succeeding Go 1.22[3][4].
Key details about Go versions:
- Go 1.23 introduced toolchain improvements while maintaining backward compatibility[3]
- Security updates and bug fixes are regularly backported to previous major versions:
- Go 1.22.x series has updates through 1.22.12[4]
- Go 1.21.x series is supported up to 1.21.12[4]
For most users, upgrading to Go 1.23.x is recommended unless maintaining legacy systems requiring older versions[1][4]. The Go team maintains support for only the two most recent major versions (currently 1.22 and 1.23)[1][3].
Citations:
- 1: https://github.com/golang/tools/releases
- 2: http://forum.exercism.org/t/update-go-version-to-the-latest-one/9326
- 3: https://tip.golang.org/doc/go1.23
- 4: https://go.dev/doc/devel/release
- 5: https://tip.golang.org/doc/go1.22
- 6: https://go.dev/dl
Go 1.23 Verified for Compatibility
Great news—the
go.modfile now specifiesgo 1.23, which is confirmed as the latest stable version (with minor revisions like 1.23.6 already out). This upgrade is spot on for enabling the new stdlib packages such asmapsandslices. No further changes are needed here.
254-254:✅ Verification successful
Consider removing
golang.org/x/expdependency.Since we're moving away from experimental packages to stdlib, we might be able to remove this dependency entirely if it's not used elsewhere.
🏁 Script executed:
#!/bin/bash # Check if golang.org/x/exp is still used rg -l "golang.org/x/exp"Length of output: 38
Dependency Removal Check:
golang.org/x/expIt looks like the only references to
golang.org/x/expare in thego.mod(listed as an indirect dependency) andgo.sumfiles—there’s no direct usage in the code. This indicates that if none of our code (or an essential transitive dependency) actively uses it, we can safely remove it. I'd recommend removing it fromgo.mod(and running ago mod tidy) to see if it cleans up without side effects. Please verify by running your tests to ensure that no library breaks as a result.
|
Hey @Juneezee , Do you mind rebasing and running |
Juneezee
force-pushed
the
refactor/exp
branch
from
4a41585 to
1ad5a5f
Compare
Juneezee
force-pushed
the
refactor/exp
branch
from
1ad5a5f to
aa4b663
Compare
Juneezee
force-pushed
the
refactor/exp
branch
from
aa4b663 to
4523d00
Compare
Juneezee
force-pushed
the
refactor/exp
branch
from
4523d00 to
1a0f837
Compare
|
@yhakbar Done! Please take a look again. Thank you 😊 |
Juneezee
force-pushed
the
refactor/exp
branch
from
1a0f837 to
462b1f8
Compare
|
The recent PR #3880 also did some of the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
37-41: Consider moving the comment above the dependency.The comment explaining the version constraint for terraform would be more visible if placed directly above the dependency.
+// Many functions of terraform was converted to internal to avoid use as a library after v0.15.3. This means that we +// can't use terraform as a library after v0.15.3, so we pull that in here. github.com/hashicorp/terraform v0.15.3 -// Many functions of terraform was converted to internal to avoid use as a library after v0.15.3. This means that we -// can't use terraform as a library after v0.15.3, so we pull that in here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cli/app.go(1 hunks)cli/commands/run/creds/getter.go(1 hunks)cli/commands/run/creds/providers/externalcmd/provider.go(1 hunks)cli/flags/flag_opts.go(0 hunks)go.mod(7 hunks)internal/cli/bool_flag_test.go(1 hunks)internal/cli/help.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/controls/deprecated_env_var.go(0 hunks)internal/strict/controls/deprecated_flag_name.go(0 hunks)pkg/log/format/format.go(2 hunks)pkg/log/format/options/color.go(2 hunks)pkg/log/format/options/common.go(2 hunks)pkg/log/format/options/time_format.go(2 hunks)tf/getproviders/lock.go(1 hunks)
💤 Files with no reviewable changes (3)
- internal/strict/controls/deprecated_env_var.go
- internal/strict/controls/deprecated_flag_name.go
- cli/flags/flag_opts.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/strict/category.go
- cli/app.go
- internal/cli/help.go
- cli/commands/run/creds/providers/externalcmd/provider.go
- internal/strict/control.go
- tf/getproviders/lock.go
- internal/cli/bool_flag_test.go
- pkg/log/format/format.go
- pkg/log/format/options/color.go
- pkg/log/format/options/common.go
- cli/commands/run/creds/getter.go
🔇 Additional comments (4)
pkg/log/format/options/time_format.go (2)
3-8: Nice work on the stdlib migration! 👍Clean transition from experimental packages to standard library. The imports look good and align perfectly with the PR's goal.
82-88:✅ Verification successful
Clean implementation using modern Go features! 🎯
The new implementation looks great! It's more concise and uses modern stdlib features effectively. Just want to double-check that the sorting behavior remains identical.
Let's verify the sorting behavior matches the old implementation:
🏁 Script executed:
#!/bin/bash # Search for any tests that might be affected by this change rg -l "SortedKeys.*Test" . # Look for any other usage of this method rg "SortedKeys\(\)" -A 3 -B 3Length of output: 2212
Great job using modern Go constructs to streamline the sorting logic!
The new implementation using
maps.Keysandslices.SortedFuncnot only makes the code more concise but also preserves the expected behavior. I verified that its usage throughout the codebase—particularly in thepkg/log/format/options/time_format.gofile and its related functions—remains consistent with the original intent, ensuring deterministic iteration order. Everything checks out.go.mod (2)
266-281: LGTM! Well-documented replace directives.The replace directives are well-documented with clear explanations for each constraint:
- Handling 404 issues with atomicgo.dev
- Managing Terraform version constraints
- Working around Vault module issues
- Managing TFLint license changes
3-3:⚠️ Potential issue✅ Verification successful
Verify Go version compatibility.
The module requires Go 1.23, but this version hasn't been released yet (current latest is 1.22).
Consider updating to Go 1.22 or the latest stable version:
-go 1.23 +go 1.22
🌐 Web query:
What is the latest version of Go?💡 Result:
The latest stable version of Go is 1.23.6, released on February 4, 2025[3][4]. This follows the major release of Go 1.23 on August 13, 2024, which introduced toolchain and runtime improvements while maintaining compatibility with previous versions through Go's strict adherence to semantic versioning[3].
Key details about recent versions:
- Go 1.23.x series has received six minor updates since its initial release, primarily addressing security vulnerabilities and bug fixes in components like
crypto/ellipticand compiler optimizations[4]- The current support policy maintains compatibility with the two most recent major versions (1.22 and 1.23)[1]
- Go's automatic toolchain upgrades (introduced in 1.21) ensure developers using recent versions can seamlessly update dependencies[1]
For most users, installing via
go install golang.org/dl/go1.23.6@latestwill provide the latest stable build[4].Citations:
- 1: https://github.com/golang/tools/releases
- 2: http://forum.exercism.org/t/update-go-version-to-the-latest-one/9326
- 3: https://tip.golang.org/doc/go1.23
- 4: https://go.dev/doc/devel/release
- 5: https://tip.golang.org/doc/go1.22
- 6: https://go.dev/dl/
Heads-up: Go version is spot on.
The module specifies
go 1.23ingo.mod, and with Go 1.23.6 being the latest stable release, this directive is correct. There's no need to roll it back to 1.22—everything is in line with the current Go release series.
|
This branch has conflicts that must be resolved go.mod |
The experimental functions are now available in the standard library in Go 1.21 [1] and Go 1.23 [2]. The difference is that `maps.Keys`/`maps.Values` in the `golang.org/x/exp/maps` package return a slice, whereas `maps.Keys`/`maps.Values` in the standard library return an iterator. To work with slices, we need to use `slices.Collect` to convert the iterator into a slice. [1]: https://go.dev/doc/go1.21#slices [2]: https://go.dev/doc/go1.23#iterators Signed-off-by: Eng Zer Jun <[email protected]>
Juneezee
force-pushed
the
refactor/exp
branch
from
462b1f8 to
7785dd7
Compare
|
Rebased |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
go.mod (2)
200-200: Klauspost Compress Update
Line 200 updatesgithub.com/klauspost/compress v1.17.11. This library is essential for performance in compression tasks. The update seems proper, but a brief check on performance benchmarks might be worthwhile if compression efficiency is critical in your modules.
266-281: Replace Block Consistency
Thereplaceblock (lines 266–281) hasn’t changed in content, but the comments and remappings are crucial. In particular, the redirection for Terraform and Vault are key for your migration efforts. Verify that these replacements are documented in your migration guide and that they continue to resolve correctly when building the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cli/app.go(1 hunks)cli/commands/run/creds/getter.go(1 hunks)cli/commands/run/creds/providers/externalcmd/provider.go(1 hunks)cli/flags/flag_opts.go(0 hunks)go.mod(7 hunks)internal/cli/bool_flag_test.go(1 hunks)internal/cli/help.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/controls/deprecated_env_var.go(0 hunks)internal/strict/controls/deprecated_flag_name.go(0 hunks)pkg/log/format/format.go(2 hunks)pkg/log/format/options/color.go(2 hunks)pkg/log/format/options/common.go(2 hunks)pkg/log/format/options/time_format.go(2 hunks)tf/getproviders/lock.go(1 hunks)
💤 Files with no reviewable changes (3)
- internal/strict/controls/deprecated_flag_name.go
- internal/strict/controls/deprecated_env_var.go
- cli/flags/flag_opts.go
🚧 Files skipped from review as they are similar to previous changes (12)
- cli/commands/run/creds/providers/externalcmd/provider.go
- tf/getproviders/lock.go
- cli/commands/run/creds/getter.go
- cli/app.go
- internal/cli/bool_flag_test.go
- internal/cli/help.go
- pkg/log/format/options/common.go
- internal/strict/category.go
- pkg/log/format/options/color.go
- pkg/log/format/options/time_format.go
- pkg/log/format/format.go
- internal/strict/control.go
🔇 Additional comments (16)
go.mod (16)
10-10: Reintroducing aws-sdk-go
Line 10 reintroducesgithub.com/aws/aws-sdk-go v1.55.6. The pinning looks intentional. Just double-check that this version is compatible with the rest of your AWS packages and that it doesn’t conflict with the newer v2 dependency further down the list.
16-17: Verify Added Dependencies for PTY & Structs
Lines 16–17 reintroducegithub.com/creack/pty v1.1.24andgithub.com/fatih/structs v1.1.0. These entries appear to be re-added rather than upgraded. Confirm that these versions are still the best fit for your use cases and won’t cause transitive conflicts.
20-20: Check go-errors Package Update
Line 20 now pinsgithub.com/go-errors/errors v1.5.1. This version looks deliberate. It might be worth ensuring any code using this package is compatible with this version update.
22-22: Review Google Shlex Version Pin
Line 22 reintroducesgithub.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510. Since this is still an older pseudo-version, verify that no newer alternatives or fixes are required in your context.
27-29: Terratest & HashiCorp Dependencies Grouping
Lines 27–29 add backgithub.com/gruntwork-io/terratest v0.47.2,github.com/hashicorp/go-cleanhttp v0.5.2, andgithub.com/hashicorp/go-getter v1.7.8. These entries are crucial if your tests or internal modules depend on them. It might be a good idea to cross-check against the migration guide to ensure these versions align with your standard library migration efforts.
32-32: HashiCorp Toolkit Dependencies Review
The changes on lines 32 (go-multierror), 34 (go-safetemp), 35 (go-version), and 36 (hcl/v2) suggest careful version pinning of critical HashiCorp libraries. Ensuring that these remain compatible with both upstream changes and your internal Terraform usage is vital.Also applies to: 34-36
46-46: Reintroduced go-zglob Dependency
Line 46 updatesgithub.com/mattn/go-zglob v0.0.6. This update looks routine; just verify that any file system operations using this library work as expected.
49-51: Mitchellh Package Updates
Lines 49–51 updategithub.com/mitchellh/go-homedir,github.com/mitchellh/go-wordwrap, andgithub.com/mitchellh/mapstructure. These packages are common utilities, so confirming that any breaking changes (if any) are handled in your code would be a safe move.
56-58: Logging, Testing, and Linting Dependencies
Lines 56–58 updategithub.com/sirupsen/logrus v1.9.3,github.com/stretchr/testify v1.10.0, andgithub.com/terraform-linters/tflint v0.55.0. These are key for logging, unit tests, and linting. It’s good to see them pinned; just make sure your CI/CD pipeline and tests aren’t adversely affected by any subtle changes.
60-60: Confirm go-cty Update
Line 60 updatesgithub.com/zclconf/go-cty v1.16.2. Given that this dependency might interact with the conversion from experimental collections to the stdlib, verifying its integration is advisable.
89-89: Cloud Google Package Version Update
Line 89 in the second require block now pinscloud.google.com/go v0.118.2(indirect). This update should be checked for compatibility with your Google Cloud integrations—nothing major here, but a quick verification against your usage would be helpful.
253-254: Persistent x/crypto and x/exp Dependencies
Lines 253–254 updategolang.org/x/crypto v0.32.0andgolang.org/x/exp v0.0.0-20250106191152-7588d65b2ba8as indirect dependencies. Note that even though the PR aims to replace experimental packages with the standard library, an entry forgolang.org/x/expstill remains. It would be good to confirm whether this is brought in transitively or if it can eventually be removed.
256-256: x/time Dependency Verification
Line 256 updatesgolang.org/x/time v0.10.0. This is a straightforward update—just ensure it aligns with any timing or context-related functions that might interact with your stdlib replacements.
258-258: Genproto Update Check
Line 258 updatesgoogle.golang.org/genprototov0.0.0-20250204164813-702378808489. It’s a minor dependency update, but do verify that API changes (if any) are compatible with your current codebase.
222-222: OTP Package Revision
Line 222 updatesgithub.com/pquerna/otp v1.4.0. If your project uses OTP generation or validation, confirm that any API changes or deprecations in this version are handled gracefully.
236-236: ulikunitz/xz Dependency Check
Line 236 now setsgithub.com/ulikunitz/xz v0.5.12. This appears standard—just ensure that any archive or decompression functions relying on it work correctly.
| github.com/hashicorp/terraform v0.15.3 | ||
| github.com/hashicorp/terraform-config-inspect v0.0.0-20210318070130-9a80970d6b34 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Terraform Dependency Internalization
Lines 40–41 reintroduce github.com/hashicorp/terraform v0.15.3 and github.com/hashicorp/terraform-config-inspect v0.0.0-20210318070130-9a80970d6b34. The accompanying comments explain the rationale. Since Terraform’s functions are now internal, please ensure that your migration guide documents this backward-incompatible change clearly for future maintainers.
Description
The experimental functions are now available in the standard library in Go 1.21 (https://go.dev/doc/go1.21#slices) and Go 1.23 (https://go.dev/doc/go1.23#iterators).
The difference is that
maps.Keys/maps.Valuesin thegolang.org/x/exp/mapspackage return a slice, whereasmaps.Keys/maps.Valuesin the standard library return an iterator. To work with slices, we need to useslices.Collectto convert the iterator into a slice.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Replace
golang.org/x/expwith stdlibMigration Guide
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests