Skip to content

ssa: fix map value closure check#1509

Merged
xushiwei merged 1 commit intogoplus:mainfrom
visualfc:mapclosure
Dec 27, 2025
Merged

ssa: fix map value closure check#1509
xushiwei merged 1 commit intogoplus:mainfrom
visualfc:mapclosure

Conversation

@visualfc
Copy link
Member

@visualfc visualfc commented Dec 27, 2025

fix map value func decl to closure check.

fix #1411

@gemini-code-assist
Copy link

Summary of Changes

Hello @visualfc, 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 addresses a critical issue in the Static Single Assignment (SSA) construction phase of the compiler, specifically targeting the correct handling and type checking of function declarations (closures) when they are used as values within map assignments. The changes ensure that the compiler properly processes these function values, preventing potential inconsistencies or errors in the generated SSA form. A dedicated test case has been introduced to validate the robustness of this fix.

Highlights

  • SSA Fix for Map Value Closures: Implemented a fix within the SSA builder to correctly handle function declarations (closures) when they are assigned as values in map data structures. This ensures proper type checking and SSA representation for such assignments.
  • New Test Case Added: A new Go program (_demo/go/mapclosure/main.go) has been added to demonstrate the specific scenario where function values are stored in maps and slices, serving as a validation test for the SSA fix.
  • MapUpdate Function Enhancement: The MapUpdate function in ssa/datastruct.go now includes a conditional check to identify vkFuncDecl (function declaration kind) values and applies checkExpr to them, ensuring they are correctly processed during map assignments.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to fix an issue with handling closures as map values. A test case is added to reproduce the issue. The fix in ssa/datastruct.go attempts to convert function declarations to closures before map assignment.

However, the implementation of the fix has a logical error: it uses the function's own type as the target for conversion, which is incorrect. My review includes a correction to use the map's value type as the target, which should properly fix the underlying issue.

@xgopilot
Copy link
Contributor

xgopilot bot commented Dec 27, 2025

Code Review Summary

Recommendation: APPROVE

This is a well-executed, minimal fix that correctly addresses a closure handling issue when storing function values in maps. The implementation:

  • Follows established patterns: Mirrors the proven approach from MakeInterface (interface.go:115-118)
  • Maintains type safety: Properly uses checkExpr for function declaration conversion
  • Includes proper test coverage: The demo test validates the fix effectively
  • Performance impact: Negligible - only adds an O(1) type check with conditional processing for the uncommon case

One minor suggestion: Added an inline comment to improve code maintainability for future developers.

All specialized reviews (code quality, performance, security, documentation) found no blocking issues.

@codecov
Copy link

codecov bot commented Dec 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.93%. Comparing base (380a6d5) to head (211b890).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1509   +/-   ##
=======================================
  Coverage   90.93%   90.93%           
=======================================
  Files          44       44           
  Lines       11525    11528    +3     
=======================================
+ Hits        10480    10483    +3     
  Misses        883      883           
  Partials      162      162           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xushiwei xushiwei merged commit 3e5807d into goplus:main Dec 27, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go/types.Check got nil pointer dereference when check a signature

2 participants