Upgrade Go version to 1.26 and apply go fix#181
Open
snelusha wants to merge 1 commit intoballerina-platform:mainfrom
Open
Upgrade Go version to 1.26 and apply go fix#181snelusha wants to merge 1 commit intoballerina-platform:mainfrom
snelusha wants to merge 1 commit intoballerina-platform:mainfrom
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (66)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs 📝 WalkthroughWalkthroughReplaces many uses of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
semantics/cfg_analyzer.go (1)
34-69:⚠️ Potential issue | 🟠 MajorProtect
panicErrfrom concurrent writes (data race).Multiple goroutines write
panicErrwithout synchronization; under the race detector this is unsafe and can drop panics. Use a buffered channel orsync.Once/atomic.Value.🔒 Example using a buffered channel
- var panicErr any = nil + panicCh := make(chan any, 1) // Run reachability analysis wg.Go(func() { defer func() { if r := recover(); r != nil { - panicErr = r + select { + case panicCh <- r: + default: + } } }() analyzeReachability(ctx, cfg) }) // ... same change in other goroutines ... wg.Wait() - if panicErr != nil { - panic(panicErr) - } + select { + case r := <-panicCh: + panic(r) + default: + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/cfg_analyzer.go` around lines 34 - 69, The goroutines are unsafely writing to the shared variable panicErr (data race) in the defer blocks after calling analyzeReachability, analyzeExplicitReturn, and analyzeUninitializedVars; replace the shared write with a concurrency-safe collector (e.g., create a buffered channel panicCh := make(chan any, 3) and in each defer where you currently set panicErr send the recovered value into panicCh if non-nil), then after wg.Wait read from panicCh (if any) and call panic() with the first received value; this removes concurrent writes to panicErr and preserves panics from analyzeReachability, analyzeExplicitReturn, and analyzeUninitializedVars.desugar/desugar.go (1)
152-163:⚠️ Potential issue | 🔴 CriticalReplace
sync.WaitGroup.Go()with traditionalAdd()/Done()pattern—functions passed toGo()must not panic per Go 1.26 API contract.The Go 1.26 documentation explicitly states that functions passed to
wg.Go(f)must not panic. The current code wraps goroutines withrecover()handlers, which violates this API contract. Additionally, multiple goroutines write to the sharedpanicErrvariable (lines 159 and 177) without synchronization, creating a data race.Use
wg.Add()/wg.Done()instead ofwg.Go():Recommended refactor
var wg sync.WaitGroup var panicErr any var panicMu sync.Mutex desugarFn := func(fn *ast.BLangFunction) { + wg.Add(1) - wg.Go(func() { + go func() { defer func() { if r := recover(); r != nil { + panicMu.Lock() panicErr = r + panicMu.Unlock() } + wg.Done() }() *fn = *desugarFunction(pkgCtx, fn) - }) + }() }Apply the same pattern to the class desugaring closure (line 172-188).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desugar/desugar.go` around lines 152 - 163, The goroutine usage must be changed: replace wg.Go(...) calls with wg.Add(1) followed by a plain goroutine that defers wg.Done() and keeps the existing recover() logic inside the goroutine; additionally protect writes to the shared panicErr variable (e.g., with a sync.Mutex or atomic.Value) so multiple goroutines cannot race when setting panicErr; apply the same change to the class-desugaring closure as well (update the closures around desugarFn / desugarFunction and the class desugaring closure to use wg.Add(1)/defer wg.Done() and synchronized panicErr writes).semtypes/list_definition.go (1)
100-111:⚠️ Potential issue | 🔴 Critical
new(env.listAtom(&atomicType))is invalid Go syntax and will not compile.The built-in
new()function expects a type, not a value expression. Sinceenv.listAtom()returns aTypeAtomvalue (not a type), this code cannot work as written. The statement needs to assign the value to a variable first, then take its address.🔧 Proposed fix
- atom = new(env.listAtom(&atomicType)) + atomVal := env.listAtom(&atomicType) + atom = &atomVal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/list_definition.go` around lines 100 - 111, The code incorrectly uses new(env.listAtom(&atomicType)) — new() needs a type, not an expression; fix ListDefinition.define by first storing the result of env.listAtom(&atomicType) into a local variable (e.g., tmp := env.listAtom(&atomicType)) and then assign atom = &tmp so you take the address of that value (keeping the existing rec branch and env.setRecListAtomType call unchanged).ast/ast.go (1)
1145-1150:⚠️ Potential issue | 🔴 Critical
new(<value>)is invalid inGetReturnParameterDocumentation.
newonly accepts a type; use a local variable and return its address.🔧 Suggested fix
if this.ReturnParameter == nil { return nil } - return new(this.ReturnParameter.GetReturnParameterDocumentation()) + doc := this.ReturnParameter.GetReturnParameterDocumentation() + return &doc }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/ast.go` around lines 1145 - 1150, The function BLangMarkdownDocumentation.GetReturnParameterDocumentation incorrectly uses new(<value>) — new only accepts types; change it to capture the returned string from this.ReturnParameter.GetReturnParameterDocumentation() into a local variable (e.g., s := this.ReturnParameter.GetReturnParameterDocumentation()) and return its address (&s), keeping the existing nil-check for this.ReturnParameter; this fixes the invalid new(...) usage while returning a *string as intended.semtypes/mapping_ops.go (1)
142-156:⚠️ Potential issue | 🔴 CriticalInvalid
new()usage inintersectMappingreturn statement.
new()requires a type argument, not a function call.new(MappingAtomicTypeFrom(...))will not compile. Create a local variable and return its pointer instead.Suggested fix
rest := intersectMemberSemTypes(env, m1.Rest, m2.Rest) - return new(MappingAtomicTypeFrom(names, types, rest)) + mt := MappingAtomicTypeFrom(names, types, rest) + return &mt }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/mapping_ops.go` around lines 142 - 156, The return uses invalid new(...) syntax; instead create a local value from MappingAtomicTypeFrom and return its address. In intersectMapping, after computing names, types and rest, assign the result of MappingAtomicTypeFrom(names, types, rest) to a local variable (e.g., result := MappingAtomicTypeFrom(...)) and then return &result so the function returns *MappingAtomicType correctly.ast/node_builder.go (1)
1048-1116:⚠️ Potential issue | 🔴 CriticalFix invalid
new(...)usages with string and function expressions.These
new(...)calls won't compile. In Go,new()requires a type, not expressions or values. Use local variables and take their addresses instead.🔧 Suggested fix
if isFiniteType { // Remove f, d, and + suffixes value = regexp.MustCompile("[fd+]").ReplaceAllString(textValue, "") - originalValue = new(strings.ReplaceAll(textValue, "+", "")) + orig := strings.ReplaceAll(textValue, "+", "") + originalValue = &orig } else { value = textValue originalValue = &textValue } } else if kind == common.NULL_LITERAL { - originalValue = new("null") + nullStr := "null" + originalValue = &nullStr typeTag = model.TypeTags_NIL bLiteral = &BLangLiteral{}Also fix lines 1617-1618:
- bLVarRef.PkgAlias = new(createIdentifier(nameReference[0].GetPosition(), new(nameReference[0].GetValue()), new(nameReference[0].GetValue()))) + alias := createIdentifier(nameReference[0].GetPosition(), &nameReference[0].GetValue(), &nameReference[0].GetValue()) + bLVarRef.PkgAlias = &alias - bLVarRef.VariableName = new(createIdentifier(nameReference[1].GetPosition(), new(nameReference[1].GetValue()), new(nameReference[1].GetValue()))) + varName := createIdentifier(nameReference[1].GetPosition(), &nameReference[1].GetValue(), &nameReference[1].GetValue()) + bLVarRef.VariableName = &varName🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/node_builder.go` around lines 1048 - 1116, The code uses invalid new(...) expressions (e.g., new(strings.ReplaceAll(...)) and new("null")) which don't compile in Go; replace each new(...) with a local string variable and take its address instead: for the numeric literal branch create a local orig := strings.ReplaceAll(textValue, "+", "") and set originalValue = &orig (so BLangNumericLiteral.OriginalValue gets the dereferenced value as before), and for the NULL_LITERAL case do tmp := "null" and set originalValue = &tmp; update the assignments around originalValue, numericLiteral (BLangNumericLiteral), and any other places using new(...) to use local variables and &var.semtypes/predefined_type.go (1)
55-96:⚠️ Potential issue | 🔴 Critical
new(CreateRecAtom(...))is invalid Go syntax and prevents compilation.
CreateRecAtomis a function returning aRecAtomvalue, not a type. Thenew()builtin only accepts types. Replace these calls with a helper function that creates the atom and returns a pointer.Lines 56 and 95 in semtypes/predefined_type.go need fixing:
- Line 56:
BDD_SUBTYPE_RO = BddAtom(new(CreateRecAtom(BDD_REC_ATOM_READONLY)))- Line 95:
OBJECT_RO_REC_ATOM = new(CreateRecAtom(BDD_REC_ATOM_OBJECT_READONLY))Suggested fix
Add this helper function to semtypes/predefined_type.go (outside the var block):
func recAtomPtr(index int) *RecAtom { ra := CreateRecAtom(index) return &ra }Then update the initializations:
BDD_SUBTYPE_RO = BddAtom(recAtomPtr(BDD_REC_ATOM_READONLY)) OBJECT_RO_REC_ATOM = recAtomPtr(BDD_REC_ATOM_OBJECT_READONLY)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/predefined_type.go` around lines 55 - 96, The code uses invalid new(CreateRecAtom(...)) calls (CreateRecAtom returns a value, not a type); add a helper like recAtomPtr(index int) *RecAtom that calls CreateRecAtom and returns its address, then replace BDD_SUBTYPE_RO = BddAtom(new(CreateRecAtom(BDD_REC_ATOM_READONLY))) with BDD_SUBTYPE_RO = BddAtom(recAtomPtr(BDD_REC_ATOM_READONLY)) and replace OBJECT_RO_REC_ATOM = new(CreateRecAtom(BDD_REC_ATOM_OBJECT_READONLY)) with OBJECT_RO_REC_ATOM = recAtomPtr(BDD_REC_ATOM_OBJECT_READONLY)).semtypes/core.go (1)
44-104:⚠️ Potential issue | 🔴 Critical
new(...)with function calls is invalid syntax — code will not compile.In Go,
new()requires a type, not a value expression. SinceBasicTypeBitSetFrom()andbasicTypeUnion()returnBasicTypeBitSetvalues (not types),new(BasicTypeBitSetFrom(...))and similar patterns are syntax errors.For all affected returns (lines 48, 65, 80, 103, 143, 167, 190, 200, 237, 257, 942, 944), store the computed value in a local variable and return its address:
🔧 Suggested fix pattern
- return new(BasicTypeBitSetFrom(b1.bitset & ^b2.bitset)) + v := BasicTypeBitSetFrom(b1.bitset & ^b2.bitset) + return &vor
- return new(basicTypeUnion(all.bitset)) + u := basicTypeUnion(all.bitset) + return &uor for non-function returns:
- return new(all) + allCopy := all + return &allCopy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/core.go` around lines 44 - 104, The code incorrectly uses new(...) with function calls (e.g., new(BasicTypeBitSetFrom(...)), new(basicTypeUnion(...)), new(all)), which is invalid in Go because new expects a type, not a value; replace each occurrence by first assigning the function result to a local variable (e.g., v := BasicTypeBitSetFrom(...), u := basicTypeUnion(...), a := all) and then return its address (&v, &u, &a) or return the value as appropriate; update all spots referencing BasicTypeBitSetFrom, basicTypeUnion, and the plain all/new(all) returns in Diff (and other affected functions mentioned in the review) to follow this pattern so the code compiles.parser/parser.go (1)
262-278:⚠️ Potential issue | 🟡 MinorUnpack variadic diagnostics args with
args...when forwarding.The functions receive
argsas a variadic parameter (args ...any) and must unpack it when calling Clone helpers that also accept variadic arguments. Currently, the slice is passed as a single argument, wrapping the diagnostics incorrectly. Fix all three instances:
- Line 265:
tree.CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode, args...)- Line 270:
this.updateANodeInListWithLeadingInvalidNode(nodeList, 0, invalidParam, diagnosticCode, args...)- Line 277:
tree.CloneWithLeadingInvalidNodeMinutiae(node, invalidParam, diagnosticCode, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parser/parser.go` around lines 262 - 278, The variadic diagnostics args are being forwarded as a single slice instead of unpacked; update the three call sites to pass args... rather than args so the variadic parameters are forwarded correctly: in updateLastNodeInListWithInvalidNode change the call to CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode, args...), in updateFirstNodeInListWithLeadingInvalidNode forward args... when calling updateANodeInListWithLeadingInvalidNode(nodeList, 0, invalidParam, diagnosticCode, args...), and in updateANodeInListWithLeadingInvalidNode change the call to CloneWithLeadingInvalidNodeMinutiae(node, invalidParam, diagnosticCode, args...).test_util/assert.go (1)
228-233:⚠️ Potential issue | 🟡 MinorSilent handling of non-string single argument.
When a single non-string argument is passed (e.g., an integer or struct), the function silently returns an empty string (lines 231-232). This may cause confusion when a test fails without showing the intended message. Consider using
fmt.Sprintto convert any single value to a string.🛡️ Proposed fix
if len(msgAndArgs) == 1 { if msg, ok := msgAndArgs[0].(string); ok { return msg } - return "" + return fmt.Sprint(msgAndArgs[0]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/assert.go` around lines 228 - 233, The current branch handling msgAndArgs when len(msgAndArgs) == 1 silently returns "" for non-string values; update that branch to return fmt.Sprint(msgAndArgs[0]) for any single argument (keeping the existing string check) so integers/structs are converted to a readable message, and add the fmt import if missing; refer to the msgAndArgs handling block in test_util/assert.go to make this change.
🧹 Nitpick comments (6)
projects/build_project_test.go (1)
311-327: Consider refactoring remaining manual loops for consistency.This test uses manual for-loops to check
foundInOldandfoundInNew, but line 331 usesslices.ContainsFuncfor a similar check. The same inconsistency appears inTestAddTestDocument(lines 388-404) andTestRemoveDocument(lines 459-476).♻️ Proposed refactor for consistency
- foundInOld := false - for _, docID := range oldModule.DocumentIDs() { - if docID.Equals(newDocumentID) { - foundInOld = true - break - } - } - assert.False(foundInOld) - - foundInNew := false - for _, docID := range newModule.DocumentIDs() { - if docID.Equals(newDocumentID) { - foundInNew = true - break - } - } - assert.True(foundInNew) + foundInOld := slices.ContainsFunc(oldModule.DocumentIDs(), newDocumentID.Equals) + assert.False(foundInOld) + + foundInNew := slices.ContainsFunc(newModule.DocumentIDs(), newDocumentID.Equals) + assert.True(foundInNew)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/build_project_test.go` around lines 311 - 327, The test currently uses manual loops (foundInOld/foundInNew) to check if newDocumentID exists in oldModule.DocumentIDs() and newModule.DocumentIDs(); replace those loops with slices.ContainsFunc(oldModule.DocumentIDs(), func(id whateverType) bool { return id.Equals(newDocumentID) }) and similarly for newModule to match the existing slices.ContainsFunc usage (also apply the same refactor in TestAddTestDocument and TestRemoveDocument where similar manual loops exist), referencing the DocumentIDs() calls and newDocumentID/Equals methods to locate the checks.test_util/require.go (3)
153-158: Same issue:msgAndArgsignored inNoError.Custom messages are silently discarded on line 156.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/require.go` around lines 153 - 158, The NoError method in type Require currently ignores the variadic msgAndArgs parameter; update Require.NoError to include msgAndArgs in the failure call so custom messages are not discarded—locate the NoError function and change the r.t.Fatalf call to use the provided msgAndArgs (e.g., include them in the format string or pass them through to Fatalf after the error text) so the custom message is printed alongside "expected no error but got: %v" when err != nil.
113-127: Same issue:msgAndArgsignored inLen.Similar to
Equal, custom messages are silently discarded on line 125.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/require.go` around lines 113 - 127, The Require.Len method currently ignores the optional msgAndArgs when reporting mismatched lengths; update the failure path in Require.Len so the call to r.t.Fatalf includes the provided msgAndArgs (same pattern used in Equal) and composes the default "expected length %d but got %d" message with any custom message/args so user-supplied text is shown on failure.
71-76: Consider usingfailNowfor consistency.The
msgAndArgsparameter is accepted but not used — the method directly callsr.t.Fatalfwith a hardcoded format string. This differs from other methods likeNotEqualwhich delegate tofailNow(defaultMsg, msgAndArgs...). Custom messages passed by callers are silently ignored.♻️ Suggested fix for consistency
func (r *Require) Equal(expected, actual any, msgAndArgs ...any) { r.t.Helper() if !reflect.DeepEqual(expected, actual) { - r.t.Fatalf("expected %v but got %v", expected, actual) + if len(msgAndArgs) > 0 { + r.t.Fatal(formatMessage(msgAndArgs...)) + } else { + r.t.Fatalf("expected %v but got %v", expected, actual) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/require.go` around lines 71 - 76, The Equal method on Require ignores msgAndArgs and directly calls r.t.Fatalf with a hardcoded message; update Require.Equal to build a default message like "expected %v but got %v" and call failNow(defaultMsg, msgAndArgs...) (same approach used by NotEqual) so custom messages are honored — locate the Require.Equal function and replace the direct r.t.Fatalf(...) invocation with failNow(defaultMsg, msgAndArgs...) while keeping the same expected/actual format.test_util/assert.go (2)
118-133: Consider usingstrings.Containsand revisiting empty substring semantics.The manual substring search could be replaced with
strings.Contains. Additionally, the empty substring check on line 120 may be overly restrictive—an empty substring is technically contained in any string.♻️ Suggested simplification
+import "strings" + // Contains asserts that the string contains the substring. func (a *Assert) Contains(s, substr string, msgAndArgs ...any) { a.t.Helper() - if len(s) == 0 || len(substr) == 0 { - a.fail("expected string to contain substring", msgAndArgs...) - return - } - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return - } - } - a.t.Errorf("expected %q to contain %q", s, substr) - if len(msgAndArgs) > 0 { - a.t.Log(formatMessage(msgAndArgs...)) + if !strings.Contains(s, substr) { + a.t.Errorf("expected %q to contain %q", s, substr) + if len(msgAndArgs) > 0 { + a.t.Log(formatMessage(msgAndArgs...)) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/assert.go` around lines 118 - 133, Replace the manual substring loop in Assert.Contains with strings.Contains and adjust the empty-substring handling so an empty substr is considered contained (i.e., do not call a.fail when substr == ""); change the body of Assert.Contains to use if strings.Contains(s, substr) { return } and only fail/log when that returns false; also add/import "strings" and remove the explicit length checks that currently call a.fail for empty inputs (keep the check that s=="" with non-empty substr should fail).
162-176: Consider reusing length-checking logic.
NotEmptyduplicates thereflect.ValueOfandswitchlogic fromLen. This could be simplified to reduce duplication.♻️ Suggested simplification
// NotEmpty asserts that the slice/map/string is not empty. func (a *Assert) NotEmpty(object any, msgAndArgs ...any) { a.t.Helper() - v := reflect.ValueOf(object) - var length int - switch v.Kind() { - case reflect.Slice, reflect.Array, reflect.Map, reflect.String, reflect.Chan: - length = v.Len() - default: - a.fail("cannot get length of non-collection type", msgAndArgs...) - return - } - if length == 0 { + length, ok := getLength(object) + if !ok { + a.fail("cannot get length of non-collection type", msgAndArgs...) + return + } + if length == 0 { a.fail("expected non-empty but got empty", msgAndArgs...) } } + +func getLength(object any) (int, bool) { + v := reflect.ValueOf(object) + switch v.Kind() { + case reflect.Slice, reflect.Array, reflect.Map, reflect.String, reflect.Chan: + return v.Len(), true + default: + return 0, false + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_util/assert.go` around lines 162 - 176, NotEmpty duplicates reflect.ValueOf + kind switch used by Len; change NotEmpty to call the existing Len helper (e.g., use a.Len(object, msgAndArgs...) to obtain the length) and then only check if that length == 0 to call a.fail("expected non-empty but got empty", msgAndArgs...), leaving type-validation and error reporting to Len; keep using the same a.fail message for consistency and retain a.t.Helper() in NotEmpty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast/node_builder.go`:
- Around line 1614-1619: The code in createBLangNameReference usage incorrectly
calls new(createIdentifier(...)) which is invalid in Go; replace those with
local identifier variables (e.g., pkgIdent := createIdentifier(...), varIdent :=
createIdentifier(...)) and assign their addresses to bLVarRef.PkgAlias and
bLVarRef.VariableName (e.g., bLVarRef.PkgAlias = &pkgIdent) while keeping the
existing getPosition(actionOrExpression),
createBLangNameReference(actionOrExpression), and BLangSimpleVarRef fields
intact to preserve behavior.
In `@semtypes/bdd_test.go`:
- Around line 27-28: The code uses new(CreateRecAtom(...)) which is invalid
because new expects a type; since RecAtom methods have pointer receivers and
only *RecAtom implements Atom, create a local RecAtom value via CreateRecAtom
(e.g., tmp := CreateRecAtom(1)) and pass its address to BddAtom (e.g.,
BddAtom(&tmp)); do the same for the second instance. Update the lines that
construct b1 and b2 to take the address of a temp variable returned by
CreateRecAtom instead of using new().
In `@semtypes/cell_ops.go`:
- Line 39: The call new(cellAtomType(p.Atom)) is invalid because new expects a
type, not a value; instead construct a pointer to a cellAtomType literal and
pass that to IntersectCellAtomicType. Replace the expression with an
address-of-literal such as &cellAtomType{Atom: p.Atom} (or &cellAtomType{p.Atom}
if it’s a single-field struct) when calling IntersectCellAtomicType(&combined,
...), referencing the existing identifiers combined, IntersectCellAtomicType,
cellAtomType, and p.Atom.
In `@semtypes/decimal_ops.go`:
- Around line 34-40: The Union implementation uses invalid
new(t1.(DecimalSubtype)) syntax; change to assert into a local variable and take
its address (e.g., tmp1 := t1.(DecimalSubtype); v1 :=
EnumerableSubtype[big.Rat](&tmp1)) so you pass a pointer to EnumerableSubtype;
do the same for t2 and use those v1/v2 with EnumerableSubtypeUnion and
CreateDecimalSubtype in DecimalOps.Union. Apply the identical fix in
semtypes/float_ops.go for the Union and Intersect functions where new(...) is
used (assert into locals and take their addresses before converting to
EnumerableSubtype).
In `@semtypes/enumerable_char_string.go`:
- Around line 48-51: The function EnumerableCharStringFrom currently calls
new(newEnumerableCharStringFromString(v)) which is invalid because new() expects
a type, not an expression; replace that with a direct return of the constructed
value (or its address if the interface requires a pointer). Concretely, update
EnumerableCharStringFrom to return newEnumerableCharStringFromString(v) (or
return &newEnumerableCharStringFromString(v) only if EnumerableType[string] is
implemented on *EnumerableCharString) and remove the incorrect new(...) wrapper.
In `@semtypes/enumerable_string.go`:
- Around line 47-49: EnumerableStringFrom uses the built-in new() incorrectly by
passing a function call; make the result addressable first: assign the output of
newEnumerableStringFromString(v) to a local variable (e.g. tmp :=
newEnumerableStringFromString(v)) and then pass its address to new (e.g. return
new(&tmp)). Update EnumerableStringFrom to use this pattern and apply the same
fix to the identical site in enumerable_char_string.go (around the function that
mirrors EnumerableStringFrom).
In `@semtypes/float_ops.go`:
- Around line 30-36: The code uses invalid Go syntax new(t1.(FloatSubtype)); fix
FloatOps.Union (and similarly Intersect) by first performing a type assertion
into a local concrete variable (e.g., fs1 := t1.(FloatSubtype), fs2 :=
t2.(FloatSubtype)), then create the EnumerableSubtype instances by taking
addresses of those locals (e.g., v1 := EnumerableSubtype[float64](&fs1), v2 :=
EnumerableSubtype[float64](&fs2)), call
EnumerableSubtypeUnion/EnumerableSubtypeIntersect with those v1/v2 and the
values slice, and finally return CreateFloatSubtype(allowed, values).
In `@semtypes/list_proj.go`:
- Around line 49-52: The code uses invalid new(And(...)) calls; replace each
new(And(...)) with a pointer to a Conjunction value by creating a local variable
and taking its address (e.g. conj := And(bddNode.Atom(), pos); pass &conj) or
construct the struct pointer directly if appropriate; update both places in
listProjBddInnerVal where new(And(...)) appears (the left and the middle/right
branches) so the Union calls receive a *Conjunction (matching the expected type)
instead of the invalid new(...) expression.
In `@semtypes/mapping_definition.go`:
- Line 71: The code incorrectly uses new() with a function call; replace the
invalid expression new(env.mappingAtom(&atomicType)) with a direct call to
env.mappingAtom(&atomicType) (i.e., atom = env.mappingAtom(&atomicType)) so you
call the constructor/factory instead of trying to use new with a call; reference
mappingAtom, atomicType and env to locate the assignment.
In `@semtypes/object_definition.go`:
- Line 143: The code incorrectly uses new(MemberKindMethod).field() where
MemberKindMethod is a constant (not a type); replace this with a properly typed
value and call field() on that value — for example construct the MemberKind
value from the constant (e.g. MemberKind(MemberKindMethod).field()) or otherwise
obtain a MemberKind instance before calling .field() so you are calling field()
on a value of the correct named type instead of using new(...) on a constant.
- Line 131: The code uses new(MemberKindField).field(), but MemberKindField is a
constant of type MemberKind so new() is invalid; replace this with a direct call
on the constant if the method has a value receiver (MemberKindField.field()), or
if the field() method requires a pointer receiver create a local variable (e.g.
mk := MemberKindField; (&mk).field()) and call the method on that pointer;
update the expression where new(MemberKindField).field() appears accordingly.
- Line 42: Remove the invalid new() call and pass a pointer to a RecAtom to
BddAtom: call CreateDistinctRecAtom(-distinctId - 1), assign its returned
RecAtom value to a local variable (e.g., rec := CreateDistinctRecAtom(...)) and
then construct the BddAtom with the address (BddAtom(&rec)); this ensures you
provide a *RecAtom (which implements Atom) instead of trying to use new on a
function call.
In `@semtypes/string_subtype.go`:
- Around line 71-78: slices.Contains is comparing interface pointer identity for
EnumerableCharStringFrom(s) and EnumerableStringFrom(s), so replace those calls
with value-based checks: use slices.ContainsFunc (or an explicit loop) on
chara.Values() and nonChar.Values(), providing a comparator that invokes
Compare() between each existing element and the created charString/stringString
to determine equality; update the checks around EnumerableCharStringFrom(s),
EnumerableStringFrom(s), chara.Values(), nonChar.Values(), and the variables
charString/stringString accordingly so the logic uses Compare() rather than ==.
In `@semtypes/xml_ops.go`:
- Line 22: The code uses invalid Go syntax `new(CreateXMLRecAtom(...))`;
instead, assign the returned RecAtom to a temporary variable and pass its
address to BddAtom. Replace the call inside XML_SUBTYPE_RO with something like
tmp := CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON) and then use
XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(&tmp)) so the address-of operator
is applied to the value returned by CreateXMLRecAtom.
In `@semtypes/xml_subtype.go`:
- Around line 83-86: The call new(CreateXMLRecAtom(atom)) is invalid because new
expects a type; instead construct the RecAtom value from CreateXMLRecAtom and
pass its address to BddAtom — i.e., call CreateXMLRecAtom(atom) to get a value,
take its address, then pass that pointer into BddAtom before unioning with
d.Sequence and returning CreateXmlSubtype(primitives, sequence); update the
expression around BddAtom(new(...)) in the sequence construction to use the
address of the created RecAtom value.
---
Outside diff comments:
In `@ast/ast.go`:
- Around line 1145-1150: The function
BLangMarkdownDocumentation.GetReturnParameterDocumentation incorrectly uses
new(<value>) — new only accepts types; change it to capture the returned string
from this.ReturnParameter.GetReturnParameterDocumentation() into a local
variable (e.g., s := this.ReturnParameter.GetReturnParameterDocumentation()) and
return its address (&s), keeping the existing nil-check for
this.ReturnParameter; this fixes the invalid new(...) usage while returning a
*string as intended.
In `@ast/node_builder.go`:
- Around line 1048-1116: The code uses invalid new(...) expressions (e.g.,
new(strings.ReplaceAll(...)) and new("null")) which don't compile in Go; replace
each new(...) with a local string variable and take its address instead: for the
numeric literal branch create a local orig := strings.ReplaceAll(textValue, "+",
"") and set originalValue = &orig (so BLangNumericLiteral.OriginalValue gets the
dereferenced value as before), and for the NULL_LITERAL case do tmp := "null"
and set originalValue = &tmp; update the assignments around originalValue,
numericLiteral (BLangNumericLiteral), and any other places using new(...) to use
local variables and &var.
In `@desugar/desugar.go`:
- Around line 152-163: The goroutine usage must be changed: replace wg.Go(...)
calls with wg.Add(1) followed by a plain goroutine that defers wg.Done() and
keeps the existing recover() logic inside the goroutine; additionally protect
writes to the shared panicErr variable (e.g., with a sync.Mutex or atomic.Value)
so multiple goroutines cannot race when setting panicErr; apply the same change
to the class-desugaring closure as well (update the closures around desugarFn /
desugarFunction and the class desugaring closure to use wg.Add(1)/defer
wg.Done() and synchronized panicErr writes).
In `@parser/parser.go`:
- Around line 262-278: The variadic diagnostics args are being forwarded as a
single slice instead of unpacked; update the three call sites to pass args...
rather than args so the variadic parameters are forwarded correctly: in
updateLastNodeInListWithInvalidNode change the call to
CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode,
args...), in updateFirstNodeInListWithLeadingInvalidNode forward args... when
calling updateANodeInListWithLeadingInvalidNode(nodeList, 0, invalidParam,
diagnosticCode, args...), and in updateANodeInListWithLeadingInvalidNode change
the call to CloneWithLeadingInvalidNodeMinutiae(node, invalidParam,
diagnosticCode, args...).
In `@semantics/cfg_analyzer.go`:
- Around line 34-69: The goroutines are unsafely writing to the shared variable
panicErr (data race) in the defer blocks after calling analyzeReachability,
analyzeExplicitReturn, and analyzeUninitializedVars; replace the shared write
with a concurrency-safe collector (e.g., create a buffered channel panicCh :=
make(chan any, 3) and in each defer where you currently set panicErr send the
recovered value into panicCh if non-nil), then after wg.Wait read from panicCh
(if any) and call panic() with the first received value; this removes concurrent
writes to panicErr and preserves panics from analyzeReachability,
analyzeExplicitReturn, and analyzeUninitializedVars.
In `@semtypes/core.go`:
- Around line 44-104: The code incorrectly uses new(...) with function calls
(e.g., new(BasicTypeBitSetFrom(...)), new(basicTypeUnion(...)), new(all)), which
is invalid in Go because new expects a type, not a value; replace each
occurrence by first assigning the function result to a local variable (e.g., v
:= BasicTypeBitSetFrom(...), u := basicTypeUnion(...), a := all) and then return
its address (&v, &u, &a) or return the value as appropriate; update all spots
referencing BasicTypeBitSetFrom, basicTypeUnion, and the plain all/new(all)
returns in Diff (and other affected functions mentioned in the review) to follow
this pattern so the code compiles.
In `@semtypes/list_definition.go`:
- Around line 100-111: The code incorrectly uses new(env.listAtom(&atomicType))
— new() needs a type, not an expression; fix ListDefinition.define by first
storing the result of env.listAtom(&atomicType) into a local variable (e.g., tmp
:= env.listAtom(&atomicType)) and then assign atom = &tmp so you take the
address of that value (keeping the existing rec branch and
env.setRecListAtomType call unchanged).
In `@semtypes/mapping_ops.go`:
- Around line 142-156: The return uses invalid new(...) syntax; instead create a
local value from MappingAtomicTypeFrom and return its address. In
intersectMapping, after computing names, types and rest, assign the result of
MappingAtomicTypeFrom(names, types, rest) to a local variable (e.g., result :=
MappingAtomicTypeFrom(...)) and then return &result so the function returns
*MappingAtomicType correctly.
In `@semtypes/predefined_type.go`:
- Around line 55-96: The code uses invalid new(CreateRecAtom(...)) calls
(CreateRecAtom returns a value, not a type); add a helper like recAtomPtr(index
int) *RecAtom that calls CreateRecAtom and returns its address, then replace
BDD_SUBTYPE_RO = BddAtom(new(CreateRecAtom(BDD_REC_ATOM_READONLY))) with
BDD_SUBTYPE_RO = BddAtom(recAtomPtr(BDD_REC_ATOM_READONLY)) and replace
OBJECT_RO_REC_ATOM = new(CreateRecAtom(BDD_REC_ATOM_OBJECT_READONLY)) with
OBJECT_RO_REC_ATOM = recAtomPtr(BDD_REC_ATOM_OBJECT_READONLY)).
In `@test_util/assert.go`:
- Around line 228-233: The current branch handling msgAndArgs when
len(msgAndArgs) == 1 silently returns "" for non-string values; update that
branch to return fmt.Sprint(msgAndArgs[0]) for any single argument (keeping the
existing string check) so integers/structs are converted to a readable message,
and add the fmt import if missing; refer to the msgAndArgs handling block in
test_util/assert.go to make this change.
---
Duplicate comments:
In `@desugar/desugar.go`:
- Around line 171-187: The panic capture in the goroutine for desugaring class
definitions (the anonymous func capturing class and setting panicErr) needs the
same panicOnce.Do protection used elsewhere: wrap the panic assignment that sets
panicErr inside panicOnce.Do(...) to ensure only the first panic is recorded and
avoid races; update the goroutine in the loop that iterates over
pkg.ClassDefinitions (the anonymous function passed to wg.Go) so the deferred
recover block calls panicOnce.Do to set panicErr instead of assigning directly.
---
Nitpick comments:
In `@projects/build_project_test.go`:
- Around line 311-327: The test currently uses manual loops
(foundInOld/foundInNew) to check if newDocumentID exists in
oldModule.DocumentIDs() and newModule.DocumentIDs(); replace those loops with
slices.ContainsFunc(oldModule.DocumentIDs(), func(id whateverType) bool { return
id.Equals(newDocumentID) }) and similarly for newModule to match the existing
slices.ContainsFunc usage (also apply the same refactor in TestAddTestDocument
and TestRemoveDocument where similar manual loops exist), referencing the
DocumentIDs() calls and newDocumentID/Equals methods to locate the checks.
In `@test_util/assert.go`:
- Around line 118-133: Replace the manual substring loop in Assert.Contains with
strings.Contains and adjust the empty-substring handling so an empty substr is
considered contained (i.e., do not call a.fail when substr == ""); change the
body of Assert.Contains to use if strings.Contains(s, substr) { return } and
only fail/log when that returns false; also add/import "strings" and remove the
explicit length checks that currently call a.fail for empty inputs (keep the
check that s=="" with non-empty substr should fail).
- Around line 162-176: NotEmpty duplicates reflect.ValueOf + kind switch used by
Len; change NotEmpty to call the existing Len helper (e.g., use a.Len(object,
msgAndArgs...) to obtain the length) and then only check if that length == 0 to
call a.fail("expected non-empty but got empty", msgAndArgs...), leaving
type-validation and error reporting to Len; keep using the same a.fail message
for consistency and retain a.t.Helper() in NotEmpty.
In `@test_util/require.go`:
- Around line 153-158: The NoError method in type Require currently ignores the
variadic msgAndArgs parameter; update Require.NoError to include msgAndArgs in
the failure call so custom messages are not discarded—locate the NoError
function and change the r.t.Fatalf call to use the provided msgAndArgs (e.g.,
include them in the format string or pass them through to Fatalf after the error
text) so the custom message is printed alongside "expected no error but got: %v"
when err != nil.
- Around line 113-127: The Require.Len method currently ignores the optional
msgAndArgs when reporting mismatched lengths; update the failure path in
Require.Len so the call to r.t.Fatalf includes the provided msgAndArgs (same
pattern used in Equal) and composes the default "expected length %d but got %d"
message with any custom message/args so user-supplied text is shown on failure.
- Around line 71-76: The Equal method on Require ignores msgAndArgs and directly
calls r.t.Fatalf with a hardcoded message; update Require.Equal to build a
default message like "expected %v but got %v" and call failNow(defaultMsg,
msgAndArgs...) (same approach used by NotEqual) so custom messages are honored —
locate the Require.Equal function and replace the direct r.t.Fatalf(...)
invocation with failNow(defaultMsg, msgAndArgs...) while keeping the same
expected/actual format.
| []Field{ | ||
| FieldFrom("value", fieldValueTy, immutable, false), | ||
| common.ToPointer(MemberKindField).field(), | ||
| new(MemberKindField).field(), |
There was a problem hiding this comment.
Invalid Go syntax: new() cannot take a constant as argument.
From the relevant code snippet, MemberKindField is a constant (MemberKindField MemberKind = iota), not a type. The new() builtin requires a type argument.
🐛 Proposed fix
- new(MemberKindField).field(),
+ (&MemberKindField).field(),Or if MemberKindField needs to be copied first:
- new(MemberKindField).field(),
+ func() Field { k := MemberKindField; return (&k).field() }(),#!/bin/bash
# Verify the type of MemberKindField
rg -n 'MemberKindField' --type=go -A2 -B2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/object_definition.go` at line 131, The code uses
new(MemberKindField).field(), but MemberKindField is a constant of type
MemberKind so new() is invalid; replace this with a direct call on the constant
if the method has a value receiver (MemberKindField.field()), or if the field()
method requires a pointer receiver create a local variable (e.g. mk :=
MemberKindField; (&mk).field()) and call the method on that pointer; update the
expression where new(MemberKindField).field() appears accordingly.
| []Field{ | ||
| FieldFrom("value", &FUNCTION, true, false), | ||
| common.ToPointer(MemberKindMethod).field(), | ||
| new(MemberKindMethod).field(), |
There was a problem hiding this comment.
Same issue: new(MemberKindMethod) is invalid.
MemberKindMethod is also a constant, not a type. This has the same issue as line 131.
🐛 Proposed fix
- new(MemberKindMethod).field(),
+ (&MemberKindMethod).field(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| new(MemberKindMethod).field(), | |
| (&MemberKindMethod).field(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/object_definition.go` at line 143, The code incorrectly uses
new(MemberKindMethod).field() where MemberKindMethod is a constant (not a type);
replace this with a properly typed value and call field() on that value — for
example construct the MemberKind value from the constant (e.g.
MemberKind(MemberKindMethod).field()) or otherwise obtain a MemberKind instance
before calling .field() so you are calling field() on a value of the correct
named type instead of using new(...) on a constant.
Comment on lines
+71
to
+78
| if slices.Contains(chara.Values(), charString) { | ||
| return chara.Allowed() | ||
| } | ||
| return !nonChar.Allowed() | ||
| } | ||
| stringString := EnumerableStringFrom(s) | ||
| for _, value := range nonChar.Values() { | ||
| if value == stringString { | ||
| return nonChar.Allowed() | ||
| } | ||
| if slices.Contains(nonChar.Values(), stringString) { | ||
| return nonChar.Allowed() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the EnumerableType definition
rg -n 'type EnumerableType' --type=go -A 10Repository: ballerina-platform/ballerina-lang-go
Length of output: 293
🏁 Script executed:
# Check the full context of the reported lines in string_subtype.go
cat -n semtypes/string_subtype.go | sed -n '60,85p'Repository: ballerina-platform/ballerina-lang-go
Length of output: 1071
🏁 Script executed:
# Search for Compare methods on EnumerableType
rg -n 'func.*Compare.*EnumerableType' --type=go -A 5Repository: ballerina-platform/ballerina-lang-go
Length of output: 1893
🏁 Script executed:
# Check what Values() returns for EnumerableType
rg -n 'Values\(\)' semtypes/ --type=go -A 3 | head -40Repository: ballerina-platform/ballerina-lang-go
Length of output: 2172
🏁 Script executed:
# Check the migration comments and look for what the old code did
rg -n 'migrated from' semtypes/string_subtype.go -B 2 -A 10Repository: ballerina-platform/ballerina-lang-go
Length of output: 2959
🏁 Script executed:
# Search for any iteration-based comparison in the file
rg -n 'for.*range.*Values' semtypes/string_subtype.go -B 2 -A 5Repository: ballerina-platform/ballerina-lang-go
Length of output: 62
🏁 Script executed:
# Check what types are returned by Values() methods
rg -n 'func.*Values\(\)' semtypes/ --type=go -A 3 | grep -A 3 'CharStringSubtype\|NonCharStringSubtype'Repository: ballerina-platform/ballerina-lang-go
Length of output: 657
🏁 Script executed:
# Look for any tests that might show the expected behavior
rg -n 'StringSubtypeContains' --type=go -lRepository: ballerina-platform/ballerina-lang-go
Length of output: 123
🏁 Script executed:
# Check the test file for StringSubtypeContains to see expected behavior
cat semtypes/string_subtype.go | grep -n 'func.*Test' || echo "No tests in this file"Repository: ballerina-platform/ballerina-lang-go
Length of output: 101
🏁 Script executed:
# Look for unit tests related to StringSubtype
fd -e _test.go | xargs rg -l 'StringSubtypeContains' 2>/dev/nullRepository: ballerina-platform/ballerina-lang-go
Length of output: 123
🏁 Script executed:
# Check the Java source to understand the original comparison logic
rg -n 'StringSubtype.java' --type=go -B 5 -A 15 | head -80Repository: ballerina-platform/ballerina-lang-go
Length of output: 4956
🏁 Script executed:
# Verify the actual issue: check if slices.Contains would work with interface types
python3 << 'EOF'
# In Go, slices.Contains uses == operator on interface types
# Interface == comparison checks if both have the same runtime type AND same pointer value
# This means two different *EnumerableCharString instances with same value will NOT be equal
print("Analysis of the issue:")
print("=" * 60)
print("1. slices.Contains(slice, elem) uses == operator")
print("2. For interface types, == compares (type, pointer)")
print("3. Two different *EnumerableCharString instances with identical values:")
print(" - Have the SAME type: *EnumerableCharString")
print(" - Have DIFFERENT pointers (different memory addresses)")
print(" - Result: NOT equal (== returns false)")
print("")
print("4. This breaks the containment check if the instance in the slice")
print(" is not the EXACT SAME pointer object")
print("")
print("Conclusion: The review comment is CORRECT.")
print("The code will fail if it tries to find an element that has")
print("the same value but is a different instance.")
EOFRepository: ballerina-platform/ballerina-lang-go
Length of output: 737
slices.Contains on interface types compares pointer identity, not value equality—newly created instances will never match existing ones in the slice.
slices.Contains uses the == operator for interface types, which compares both the runtime type AND the pointer address. Lines 70 and 76 create new instances via EnumerableCharStringFrom(s) and EnumerableStringFrom(s), which will have different pointer addresses than any existing instances in the slice. Even if the underlying values are identical, slices.Contains will return false because the pointers differ.
Replace with slices.ContainsFunc using the existing Compare() method, or iterate through the slice calling Compare() directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/string_subtype.go` around lines 71 - 78, slices.Contains is
comparing interface pointer identity for EnumerableCharStringFrom(s) and
EnumerableStringFrom(s), so replace those calls with value-based checks: use
slices.ContainsFunc (or an explicit loop) on chara.Values() and
nonChar.Values(), providing a comparator that invokes Compare() between each
existing element and the created charString/stringString to determine equality;
update the checks around EnumerableCharStringFrom(s), EnumerableStringFrom(s),
chara.Values(), nonChar.Values(), and the variables charString/stringString
accordingly so the logic uses Compare() rather than ==.
| } | ||
|
|
||
| var XML_SUBTYPE_RO = XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(common.ToPointer(CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON)))) | ||
| var XML_SUBTYPE_RO = XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(new(CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON)))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the file
fd xml_ops.go | head -5Repository: ballerina-platform/ballerina-lang-go
Length of output: 99
🏁 Script executed:
# Get file info and read around line 22
wc -l semtypes/xml_ops.go
head -30 semtypes/xml_ops.go | cat -nRepository: ballerina-platform/ballerina-lang-go
Length of output: 1282
🏁 Script executed:
# Search for CreateXMLRecAtom definition
rg "CreateXMLRecAtom" -A 3 -B 1Repository: ballerina-platform/ballerina-lang-go
Length of output: 939
🏁 Script executed:
# Also search for BddAtom to understand what it expects
rg "func BddAtom" -A 5Repository: ballerina-platform/ballerina-lang-go
Length of output: 421
new(CreateXMLRecAtom(...)) is invalid Go syntax.
new only accepts a type, not a function call. CreateXMLRecAtom() returns a RecAtom value, not a type. Use a temporary variable and take its address instead.
Suggested fix
-var XML_SUBTYPE_RO = XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(new(CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON))))
+var XML_SUBTYPE_RO = func() XmlSubtype {
+ rec := CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON)
+ return XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(&rec))
+}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var XML_SUBTYPE_RO = XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(new(CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON)))) | |
| var XML_SUBTYPE_RO = func() XmlSubtype { | |
| rec := CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON) | |
| return XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(&rec)) | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/xml_ops.go` at line 22, The code uses invalid Go syntax
`new(CreateXMLRecAtom(...))`; instead, assign the returned RecAtom to a
temporary variable and pass its address to BddAtom. Replace the call inside
XML_SUBTYPE_RO with something like tmp :=
CreateXMLRecAtom(XML_PRIMITIVE_RO_SINGLETON) and then use
XmlSubtypeFrom(XML_PRIMITIVE_RO_MASK, BddAtom(&tmp)) so the address-of operator
is applied to the value returned by CreateXMLRecAtom.
Comment on lines
83
to
86
| primitives := (XML_PRIMITIVE_NEVER | d.Primitives) | ||
| atom := (d.Primitives & XML_PRIMITIVE_SINGLETON) | ||
| sequence := BddUnion(BddAtom(common.ToPointer(CreateXMLRecAtom(atom))), d.Sequence) | ||
| sequence := BddUnion(BddAtom(new(CreateXMLRecAtom(atom))), d.Sequence) | ||
| return CreateXmlSubtype(primitives, sequence) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C2 --type=go '\bfunc\s+new\b'Repository: ballerina-platform/ballerina-lang-go
Length of output: 62
🏁 Script executed:
rg -n -C2 --type=go '\bvar\s+new\b|\bconst\s+new\b'Repository: ballerina-platform/ballerina-lang-go
Length of output: 62
🏁 Script executed:
cat -n semtypes/xml_subtype.go | sed -n '75,95p'Repository: ballerina-platform/ballerina-lang-go
Length of output: 894
🏁 Script executed:
rg -n 'func CreateXMLRecAtom' --type=go -A3Repository: ballerina-platform/ballerina-lang-go
Length of output: 308
🏁 Script executed:
rg -n 'func BddAtom' --type=go -A3Repository: ballerina-platform/ballerina-lang-go
Length of output: 342
🏁 Script executed:
rg -n 'type RecAtom' --type=go -A5Repository: ballerina-platform/ballerina-lang-go
Length of output: 333
🏁 Script executed:
rg -n 'type Atom' --type=go -B2 -A5Repository: ballerina-platform/ballerina-lang-go
Length of output: 598
🏁 Script executed:
rg -n 'func.*RecAtom.*Atom' --type=go -A3Repository: ballerina-platform/ballerina-lang-go
Length of output: 3448
new(CreateXMLRecAtom(atom)) is invalid Go syntax and will not compile.
The builtin new() operator expects a type argument, but CreateXMLRecAtom(atom) is a function call that returns a RecAtom value. This cannot be compiled. The code should create the value first, then take its address:
🔧 Proposed fix
- sequence := BddUnion(BddAtom(new(CreateXMLRecAtom(atom))), d.Sequence)
+ rec := CreateXMLRecAtom(atom)
+ sequence := BddUnion(BddAtom(&rec), d.Sequence)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| primitives := (XML_PRIMITIVE_NEVER | d.Primitives) | |
| atom := (d.Primitives & XML_PRIMITIVE_SINGLETON) | |
| sequence := BddUnion(BddAtom(common.ToPointer(CreateXMLRecAtom(atom))), d.Sequence) | |
| sequence := BddUnion(BddAtom(new(CreateXMLRecAtom(atom))), d.Sequence) | |
| return CreateXmlSubtype(primitives, sequence) | |
| primitives := (XML_PRIMITIVE_NEVER | d.Primitives) | |
| atom := (d.Primitives & XML_PRIMITIVE_SINGLETON) | |
| rec := CreateXMLRecAtom(atom) | |
| sequence := BddUnion(BddAtom(&rec), d.Sequence) | |
| return CreateXmlSubtype(primitives, sequence) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/xml_subtype.go` around lines 83 - 86, The call
new(CreateXMLRecAtom(atom)) is invalid because new expects a type; instead
construct the RecAtom value from CreateXMLRecAtom and pass its address to
BddAtom — i.e., call CreateXMLRecAtom(atom) to get a value, take its address,
then pass that pointer into BddAtom before unioning with d.Sequence and
returning CreateXmlSubtype(primitives, sequence); update the expression around
BddAtom(new(...)) in the sequence construction to use the address of the created
RecAtom value.
snelusha
force-pushed
the
chore/go-1.26
branch
from
22d3960 to
67d9438
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
=======================================
Coverage 29.48% 29.48%
=======================================
Files 257 257
Lines 54514 54470 -44
=======================================
- Hits 16074 16062 -12
+ Misses 37451 37425 -26
+ Partials 989 983 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
semtypes/field_pair.go (1)
96-116:⚠️ Potential issue | 🔴 CriticalInvalid use of
new()- this will not compile.All five occurrences of
new(CreateFieldPair(...))are invalid becausenew()requires a type argument, not a function call expression.CreateFieldPair()returns aFieldPairvalue.🐛 Proposed fix
func (i *mappingPairIterator) internalNext() *FieldPair { var p *FieldPair if i.i1 >= i.len1 { if i.i2 >= i.len2 { return nil } - p = new(CreateFieldPair(i.curName2(), i.rest1, i.curType2(), nil, &i.i2)) + tmp := CreateFieldPair(i.curName2(), i.rest1, i.curType2(), nil, &i.i2) + p = &tmp i.i2++ } else if i.i2 >= i.len2 { - p = new(CreateFieldPair(i.curName1(), i.curType1(), i.rest2, &i.i1, nil)) + tmp := CreateFieldPair(i.curName1(), i.curType1(), i.rest2, &i.i1, nil) + p = &tmp i.i1++ } else { name1 := i.curName1() name2 := i.curName2() if codePointCompare(name1, name2) { - p = new(CreateFieldPair(name1, i.curType1(), i.rest2, &i.i1, nil)) + tmp := CreateFieldPair(name1, i.curType1(), i.rest2, &i.i1, nil) + p = &tmp i.i1++ } else if codePointCompare(name2, name1) { - p = new(CreateFieldPair(name2, i.rest1, i.curType2(), nil, &i.i2)) + tmp := CreateFieldPair(name2, i.rest1, i.curType2(), nil, &i.i2) + p = &tmp i.i2++ } else { - p = new(CreateFieldPair(name1, i.curType1(), i.curType2(), &i.i1, &i.i2)) + tmp := CreateFieldPair(name1, i.curType1(), i.curType2(), &i.i1, &i.i2) + p = &tmp i.i1++ i.i2++ } } return p }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/field_pair.go` around lines 96 - 116, The code incorrectly uses new(CreateFieldPair(...)) which is a compile error because new expects a type; replace each new(CreateFieldPair(...)) with either calling CreateFieldPair(...) directly (assigning its returned FieldPair value) or taking its address (&CreateFieldPair(...)) if a *FieldPair is required, or construct with &FieldPair{...} as appropriate; update the five sites in the iterator function that reference CreateFieldPair so p receives the correct FieldPair or *FieldPair type matching the rest of the code (reference symbols: CreateFieldPair, FieldPair, p, i.curName1, i.curType1, i.curName2, i.curType2).parser/parser.go (1)
262-279:⚠️ Potential issue | 🟡 MinorForward variadic args with
...to avoid wrapping the slice.
Passingargswithout expansion sends a single[]anyelement to the callee, which can break diagnostic formatting.🔧 Suggested fix
- newNode := tree.CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode, args) + newNode := tree.CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode, args...)- newNode := tree.CloneWithLeadingInvalidNodeMinutiae(node, invalidParam, diagnosticCode, args) + newNode := tree.CloneWithLeadingInvalidNodeMinutiae(node, invalidParam, diagnosticCode, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parser/parser.go` around lines 262 - 279, The variadic parameter args is being forwarded without expansion which wraps it as a single []any argument; updateLastNodeInListWithInvalidNode should call tree.CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam, diagnosticCode, args...) and updateFirstNodeInListWithLeadingInvalidNode should forward to updateANodeInListWithLeadingInvalidNode as this.updateANodeInListWithLeadingInvalidNode(nodeList, 0, invalidParam, diagnosticCode, args...) so the callee receives the variadic arguments correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast/node_builder.go`:
- Line 1051: The code uses new(strings.ReplaceAll(...)) which is invalid;
instead create a string variable with strings.ReplaceAll(textValue, "+", "") and
assign its address to originalValue (e.g., tmp := strings.ReplaceAll(textValue,
"+", ""); originalValue = &tmp) or allocate then set value via ptr :=
new(string); *ptr = strings.ReplaceAll(...); originalValue = ptr; update the
assignment where originalValue is set so it compiles.
- Line 1115: The code uses invalid Go syntax new("null") for variable
originalValue; replace this with a valid construction based on the expected
type: if originalValue should be a string pointer, use p := new(string); *p =
"null"; originalValue = p (or directly originalValue = "null" if originalValue
is a string), or if it should be nil use originalValue = nil; update the
expression that references new("null") accordingly (look for originalValue and
the invalid new("null") call).
In `@semtypes/common.go`:
- Around line 29-31: The code incorrectly uses new(And(...)) which is invalid
Go; replace those new(And(bn.Atom(), pos)) and new(And(bn.Atom(), neg)) usages
with pointer-valued Conjunction composite literals (e.g. &Conjunction{Left:
bn.Atom(), Right: pos} and &Conjunction{Left: bn.Atom(), Right: neg}) so
bddEvery receives a *Conjunction; update the call sites in bddEvery to pass
these pointers instead of using new() around the And(...) expression.
In `@semtypes/error.go`:
- Around line 41-45: The call in ErrorDistinct uses new(...) with a function
result which doesn't compile; instead instantiate the RecAtom locally and pass
its address to BddAtom. Concretely, replace the new(CreateDistinctRecAtom(...))
pattern by creating a local variable like rec :=
CreateDistinctRecAtom(((-distinctId)-1)) and then call BddAtom(&rec) (or take
the address of the RecAtom value) so the pointer receiver methods satisfy the
Atom interface; apply the same change where CreateDistinctRecAtom is used in
object_definition (the analogous occurrence referenced in
semtypes/object_definition.go).
In `@semtypes/float_subtype.go`:
- Around line 73-75: In EnumerableFloat's membership check (where code currently
uses slices.Contains on v.values), replace slices.Contains with an explicit loop
that uses the type's equality logic—either call the EnumerableFloat.Compare
method for each element or use the bFloatEq helper—to compare each stored float
to f (this ensures NaN values are treated per the custom semantics); if a match
is found return v.allowed, otherwise continue to the end and preserve existing
behavior.
In `@semtypes/function_definition.go`:
- Line 69: The code incorrectly uses new() with a function call: replace the
invalid expression atom = new(env.functionAtom(&atomicType)) by directly using
the value returned by env.functionAtom(&atomicType) (or take its address if you
actually need a pointer). In short, update the assignment for variable atom to
either atom = env.functionAtom(&atomicType) or atom =
&env.functionAtom(&atomicType) depending on whether atom expects a value or a
pointer; reference env.functionAtom, atomicType and atom to locate the change.
In `@semtypes/list_definition.go`:
- Line 110: The code incorrectly calls new(env.listAtom(&atomicType)) — new
expects a type, not a function call. Replace that expression by calling the
helper directly and assigning its result to atom (e.g., atom =
env.listAtom(&atomicType)); if listAtom returns a value but you need a pointer,
take its address or ensure listAtom already returns a pointer. Update the
assignment for atom and keep the use of env.listAtom and atomicType as the
referenced symbols.
In `@semtypes/list_ops.go`:
- Line 227: The code uses new() incorrectly around function call results
(new(FixedLengthArrayFrom(...)) and new(intersectMemberSemTypes(...)))—new
expects a type, not a function call; fix by removing new() and returning the
function call results directly (i.e., return FixedLengthArrayFrom(initial,
fixedLen), intersectMemberSemTypes(env, rest1, rest2), true) or, if you intended
to take an address of a composite literal, replace with a proper address-of
composite (e.g., &FixedLengthArray{...}) consistent with the signatures of
FixedLengthArrayFrom and intersectMemberSemTypes.
---
Outside diff comments:
In `@parser/parser.go`:
- Around line 262-279: The variadic parameter args is being forwarded without
expansion which wraps it as a single []any argument;
updateLastNodeInListWithInvalidNode should call
tree.CloneWithTrailingInvalidNodeMinutiae(prevNode, invalidParam,
diagnosticCode, args...) and updateFirstNodeInListWithLeadingInvalidNode should
forward to updateANodeInListWithLeadingInvalidNode as
this.updateANodeInListWithLeadingInvalidNode(nodeList, 0, invalidParam,
diagnosticCode, args...) so the callee receives the variadic arguments
correctly.
In `@semtypes/field_pair.go`:
- Around line 96-116: The code incorrectly uses new(CreateFieldPair(...)) which
is a compile error because new expects a type; replace each
new(CreateFieldPair(...)) with either calling CreateFieldPair(...) directly
(assigning its returned FieldPair value) or taking its address
(&CreateFieldPair(...)) if a *FieldPair is required, or construct with
&FieldPair{...} as appropriate; update the five sites in the iterator function
that reference CreateFieldPair so p receives the correct FieldPair or *FieldPair
type matching the rest of the code (reference symbols: CreateFieldPair,
FieldPair, p, i.curName1, i.curType1, i.curName2, i.curType2).
---
Duplicate comments:
In @.github/workflows/native-ci.yml:
- Around line 16-19: The workflow currently hardcodes actions/setup-go@v5 with
go-version: '1.26' which may not match the Go version specified in the
repository's root go.mod; update the workflow to use the exact Go version from
the root go.mod (or a variable/parameter that is kept in sync) so the
actions/setup-go go-version value matches the module's go directive; locate the
actions/setup-go@v5 step and replace the literal '1.26' with the correct version
string from root go.mod (or reference a single shared input/ENV used by other CI
jobs) to ensure consistent availability checks.
In @.github/workflows/push.yml:
- Around line 25-28: The workflow's "Set up Go" step hardcodes go-version:
'1.26' which must match the root module's go directive; update the workflow so
the Go version is derived from or validated against the root go.mod instead of a
hardcoded value—either replace the hardcoded with a variable sourced from the
repo's go.mod or add a preceding verification step that reads the root go.mod's
"go X.Y" line and fails the job if it doesn't equal the value passed to the "Set
up Go" step (reference the "Set up Go" step and its with: go-version field when
making the change).
In @.github/workflows/release.yml:
- Around line 19-22: The workflow currently pins actions/setup-go@v5 with
go-version: '1.26' — update this to match the Go version declared in the
repository's root go.mod and ensure the same availability check is applied;
modify the actions/setup-go@v5 step (the go-version setting) to either
read/derive the version from the root go.mod or replace the hardcoded '1.26'
with the exact version string used in root go.mod and add the same
availability/check configuration used there (so the workflow and root go.mod
stay in sync).
In @.github/workflows/wasm-ci.yml:
- Around line 16-19: The workflow currently hardcodes go-version: '1.26' under
uses: actions/setup-go@v5; update the wasm-ci.yml so the Go version is kept in
sync with the repository root go.mod (do not leave a different hardcoded value).
Fix by replacing the literal '1.26' with a reference to the
single-source-of-truth (for example export a GO_VERSION workflow/env variable
set from the root go.mod or use a shared workflow input/template) so the value
used by uses: actions/setup-go@v5 always matches the root go.mod declaration.
In `@ast/node_builder.go`:
- Around line 1617-1618: The code incorrectly wraps calls in new(...), causing a
compile error; remove the invalid new(...) wrappers and assign the results of
the createIdentifier calls directly to bLVarRef.PkgAlias and
bLVarRef.VariableName (i.e. bLVarRef.PkgAlias =
createIdentifier(nameReference[0].GetPosition(), nameReference[0].GetValue(),
nameReference[0].GetValue()) and similarly for bLVarRef.VariableName),
referencing the createIdentifier function and the nameReference slices to locate
the change.
In `@compiler-tools/update-corpus/go.mod`:
- Line 3: Ensure this module's Go version directive matches the root go.mod's
availability check: keep the "go 1.26" directive in this go.mod if the root uses
"go 1.26" so the toolchain requirement is consistent, or remove the redundant
availability check here if the root/CI already enforces it; locate the "go 1.26"
directive in this file and make it identical to the root module's declaration or
delete it to avoid duplication.
In `@semtypes/enumerable_char_string.go`:
- Line 50: The expression uses new() incorrectly around a function call; replace
"return new(newEnumerableCharStringFromString(v))" with a direct return of the
constructed value from newEnumerableCharStringFromString(v) (or take its address
with & if newEnumerableCharStringFromString returns a non-pointer value) so the
code compiles; locate the call site in the function that returns the enumerable
char string and update the return to either "return
newEnumerableCharStringFromString(v)" or "return
&newEnumerableCharStringFromString(v)" based on
newEnumerableCharStringFromString's signature.
In `@semtypes/float_ops.go`:
- Around line 42-43: The code incorrectly calls new(t1.(FloatSubtype)) and
new(t2.(FloatSubtype)) — new expects a type, not an expression; replace these
with proper type assertions to the EnumerableSubtype[float64] interface (or take
the address of the concrete value if the implementation requires a pointer).
Concretely, change the v1/v2 initializations to either v1 :=
t1.(EnumerableSubtype[float64]) and v2 := t2.(EnumerableSubtype[float64]) or, if
FloatSubtype implements the interface only on pointer receivers, v1 :=
&t1.(FloatSubtype) and v2 := &t2.(FloatSubtype); mirror the fix used in the
Union method for the correct form.
- Around line 33-34: The code incorrectly uses new(t1.(FloatSubtype)) /
new(t2.(FloatSubtype)) — new takes a type, not an expression. Replace these with
proper type assertions and assign the asserted value (or its address) to the
EnumerableSubtype[float64] variables: e.g., v1Val, ok := t1.(FloatSubtype) and
then set var v1 EnumerableSubtype[float64] = v1Val or var v1
EnumerableSubtype[float64] = &v1Val depending on whether
EnumerableSubtype[float64] expects a value or a pointer; do the same for t2 and
handle the assertion failure (ok == false) appropriately.
In `@semtypes/mapping_definition.go`:
- Line 71: The code incorrectly uses new() with an expression: replace the
invalid expression new(env.mappingAtom(&atomicType)) by either calling the
function directly or creating a pointer composite literal depending on what
mappingAtom is; specifically, if env.mappingAtom is a constructor/function that
returns the atom, assign atom = env.mappingAtom(&atomicType), otherwise if
mappingAtom is a struct type use a composite literal like atom =
&env.mappingAtom{ /* set fields using atomicType */ } so that atom is assigned a
proper pointer to a mappingAtom instead of using new(...) with a call
expression.
In `@semtypes/string_subtype.go`:
- Around line 69-81: The current checks use slices.Contains with interface
values (EnumerableCharStringFrom/EnumerableStringFrom) which compares pointer
identity and will fail; replace those uses with slices.ContainsFunc and call
each element's Compare() to test equality: for the single-char branch, iterate
over chara.Values() with slices.ContainsFunc comparing each element to
EnumerableCharStringFrom(s).Compare(...), and for the multi-char branch do the
same against nonChar.Values() using EnumerableStringFrom(s).Compare(...); keep
the existing return logic using chara.Allowed() and nonChar.Allowed().
Comment on lines
41
to
45
| func ErrorDistinct(distinctId int) SemType { | ||
| // migrated from Error.java:57:5 | ||
| common.Assert(distinctId >= 0) | ||
| bdd := BddAtom(common.ToPointer(CreateDistinctRecAtom(((-distinctId) - 1)))) | ||
| bdd := BddAtom(new(CreateDistinctRecAtom(((-distinctId) - 1)))) | ||
| return basicSubtype(BT_ERROR, bdd) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine semtypes/error.go
find . -name "error.go" -path "*/semtypes/*" -type fRepository: ballerina-platform/ballerina-lang-go
Length of output: 99
🏁 Script executed:
# Once located, read the specific lines mentioned (41-45)
if [ -f "semtypes/error.go" ]; then
sed -n '35,50p' semtypes/error.go
fiRepository: ballerina-platform/ballerina-lang-go
Length of output: 397
🏁 Script executed:
# Search for CreateDistinctRecAtom function definition to understand its return type
rg -n "func.*CreateDistinctRecAtom" -A 3Repository: ballerina-platform/ballerina-lang-go
Length of output: 318
🏁 Script executed:
# Search for other uses of new(CreateDistinctRecAtom in the codebase
rg -n "new\s*\(\s*CreateDistinctRecAtom"Repository: ballerina-platform/ballerina-lang-go
Length of output: 259
🏁 Script executed:
# Find BddAtom function signature to see what it expects
rg -n "func.*BddAtom" -A 2Repository: ballerina-platform/ballerina-lang-go
Length of output: 310
🏁 Script executed:
# Check the RecAtom type definition to understand its structure
rg -n "type RecAtom" -A 5 | head -20Repository: ballerina-platform/ballerina-lang-go
Length of output: 333
🏁 Script executed:
# Count all occurrences of new(CreateDistinctRecAtom
rg "new\s*\(\s*CreateDistinctRecAtom" --count-matchesRepository: ballerina-platform/ballerina-lang-go
Length of output: 131
🏁 Script executed:
# Check what Atom interface looks like to understand if *RecAtom satisfies it
rg -n "type Atom" -A 5 | head -20Repository: ballerina-platform/ballerina-lang-go
Length of output: 996
🏁 Script executed:
# Check if RecAtom implements Atom methods (look for methods on RecAtom)
rg -n "func \(\w+ \*?RecAtom\)" | head -20Repository: ballerina-platform/ballerina-lang-go
Length of output: 269
🏁 Script executed:
# Verify the exact error that would occur by checking Go spec - test if new(functioncall) syntax works
# Also check if there are any workarounds in the codebase
rg "new\s*\(\s*\w+\s*\(" -cRepository: ballerina-platform/ballerina-lang-go
Length of output: 781
Fix invalid new(...) usage (compile error).
new only accepts a type, not a function call result; new(CreateDistinctRecAtom(...)) won't compile. Since RecAtom methods use pointer receivers, you must create a local variable and take its address to satisfy the Atom interface that BddAtom expects.
🛠️ Suggested fix
func ErrorDistinct(distinctId int) SemType {
// migrated from Error.java:57:5
common.Assert(distinctId >= 0)
- bdd := BddAtom(new(CreateDistinctRecAtom(((-distinctId) - 1))))
+ atom := CreateDistinctRecAtom(((-distinctId) - 1))
+ bdd := BddAtom(&atom)
return basicSubtype(BT_ERROR, bdd)
}This same error also appears in semtypes/object_definition.go:42.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ErrorDistinct(distinctId int) SemType { | |
| // migrated from Error.java:57:5 | |
| common.Assert(distinctId >= 0) | |
| bdd := BddAtom(common.ToPointer(CreateDistinctRecAtom(((-distinctId) - 1)))) | |
| bdd := BddAtom(new(CreateDistinctRecAtom(((-distinctId) - 1)))) | |
| return basicSubtype(BT_ERROR, bdd) | |
| func ErrorDistinct(distinctId int) SemType { | |
| // migrated from Error.java:57:5 | |
| common.Assert(distinctId >= 0) | |
| atom := CreateDistinctRecAtom(((-distinctId) - 1)) | |
| bdd := BddAtom(&atom) | |
| return basicSubtype(BT_ERROR, bdd) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/error.go` around lines 41 - 45, The call in ErrorDistinct uses
new(...) with a function result which doesn't compile; instead instantiate the
RecAtom locally and pass its address to BddAtom. Concretely, replace the
new(CreateDistinctRecAtom(...)) pattern by creating a local variable like rec :=
CreateDistinctRecAtom(((-distinctId)-1)) and then call BddAtom(&rec) (or take
the address of the RecAtom value) so the pointer receiver methods satisfy the
Atom interface; apply the same change where CreateDistinctRecAtom is used in
object_definition (the analogous occurrence referenced in
semtypes/object_definition.go).
| } | ||
| return common.ToPointer(FixedLengthArrayFrom(initial, fixedLen)), common.ToPointer(intersectMemberSemTypes(env, rest1, rest2)), true | ||
| fixedLen := max(members2.FixedLength, members1.FixedLength) | ||
| return new(FixedLengthArrayFrom(initial, fixedLen)), new(intersectMemberSemTypes(env, rest1, rest2)), true |
There was a problem hiding this comment.
Invalid new() usage - compile error.
new() requires a type argument, not a function call result. Both new(FixedLengthArrayFrom(...)) and new(intersectMemberSemTypes(...)) are invalid.
🔧 Suggested fix
- return new(FixedLengthArrayFrom(initial, fixedLen)), new(intersectMemberSemTypes(env, rest1, rest2)), true
+ members := FixedLengthArrayFrom(initial, fixedLen)
+ rest := intersectMemberSemTypes(env, rest1, rest2)
+ return &members, &rest, true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new(FixedLengthArrayFrom(initial, fixedLen)), new(intersectMemberSemTypes(env, rest1, rest2)), true | |
| members := FixedLengthArrayFrom(initial, fixedLen) | |
| rest := intersectMemberSemTypes(env, rest1, rest2) | |
| return &members, &rest, true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@semtypes/list_ops.go` at line 227, The code uses new() incorrectly around
function call results (new(FixedLengthArrayFrom(...)) and
new(intersectMemberSemTypes(...)))—new expects a type, not a function call; fix
by removing new() and returning the function call results directly (i.e., return
FixedLengthArrayFrom(initial, fixedLen), intersectMemberSemTypes(env, rest1,
rest2), true) or, if you intended to take an address of a composite literal,
replace with a proper address-of composite (e.g., &FixedLengthArray{...})
consistent with the signatures of FixedLengthArrayFrom and
intersectMemberSemTypes.
snelusha
force-pushed
the
chore/go-1.26
branch
from
67d9438 to
15f96ba
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$subject
Summary by CodeRabbit
Chores