Skip to content

parser: TestErrType, TestErrStaticMember - #2794

Merged
xushiwei merged 1 commit into
goplus:mainfrom
xushiwei:q
Jun 23, 2026
Merged

parser: TestErrType, TestErrStaticMember#2794
xushiwei merged 1 commit into
goplus:mainfrom
xushiwei:q

Conversation

@xushiwei

Copy link
Copy Markdown
Member

No description provided.

@xushiwei
xushiwei merged commit b814471 into goplus:main Jun 23, 2026
9 checks passed

@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 restricts period-prefixed identifier parsing in parseIdentEx to class files only and adds several unit tests covering classfile errors, static member name whitespace errors, and type parsing errors. The reviewer suggested reordering the conditional check in parseIdentEx to evaluate p.tok == token.PERIOD first, which allows the condition to short-circuit and avoids unnecessary method calls.

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.

Comment thread parser/parser.go
var posErr token.Pos
var ret string
if p.tok == token.PERIOD {
if p.inClassFile() && p.tok == token.PERIOD {

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.

medium

For better efficiency, consider reordering the conditions to check p.tok == token.PERIOD first. Since p.tok is rarely token.PERIOD at the start of parseIdentEx, checking it first allows the condition to short-circuit without invoking the p.inClassFile() method call in the vast majority of cases.

Suggested change
if p.inClassFile() && p.tok == token.PERIOD {
if p.tok == token.PERIOD && p.inClassFile() {

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.18%. Comparing base (be46800) to head (0a73f93).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2794      +/-   ##
==========================================
+ Coverage   94.03%   94.18%   +0.14%     
==========================================
  Files          32       32              
  Lines       10301    10301              
==========================================
+ Hits         9687     9702      +15     
+ Misses        440      428      -12     
+ Partials      174      171       -3     

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

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

Focused, surgical fix — adding p.inClassFile() guard correctly restricts the leading-period static-member parsing path to classfiles. All four review dimensions passed cleanly; two minor non-blocking observations below.

Condition ordering (parser/parser.go:715): the current order p.inClassFile() && p.tok == token.PERIOD always calls inClassFile() first. Reversing to p.tok == token.PERIOD && p.inClassFile() lets the cheap token comparison short-circuit for the common case (most identifiers are not preceded by .), skipping the method call entirely. This also matches the pattern in the else branch which checks p.tok != token.PERIOD directly.

Missing non-classfile leading-dot test (parser/parser_test.go:278): TestErrStaticMember tests whitespace-around-dot in regular .xgo files (A. B, A .B) but does not cover . B in a non-classfile context — precisely the parse path that the new inClassFile() guard changes. Adding a testErrCode case for . B in a .xgo file would directly pin the regression this PR fixes.

Comment thread parser/parser.go
var posErr token.Pos
var ret string
if p.tok == token.PERIOD {
if p.inClassFile() && p.tok == token.PERIOD {

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.

Minor nit: reversing the condition to p.tok == token.PERIOD && p.inClassFile() is slightly more efficient — the cheap token comparison short-circuits for most identifiers (which are not .-prefixed), avoiding the inClassFile() call altogether. It also mirrors the else branch which tests p.tok != token.PERIOD first.

Comment thread parser/parser_test.go
`, `/foo/bar.gox:5:7: missing constant value`, ``)
}

func TestErrStaticMember(t *testing.T) {

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.

The three cases here cover whitespace-around-dot in .xgo, but the non-classfile leading-dot case (e.g. var (\n\t. B int\n)\n through testErrCode) is not tested. That is precisely the path gated by the new inClassFile() guard in parseIdentEx. Adding a testErrCode case for . B would directly pin the regression this PR fixes.

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.

1 participant