Skip to content

feat(cl): add static value member semantics#2792

Open
KurodaKayn wants to merge 15 commits into
goplus:mainfrom
KurodaKayn:static-member-semantic
Open

feat(cl): add static value member semantics#2792
KurodaKayn wants to merge 15 commits into
goplus:mainfrom
KurodaKayn:static-member-semantic

Conversation

@KurodaKayn

Copy link
Copy Markdown
Contributor

Summary

  • Adds cl semantic support for static value declarations introduced by feat(syntax): add static value declarations #2783, including const T.name, var T.count, and classfile .name shorthand.
  • Resolves static value references through T.name selectors and classfile bare-name fallback while preserving local selector shadowing.
  • Validates explicit static receivers as package-scope types and records selector receiver uses for recorder completeness.
  • Part of Proposal: Static Members #2513.

Implementation

  • Maps static declaration names to generated XGos_T_Name symbols during preload/load and keeps static classfile values out of instance field generation.
  • Splits mixed classfile value specs so static names become package symbols while bare names remain instance fields.
  • Gates static selector resolution on a package-scope type receiver, so locals, imports, and undeclared receivers do not resolve as static members.
  • Records both the selector member and explicit receiver type for static selector references.

Testing

  • go test ./cl -run 'TestStaticMemberMissingReceiver|TestStaticMembers|TestStaticMemberUnicodeName|TestStaticMemberSelectorShadow|TestClassfileStaticMembers|TestClassfileStaticVarBeforeFields|TestClassfileMixedStaticAndFieldValueSpec|TestStaticMemberSelectorRecorderUsesReceiver' -count=1 passes.
  • go test ./parser ./cl passes.
  • go test ./parser ./cl -cover passes: parser coverage 95.2%, cl coverage 98.0%.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.01478% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.24%. Comparing base (0ac0cf4) to head (2fac8bc).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
cl/expr.go 97.14% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2792      +/-   ##
==========================================
+ Coverage   94.14%   94.24%   +0.10%     
==========================================
  Files          32       32              
  Lines       10316    10481     +165     
==========================================
+ Hits         9712     9878     +166     
+ Misses        432      430       -2     
- Partials      172      173       +1     

☔ View full report in Codecov by Harness.
📢 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.

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

Copy link
Copy Markdown
Contributor

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 support for static members (static constants and variables associated with types or classes) in the compiler. It updates the compilation process to recognize, validate, and compile static member declarations and references (such as foo.name or .name in classfiles), translating them to package-level symbols with a specific naming convention (e.g., XGos_foo_Name) and recording their usage. Additionally, comprehensive unit tests have been added to verify various static member scenarios, including name translation, selector shadowing, missing receivers, and classfile integration. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: feat(cl): add static value member semantics

The implementation is well-structured and the helper decomposition is clean. The tests cover the primary happy paths, unicode, missing receivers, shadowing, and mixed specs. A few correctness and maintainability issues worth addressing before merge:


HasStatic inconsistency in valueSpecSubset all-same-kind branch

When all names in a spec are the same kind (all static or all non-static), valueSpecSubset returns the original v unchanged — without setting v.HasStatic = wantStatic. The mixed branch (len(idxs) < len(v.Names)) does set spec.HasStatic = wantStatic on the copy. If any downstream code checks HasStatic, the all-static spec will be mis-flagged. Either set the flag before returning v in that branch, or confirm and document that HasStatic is intentionally unset for pure specs.


Asymmetric validation between implicit .name and explicit T.name

validateStaticValueSpec uses explicitStaticReceiverName, which returns ok=false for leading-dot names (.name). So an implicit .name declared where classRecvName(ctx) == "" silently falls back to a non-static symbol with no diagnostic, while the same logical mistake with explicit Missing.name correctly produces "undefined: Missing". The two syntax forms should produce consistent diagnostics when the receiver is absent/invalid.


staticMember is a pure alias to staticMethod — shared namespace between value members and methods

