Skip to content

func SetVarKind/VarKind#594

Open
visualfc wants to merge 1 commit intogoplus:mainfrom
visualfc:varkind
Open

func SetVarKind/VarKind#594
visualfc wants to merge 1 commit intogoplus:mainfrom
visualfc:varkind

Conversation

@visualfc
Copy link
Copy Markdown
Member

@visualfc visualfc commented Mar 5, 2026

implement goplus/xgo#2663

  • (*Package).NewResult
  • (*Package).NewRecv

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 introduces a standardized way to manage and retrieve variable kinds across different Go versions. It adds SetVarKind and VarKind functions, along with a VarKind type and its constants, to provide a consistent API. This change ensures that variable kind information can be handled uniformly, whether using the native go/types package features available in newer Go versions or a custom implementation for legacy environments.

Highlights

  • Introduction of VarKind API: New SetVarKind and VarKind methods are introduced to manage variable kinds, along with a VarKind type and associated constants.
  • Go 1.25+ Compatibility: For Go 1.25 and later, the VarKind type is aliased to go/types.VarKind, and the new methods directly interact with the types.Var's Kind() and SetKind() methods.
  • Legacy Go Version Support: For older Go versions, the optionalVars struct is adapted to store VarKind in its paramsMeta map, and custom SetVarKind and VarKind methods are provided to emulate the behavior.

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

Changelog
  • param_go125.go
    • Added SetVarKind and VarKind methods to optionalVars.
    • Defined VarKind type as an alias to types.VarKind.
    • Introduced VarKind constants.
  • param_legacy.go
    • Modified optionalVars to use VarKind for paramsMeta instead of bool.
    • Updated setParamOptional and isParamOptional to work with VarKind.
    • Added SetVarKind and VarKind methods to optionalVars.
    • Defined VarKind type as uint8 with associated constants.
    • Implemented String() method for VarKind.
Activity
  • The pull request is currently a Work In Progress ([WIP]) and lacks a detailed description, indicating that it is still under active development and has not yet received formal review or comments.
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
Copy Markdown
Contributor

@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 introduces SetVarKind and VarKind functions to handle variable kinds, with build-tag-based implementations for Go 1.25+ and older versions. This is a good approach for managing version-specific logic. My review includes suggestions to improve robustness and maintainability. Specifically, for the Go 1.25+ implementation, I recommend aliasing constants from the go/types package instead of redefining them to avoid potential inconsistencies. For the legacy implementation, I've pointed out an opportunity to reduce code duplication and to replace a magic number with a named constant for better readability.

Comment thread param_go125.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_go125.go Outdated
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.10%. Comparing base (6641c10) to head (8d8a190).

Files with missing lines Patch % Lines
stmt.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
+ Coverage   94.07%   94.10%   +0.02%     
==========================================
  Files          28       29       +1     
  Lines        7331     7364      +33     
==========================================
+ Hits         6897     6930      +33     
  Misses        358      358              
  Partials       76       76              

☔ 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.

@xgopilot
Copy link
Copy Markdown
Contributor

xgopilot bot commented Mar 5, 2026

The PR introduces SetVarKind/VarKind accessors with clean Go 1.25+ delegation via types.Var.SetKind/Kind. Two blocking issues need resolution before merging:

  1. Compile error (param_legacy.go): fmt.Sprintf in String() but "fmt" is not imported.
  2. Data corruption (param_legacy.go): setParamOptional (sentinel 0xff) and SetVarKind share the same map — a SetVarKind call on an optional param silently destroys its optional marking.

The constant re-declaration in param_go125.go also warrants verification against types.VarKind ordinals.

@visualfc visualfc marked this pull request as draft March 5, 2026 02:51
@visualfc visualfc marked this pull request as ready for review March 6, 2026 03:39
Comment thread func.go Outdated
Comment thread func.go Outdated
Comment thread param_go125.go Outdated
Comment thread param_go125.go Outdated
Comment thread param_legacy.go Outdated
Comment thread param_legacy.go
Comment thread type_var_and_const.go
@xgopilot
Copy link
Copy Markdown
Contributor

