fix: handle untyped shift expressions in SSA build#1531
Conversation
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where nested shift expressions involving untyped constants could cause a panic during the SSA build process. The fix introduces a preprocessing step to correctly type these expressions according to Go's specification, ensuring the SSA generator receives properly typed inputs and preventing runtime failures. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a workaround for an upstream bug in go/types that causes panics in the SSA build for certain nested shift expressions. The fix involves iterating through expression types and correcting untyped shift expression results to their default types. A comprehensive set of tests has been added to cover the buggy behavior and ensure the fix is effective. The implementation is correct, but I've suggested a small refactoring to improve readability by reducing nested conditions.
Code Review SummaryExcellent fix for the SSA sanity check failure! The implementation is clean, focused, and includes comprehensive test coverage. Key Strengths:
Critical Issue:
Documentation Improvements:
No security or performance concerns identified. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1531 +/- ##
=======================================
Coverage 91.04% 91.04%
=======================================
Files 45 45
Lines 11999 11999
=======================================
Hits 10924 10924
Misses 899 899
Partials 176 176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This fixes the SSA sanity check failure on nested shift expressions like: value << (1 << bytes[3]) The issue is that go/types returns 'untyped int' for '1 << bytes[3]', but go/ssa sanity check disallows untyped instruction results. According to the Go spec: 'If the left operand of a non-constant shift expression is an untyped constant, it is first implicitly converted to the type it would assume if the shift expression were replaced by its left operand alone.' Added fixUntypedShiftTypes() to convert untyped shift types to their default type (using types.Default()) before calling prog.CreatePackage. Uses two-pass approach to avoid map modification during iteration. Fixes goplus#1530 See also: golang/go#77067 Co-Authored-By: Warp <[email protected]>
Summary
This PR fixes the SSA sanity check failure on nested shift expressions like:
Root Cause
The issue is that
go/typesreturnsuntyped intfor1 << bytes[3], butgo/ssasanity check disallows untyped instruction results, causing a panic:This was reported upstream: golang/go#77067
Solution
Added
fixUntypedShiftTypes()function to convert untyped shift types to their default type (usingtypes.Default()) before callingprog.CreatePackage. This applies the conversion that the Go spec requires:Changes
internal/build/build.go: AddedfixUntypedShiftTypes()workaroundtest/go/shift_untyped_test.go: Added test casesTesting
go test ./test/gopassesgo run ./cmd/llgo test ./test/gopassesFixes #1530