Skip to content

refactor(runtime): clear the three Sonar smells left on develop - #1771

Merged
HuiJun merged 1 commit into
developfrom
refactor/sonar-go-smells-0.12.0
Oct 30, 2026
Merged

HuiJun merged 1 commit into
developfrom
refactor/sonar-go-smells-0.12.0

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

What and why

SonarCloud reports three open Go code smells on develop that should be cleared before 0.12.0 is cut:

  • go:S3776 on eachUnwrappedDeclMember in internal/exec/runtime/invoke_calc.go: cognitive complexity 33, the allowed maximum is 30.
  • godre:S8193 twice in internal/frontend/grpc/validate_test.go: a got := variable declared in an if and used only in the condition.

eachUnwrappedDeclMember and unwrappedDeclMembers both opened with the same three-way dispatch — ast.OwnedConstraintOf, then a type switch over *ast.AssumeMember and *ast.RequireMember — and the iterator form repeated the yield loop in each arm. ast.OwnedConstraintOf only answers true for those same two node types and hands back their Body unchanged, so the first check never selected a member list the type switch would not have selected. Both readers now call one helper, constraintMemberBody(decl) ([]ast.Node, bool), which returns the nested members of an assume or require member in either of its forms; the iterator keeps a single yield loop over that body and the existing ast.DeclMembers walk. The sequence of members each reader produces is unchanged for every declaration kind, so no caller, error or error timing moves. The iterator's cognitive complexity drops from 33 to 17.

The two test variables are inlined into their conditions; the failure messages already print the slices, not the joined string.

No changelog fragment: a pure refactor with no user-visible change.

How it was verified

  • go build ./..., go vet ./..., gofmt -l . (empty)
  • go test ./internal/exec/runtime/... ./internal/frontend/grpc/...
  • make test-short

eachUnwrappedDeclMember was just perf-tuned (#1742 removed allocations from the binding walks), so the three benchmarks that walk it were measured on develop (8dc84706) and on this branch, six rounds each, with go test -run '^$' -bench <name> -benchmem -count=6; InstantiateWarmModel was measured in alternating develop/branch rounds because its single-tree runs vary by ±50% on this box. benchstat finds B/op, allocs/op and sec/op unchanged within noise (the new helper allocates nothing and is inlined):

pkg: github.com/Open-MBEE/OpenSysML/internal/exec/runtime
                     │ develop.txt  │          branch.txt           │
                     │    sec/op    │    sec/op     vs base         │
AssignmentLoopStep-8   2.943m ± 20%   3.093m ± 13%  ~ (p=0.485 n=6)

                     │ develop.txt  │          branch.txt           │
                     │     B/op     │     B/op      vs base         │
AssignmentLoopStep-8   926.2Ki ± 0%   926.3Ki ± 0%  ~ (p=0.394 n=6)

                     │ develop.txt │          branch.txt          │
                     │  allocs/op  │  allocs/op   vs base         │
AssignmentLoopStep-8   8.543k ± 0%   8.545k ± 0%  ~ (p=0.537 n=6)

pkg: github.com/Open-MBEE/OpenSysML/internal/frontend/grpc
                       │ develop.txt  │          branch.txt           │
                       │    sec/op    │    sec/op     vs base         │
InstantiateWarmModel-8   363.3µ ± 39%   395.3µ ± 21%  ~ (p=0.818 n=6)

                       │ develop.txt  │          branch.txt           │
                       │     B/op     │     B/op      vs base         │
InstantiateWarmModel-8   126.1Ki ± 3%   125.8Ki ± 6%  ~ (p=0.699 n=6)

                       │ develop.txt │          branch.txt          │
                       │  allocs/op  │  allocs/op   vs base         │
InstantiateWarmModel-8   2.046k ± 1%   2.045k ± 2%  ~ (p=0.818 n=6)

pkg: github.com/Open-MBEE/OpenSysML/tests/perf
                       │ develop.txt  │          branch.txt           │
                       │    sec/op    │    sec/op     vs base         │
GRPCVerifyConstraint-8   3.056m ± 42%   2.719m ± 20%  ~ (p=0.132 n=6)

                       │ develop.txt  │          branch.txt           │
                       │     B/op     │     B/op      vs base         │
GRPCVerifyConstraint-8   766.3Ki ± 0%   766.3Ki ± 0%  ~ (p=0.184 n=6)

                       │ develop.txt │           branch.txt           │
                       │  allocs/op  │  allocs/op   vs base           │
GRPCVerifyConstraint-8   14.89k ± 0%   14.89k ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

Checklist

  • make test and make lint pass locally (make test-short and the race-free package suites; see above)
  • Tests added or updated for the change — none needed: a behaviour-preserving refactor covered by the existing runtime and gRPC suites
  • Documentation extended where it already covers the surface — no documented surface changes
  • Changelog entry added — not needed for a pure refactor with no user-visible change
  • baselines regenerated — no gate count moved
  • No internal work-item labels (waves, slices, F4, K5) in the body, docs, or changelog

eachUnwrappedDeclMember and unwrappedDeclMembers opened with the same three-way dispatch — ast.OwnedConstraintOf, then a type switch over the assume and require members — although the first check only ever answers for those same two node types and returns their Body unchanged. Both now call one helper, constraintMemberBody, so the iterator keeps a single yield loop and its cognitive complexity drops from 33 to 17 (go:S3776). The member sequence each reader yields is unchanged for every declaration kind, and the helper inlines and allocates nothing.

Two `got :=` variables in validate_test.go that were only read in their if condition are inlined (godre:S8193).

Co-Authored-By: jason.han <hanhuijun@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration
devin-ai-integration Bot marked this pull request as ready for review October 30, 2026 16:03

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@HuiJun
HuiJun merged commit b4d0a35 into develop Oct 30, 2026
30 of 51 checks passed
@HuiJun
HuiJun deleted the refactor/sonar-go-smells-0.12.0 branch October 30, 2026 17:34
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