xgopilot bot commented Mar 6, 2026

Good direction — the SetVarKind/VarKind API provides a clean abstraction over the go1.25 native Var.Kind() field. A few issues worth resolving before merging:

  • func.go:55 — wrong name in doc comment (NewReturn vs NewResult).
  • param_go125.go:41type VarKind types.VarKind with independent iota constants creates a silent numeric coupling to go/types; no compile-time assertion guards it.
  • param_legacy.go:43setParamOptional and SetVarKind share one map, so calling either overwrites the other's state.
  • param_legacy.go:46 / type_var_and_const.go:464VarKind() returns 0 for unregistered vars on the legacy path but the correct kind on go1.25, creating a cross-build behavioral difference.
  • param_go125.go:39varKindLegacy is unused dead code in both build files.
  • Coverage dropped ~0.3%; NewResult/NewRecv/SetVarKind/VarKind have no tests yet.

Copy link
Copy Markdown
Member

@aofei aofei left a comment

Choose a reason for hiding this comment

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

There is also an additional inconsistency outside this diff: CodeBuilder.NewAutoVar in codebuild.go still creates a bare types.NewVar, so function-local declarations created through that builder keep the default PackageVar on Go 1.25+ instead of LocalVar.

Comment thread func.go Outdated
Comment thread type_var_and_const.go
aofei added a commit to aofei/fork.goplus.xgolsw that referenced this pull request Mar 26, 2026
Replace `gogen` and `xgo` with pseudo-versions that expose the new
variable kind metadata and use it throughout the server.

This distinguishes receivers, parameters, named results, and local
variables in semantic tokens and spx definitions while keeping field
checks on `IsField()` where that API is clearer. Add regression tests
for `TypeInfo`, semantic tokens, hover overviews, and spx definitions.

Updates goplus/gogen#594
Updates goplus/xgo#2665

Signed-off-by: Aofei Sheng <aofei@aofeisheng.com>
aofei added a commit to aofei/fork.goplus.xgolsw that referenced this pull request Mar 26, 2026
This distinguishes receivers, parameters, named results, and local
variables in semantic tokens and spx definitions while keeping field
checks on `IsField()` where that API is clearer. Add regression tests
for `TypeInfo`, semantic tokens, hover overviews, and spx definitions.

Updates goplus/gogen#594
Updates goplus/xgo#2665

Signed-off-by: Aofei Sheng <aofei@aofeisheng.com>
@visualfc visualfc force-pushed the varkind branch 2 times, most recently from 8d2fc68 to 103aebc Compare April 3, 2026 02:00
@visualfc visualfc changed the title [WIP] func SetVarKind/VarKind func SetVarKind/VarKind Apr 4, 2026
@visualfc visualfc force-pushed the varkind branch 2 times, most recently from da47c9c to 2e4483b Compare April 15, 2026 07:04
@aofei
Copy link
Copy Markdown
Member

aofei commented Apr 16, 2026

package main

import (
	"fmt"
	"go/token"
	"go/types"

	"github.com/goplus/gogen"
)

func main() {
	pkg := gogen.NewPackage("", "main", nil)
	cb := pkg.NewFunc(nil, "main", nil, nil, false).BodyStart(pkg)

	var auto *types.Var
	cb.NewAutoVar(token.NoPos, token.NoPos, "auto", &auto)

	cb.NewVar(types.Typ[types.Int], "normal")
	normal := cb.Scope().Lookup("normal").(*types.Var)

	fmt.Printf("auto: pkg.VarKind=%v, go/types.Kind=%v\n", pkg.VarKind(auto), auto.Kind())
	fmt.Printf("normal: pkg.VarKind=%v, go/types.Kind=%v\n", pkg.VarKind(normal), normal.Kind())
}

Should NewAutoVar also set LocalVar explicitly so this path stays consistent with the other local-variable creation paths?

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.

2 participants