Static value members (const foo.name) and static methods share the same name-mangling scheme (XGos_T_Name). A value member foo.Name and a static method named Name on foo would generate the same symbol, causing a confusing redeclaration error. If this collision is intentional by design (shared namespace), add a comment explaining it. If it is not intended, consider a distinct prefix (e.g. XGosV_) for value members.


lookupPackageTypeName early-return suppresses lazy loading

if obj := scope.Lookup(name); obj != nil {
    if t, ok := obj.(*types.TypeName); ok { return t }
    return nil  // returns here without trying loadSymbol
}
if ctx.loadSymbol(name) { ... }

If name is already in scope but resolves to a non-type (a var or func placeholder from a prior partial load), the function returns nil without giving loadSymbol a chance to refine the binding. The lazy-load path is only reached for names absent from scope. This creates a load-order sensitivity: two identical programs can have different validation outcomes depending on whether the name was previously (partially) inserted.


Performance: staticMemberRecvNames allocates on every identifier

tryStaticMemberIdent is called for every identifier in a class-context file. It calls staticMemberRecvNames(ctx) each time, which allocates a new []string slice via append. The receiver set (0–2 names) is invariant for the lifetime of the blockCtx. Consider caching it on blockCtx once per file to avoid per-identifier allocations.


Minor: test coverage gaps

Two cases not currently covered:

  1. Multiple static member types in the same filestaticMemberRecvNames and classTypes are keyed per type; a multi-type-per-file scenario is untested.
  2. Static const with iota — the iota-grouping case where v.Values is empty (iota implied) combined with the valueSpecSubset path is unverified.

Positive observations

  • Unicode-aware first-rune uppercasing (utf8.DecodeRuneInString) is correct — a common mistake spot.
  • canUseStaticMemberSelector correctly rejects locals that shadow a type name, so foo.name on a local foo variable takes the instance-field path as expected.
  • The hasSymbol/loadSymbol guard pattern in tryStaticMemberName/tryStaticMemberRef avoids forcing loads for absent symbols.
  • Error messages mirror Go's own style ("undefined: %s", "%s is not a type").

View job run

Comment thread cl/compile.go
Comment thread cl/compile.go
Comment thread cl/compile.go
Comment thread cl/compile.go
Comment thread cl/expr.go
@KurodaKayn KurodaKayn force-pushed the static-member-semantic branch from 7f9c933 to 07d19c9 Compare June 23, 2026 12:23
@xushiwei

Copy link
Copy Markdown
Member

Add test cases to the cl/_testxgo directory instead of writing new test functions. See the TestTestxgo test function in cl/compile_testdir_test.go for details.

@KurodaKayn KurodaKayn force-pushed the static-member-semantic branch from dfe2288 to 2fac8bc Compare June 23, 2026 14:52
@KurodaKayn

Copy link
Copy Markdown
Contributor Author

Add test cases to the cl/_testxgo directory instead of writing new test functions. See the TestTestxgo test function in cl/compile_testdir_test.go for details.

alright, i've fixed that
image

classfile cases went to cl/_testspx because TestTestxgo only loads in.xgo.
kept recorder/error tests as Go tests: _testxgo cannot assert Recorder callbacks or expected compile errors.

@xushiwei

Copy link
Copy Markdown
Member

The adjustments made to the cl module are too significant. A better approach would be to let the gogen module handle the name resolution process (which is how other features currently operate).

@@ -0,0 +1,17 @@
package main

const XGos_Rect_Name = "rect"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be XGos_Rect_name

type Rect struct {
}

var XGos_Rect_Count int = 100

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be XGos_Rect_count


type foo int

const XGos_foo_Name = "xgo"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be XGos_foo_name

@KurodaKayn

KurodaKayn commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

The adjustments made to the cl module are too significant. A better approach would be to let the gogen module handle the name resolution process (which is how other features currently operate).

indeed
i've opened an issue and a PR in repo gogen

goplus/gogen#637
goplus/gogen#638

however this pr just solved static value member core

the XGos_T_name import is gonna be solved next

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