Conversation
|
/korbit-review |
|
@coderabbitai full |
|
@ItsMeBrianD I'll perform a full review of all the changes in this PR. ✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Noise DSL and Noise construct: grammar updated (new Noise.g4, surface→surfaceBlock, NoiseThreshold renames), generated parser/lexer regenerated, new Go noise construct with export, traversal/symbol typing and nil guards, diagnostics dedup via lib.Unique, removal of IfRule negation, builder-chain dispatch and invocation hardening, and small build log tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (.mms)
participant P as Parser (ANTLR)
participant T as Traversal
participant B as BuilderChain
participant S as SymbolTable
participant N as Noise Construct
participant E as Exporter/FS
rect rgb(240,248,255)
U->>P: Parse source (surfaceBlock + noiseBlock)
P-->>T: Parse tree (surfaceBlock, noiseBlock, builders)
end
rect rgb(245,255,240)
T->>T: ProcessDeclaration(ctx,..., kind)
alt kind == "Noise"
T->>N: Build Noise (FirstOctave, Amplitudes) via constructor
T->>B: Invoke builder chain on builder child contexts
B-->>T: builder values
T->>S: Register Symbol(name, type="Noise", ref, locations)
end
end
rect rgb(255,250,240)
T->>E: ExportSymbol(Noise) -> write namespace/worldgen/noise/<name>.json
end
sequenceDiagram
autonumber
participant D as Declaration
participant T as Traversal
participant S as Symbol
participant W as WASM API
D->>T: ProcessDeclaration(..., "SurfaceRule" / "SurfaceCondition")
T->>S: Create symbol with Type = kind
Note over S: Symbol JSON includes Type field
T-->>W: Emit serialized symbols (includes type)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
lang/traversal/Symbol.go (1)
47-52: Panic risk: nil dereference of s.ref during JSON marshal when Identifier() is missing.If id is nil, out.ref remains nil; MarshalJSON dereferences it.
Apply these defensive fixes:
@@ - if id := ctx.Identifier(); id == nil { - scope.DiagnoseSemanticError("Missing Identifier", ctx) - } else { + if id := ctx.Identifier(); id == nil { + scope.DiagnoseSemanticError("Missing Identifier", ctx) + return nil + } else { out.ref = NewReference(id.GetText(), namespace) out.nameLocation = TerminalNodeLocation(id, scope.CurrentFile) } @@ -func (s BaseSymbol) MarshalJSON() ([]byte, error) { - return json.Marshal(struct { +func (s BaseSymbol) MarshalJSON() ([]byte, error) { + var ref Reference + if s.ref != nil { + ref = *s.ref + } + return json.Marshal(struct { NameLocation TextLocation `json:"nameLocation"` ContentLocation TextLocation `json:"contentLocation"` Value Construct `json:"value"` Ref Reference `json:"ref"` Type string `json:"type"` }{ NameLocation: s.nameLocation, ContentLocation: s.contentLocation, Value: s.GetValue(), - Ref: *s.ref, + Ref: ref, Type: s.GetType(), }) }Also applies to: 86-99
grammar/Surface.g4 (1)
15-24: And/Or allow a single operand — likely unintendedCurrent forms accept one condition; typically And/Or should require 2+. Recommend enforcing 2+.
Apply:
-surfaceCondition_And: 'And' NL* '(' NL* (surfaceCondition NL*)* surfaceCondition NL* ')'; -surfaceCondition_Or: 'Or' NL* '(' NL* (surfaceCondition NL*)* surfaceCondition NL* ')'; +surfaceCondition_And: 'And' NL* '(' NL* surfaceCondition (NL* surfaceCondition)+ NL* ')'; +surfaceCondition_Or: 'Or' NL* '(' NL* surfaceCondition (NL* surfaceCondition)+ NL* ')';lang/constructs/worldgen/surface_rules/Rule_If.go (2)
94-106: Fix: propagate scope in AND expansion to avoid nil deref during MarshalJSONNested IfRule instances created in the AND rewrite don’t carry scope. If a nested condition is a ReferenceCondition, MarshalJSON will call c.scope.Get(...) and panic. Add scope propagation.
- current = IfRule{ - Condition: cond, - Action: next, - } + current = IfRule{ + Condition: cond, + Action: next, + scope: c.scope, + }
16-21: Use pointer type in ConstructRegistry key for SurfaceRule_IfContext
In lang/constructs/worldgen/surface_rules/Rule_If.go at line 16, replacereflect.TypeFor[grammar.SurfaceRule_IfContext]()with
reflect.TypeFor[*grammar.SurfaceRule_IfContext]()so the registry key matches the actual
*SurfaceRule_IfContextpointer type.
♻️ Duplicate comments (2)
lang/grammar/minecraftmetascript_listener.go (2)
178-180: ExitSurfaceBlock rename mirrors Enter—no further action
235-246: Exit NoiseThreshold renames mirror Enter—ensure consumer exits updated
🧹 Nitpick comments (21)
lib/Unique.go (1)
4-11: Micro-opt: use struct{} set and preallocate capacities.Saves a bool per entry and reduces reallocations.
Apply this diff:
-func Unique[T comparable](arr []T) []T { - seen := make(map[T]bool) // A map to store seen elements - result := make([]T, 0) // The array to store unique elements +// Unique returns the first occurrence of each element in input order. +func Unique[T comparable](arr []T) []T { + seen := make(map[T]struct{}, len(arr)) // Set of seen elements + result := make([]T, 0, len(arr)) // Preallocate upper bound @@ - if _, ok := seen[v]; !ok { // If element not in map (not seen yet) - seen[v] = true // Mark as seen + if _, ok := seen[v]; !ok { // If element not in map (not seen yet) + seen[v] = struct{}{} // Mark as seenwasm/mms.d.ts (1)
33-39: Add minimal docs or a discriminant type for MmsSymbol.type.If possible, narrow from string to a union of known symbol kinds, or at least document examples to aid consumers.
Apply this small doc diff:
export type MmsSymbol = { @@ - type: string + /** Discriminant for the construct type (e.g., "Noise", "SurfaceRule", "NoiseThreshold"). */ + type: string }cmd/build.go (3)
67-73: Handle JSON marshal errors explicitly.Avoid continuing with a bad payload; log and return early.
Apply this diff:
- r, err := json.MarshalIndent(project.BuildFsLike(outFile), "", " ") + r, err := json.MarshalIndent(project.BuildFsLike(outFile), "", " ") + if err != nil { + log.Println("Error marshaling project:", err) + return + } @@ - string(r), - err, + string(r), + nil,
39-41: Nit: prefer log.Printf over log.Println(fmt.Sprintf()).Removes an allocation and is idiomatic.
Apply this diff:
- log.Println( - fmt.Sprintf("Building your project from %s to %s", inFile, outFile), - ) + log.Printf("Building your project from %s to %s", inFile, outFile)
47-49: Optional: use os.Stat for path parity with os.ReadFile.fs.Stat(os.DirFS("."), inFile) restricts paths to CWD; os.ReadFile(inFile) doesn’t. Using os.Stat keeps behavior consistent.
Apply this diff:
- stat, err := fs.Stat(os.DirFS("."), inFile) + stat, err := os.Stat(inFile)lang/traversal/Symbol.go (2)
29-36: NewSymbol doesn’t set _type; JSON emits type:"".Either thread type through constructor or provide a typed variant to avoid empty discriminants.
Add a typed constructor:
func NewSymbol(nameLocation TextLocation, contentLocation TextLocation, value Construct, ref *Reference) BaseSymbol { return BaseSymbol{ nameLocation: nameLocation, contentLocation: contentLocation, value: value, ref: ref, } } + +// NewSymbolWithType constructs a BaseSymbol with a type discriminant. +func NewSymbolWithType(nameLocation TextLocation, contentLocation TextLocation, value Construct, ref *Reference, typeName string) BaseSymbol { + return BaseSymbol{ + nameLocation: nameLocation, + contentLocation: contentLocation, + value: value, + ref: ref, + _type: typeName, + } +}
43-46: Minor: set type at initialization.Cleaner and avoids a separate assignment.
Apply this diff:
-func ProcessDeclaration(ctx DeclarationContext, valueCtx antlr.ParserRuleContext, scope *Scope, namespace string, typeName string) Symbol { - out := &BaseSymbol{} - out._type = typeName +func ProcessDeclaration(ctx DeclarationContext, valueCtx antlr.ParserRuleContext, scope *Scope, namespace string, typeName string) Symbol { + out := &BaseSymbol{_type: typeName}grammar/Surface.g4 (1)
50-54: NoiseThreshold builders look good; consider labeled alternatives for stable APIOptional: use labeled alternatives to generate stable listener/visitor method names without separate rules.
Example:
-surfaceCondition_NoiseThresholdBuilder: surfaceCondition_NoiseThresholdBuilder_Max | surfaceCondition_NoiseThresholdBuilder_Min; -surfaceCondition_NoiseThresholdBuilder_Min: '.Min(' number ')'; -surfaceCondition_NoiseThresholdBuilder_Max: '.Max(' number ')'; +surfaceCondition_NoiseThresholdBuilder + : '.Min(' number ')' #NoiseThresholdMin + | '.Max(' number ')' #NoiseThresholdMax + ;lang/Project.go (1)
23-24: Diagnostic is fully comparable; key-based dedup remains optional
Verified thatDiagnosticonly contains comparable fields (string,TextLocation,Severity), solib.Unique(diagnostics)compiles without error. If you’d rather dedupe by a subset of fields (e.g. message + location), introduce aUniqueByhelper (and aDiagnostic.Key()or similar) for custom‐key deduplication.main_wasm.go (1)
82-82: Consolidate diagnostic deduplication to a single stage
Remove the redundant lib.Unique call in either lang/Project.go:24 or main_wasm.go:82 so diagnostics are only deduplicated once.examples/overworld-noise-settings/surface-rules/badlands.mms (1)
21-23: NoiseThreshold used consistently for conditions
No remaining rawNoise(…)calls in condition contexts; theNoise(-4)…block at the top is the intended noise generator, not a threshold.
- Audit spelling of “Bandlands” in grammar/Surface.g4 (surfaceRule_Bandlands) vs “Badlands” in examples/overworld-noise-settings/surface-rules/badlands.mms to ensure consistency.
lang/constructs/worldgen/surface_rules/SurfaceRules.go (1)
39-39: Approve ProcessDeclaration signature updates
- All ProcessDeclaration call sites (Noise, SurfaceRules, SurfaceConditions) now include the new
typeNameargument, matching the updated signature.- Optional: extract symbol kind strings (e.g.
"SurfaceRule") into sharedconstdefinitions (e.g.KindSurfaceRule) to prevent string drift.grammar/Noise.g4 (2)
11-11: Allow trailing comma in amplitudes for friendlier diffs.Optional but common in config-like DSLs.
- noise_Builder_Amplitudes: '.Amplitudes' NL* '(' NL* number (NL* ',' NL* number)* NL* ')'; + noise_Builder_Amplitudes: '.Amplitudes' NL* '(' NL* number (NL* ',' NL* number)* (NL* ',')? NL* ')';
4-4: Consider requiring at least one declaration insideNoise { ... }.Prevents empty blocks that don’t produce artifacts.
- noiseBlock: 'Noise' NL* '{' NL* (noiseDeclaration NL*)* NL* '}'; + noiseBlock: 'Noise' NL* '{' NL* noiseDeclaration (NL+ noiseDeclaration)* NL* '}';lang/constructs/worldgen/surface_rules/Rule_If.go (1)
108-121: Optional: use pointers for consistency and to reduce copyingOR-branch uses &IfRule while AND-branch boxes values. Consider using pointers in both branches for consistency and less copying.
- next := c.Action + next := c.Action if i > 0 { - next = current + next = ¤t } - current = IfRule{ ... } + current = IfRule{ ... } // or make current a *IfRule and thread pointers throughoutlang/constructs/worldgen/noise/Noise.go (1)
79-89: Nit: preallocate amplitudes when possibleIf grammar exposes count, consider preallocating Amplitudes to avoid reslices. Not blocking.
lang/grammar/MinecraftMetascript.tokens (1)
85-87: ‘Noise(’ and ‘.Amplitudes(’ enforce no whitespace before ‘(’. Confirm this is intentional for the DSL.This mirrors existing builder tokens (e.g., ‘.Mul(’), but it rejects forms like
Noise ( ... ). If you want to allow a space, consider parsingNoisethen(instead of a single literal token.lang/grammar/minecraftmetascript_lexer.go (1)
49-52: Noise builder tokens added—document adjacency constraint.Like other builders, ‘Noise(’ and ‘.Amplitudes(’ forbid whitespace. Add a note in the language reference to avoid author friction.
lang/grammar/MinecraftMetascript.interp (2)
39-41: Noise DSL literals introduced; confirm whitespace policy.‘Noise(’ and ‘.Amplitudes(’ require no space before ‘(’. If you want to accept
Noise ( ... ), switch to tokenizingNoiseand(separately in the grammar for builders.
108-156: New rule set (noiseBlock, noiseDeclaration, noise, builder, amplitudes) integrated into contentBlocks—nice.Please add golden tests parsing a small Noise block and a Surface rule using NoiseThreshold min/max to lock this in.
I can draft minimal inputs and Go tests against the parser to exercise these paths.
lang/grammar/minecraftmetascript_base_listener.go (1)
369-399: New Noise hooks added—LGTM; pin codegen toolchainHooks look correct. Recommend pinning ANTLR (4.13.2) in your generation step to avoid drift across environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
cmd/build.go(1 hunks)examples/overworld-noise-settings/surface-rules/badlands.mms(2 hunks)grammar/MinecraftMetascript.g4(1 hunks)grammar/Noise.g4(1 hunks)grammar/Surface.g4(3 hunks)lang/Project.go(2 hunks)lang/constructs/worldgen/noise/Noise.go(1 hunks)lang/constructs/worldgen/surface_rules/Condition_Noise.go(2 hunks)lang/constructs/worldgen/surface_rules/Rule_If.go(2 hunks)lang/constructs/worldgen/surface_rules/SurfaceConditions.go(1 hunks)lang/constructs/worldgen/surface_rules/SurfaceRules.go(1 hunks)lang/grammar/MinecraftMetascript.interp(6 hunks)lang/grammar/MinecraftMetascript.tokens(3 hunks)lang/grammar/MinecraftMetascriptLexer.interp(5 hunks)lang/grammar/MinecraftMetascriptLexer.tokens(3 hunks)lang/grammar/minecraftmetascript_base_listener.go(3 hunks)lang/grammar/minecraftmetascript_lexer.go(2 hunks)lang/grammar/minecraftmetascript_listener.go(6 hunks)lang/traversal/Construct.go(1 hunks)lang/traversal/Symbol.go(4 hunks)lib/Unique.go(1 hunks)main_wasm.go(1 hunks)wasm/mms.d.ts(1 hunks)
🔇 Additional comments (28)
lib/Unique.go (1)
3-14: LGTM: stable, order-preserving dedup.Implementation is correct and idiomatic Go generics.
cmd/build.go (1)
47-52: Good: include underlying error when build stat fails.This improves diagnosability.
lang/traversal/Symbol.go (3)
56-57: Good: semantic error now anchors to decl ctx when value is missing.Better UX for error locations.
9-15: Ignore interface-expansion warning: only BaseSymbol implements Symbol and it already defines GetType(), no other implementations exist.Likely an incorrect or invalid review comment.
86-93: Approve addition of “type” field
No TS/JS consumers referencesymbol.type, so including this field in the JSON output is safe.lang/traversal/Construct.go (1)
41-43: Nil-guard prevents reflect panic — LGTMThis early return avoids a nil dereference on t.Kind() when ctx is a typed nil. Good defensive fix.
lang/grammar/MinecraftMetascriptLexer.tokens (1)
71-71: Regenerated artifacts reflect new tokens and all downstream references updated
All lexer/parser stubs includeNoiseThreshold,Noise(), and.Amplitudes(), and no stale references remain.grammar/Surface.g4 (1)
4-4: Rename surface → surfaceBlock: no legacy hooks found
Search across Go listener/visitor code reveals no occurrences ofEnterSurface/ExitSurfaceorSurfaceContext; no consumer updates required.lang/Project.go (1)
10-10: Noise constructs registration via side‑effect import looks correctBlank import ensures package init runs and registers Noise constructs.
grammar/MinecraftMetascript.g4 (1)
3-10: Imports and contentBlocks rules verified; no leftoverSurface(references detected. Regenerate the parser/lexer and sanity-parse representative scripts to confirm there are no FIRST/FOLLOW conflicts.examples/overworld-noise-settings/surface-rules/badlands.mms (1)
2-4: Example Noise block addition reads wellNice concise example showcasing Noise DSL usage.
grammar/Noise.g4 (1)
11-11:numberparser rule confirmed present (defined asnumber: Int | Float;), no fallback needed.lang/constructs/worldgen/surface_rules/SurfaceConditions.go (1)
49-50: VerifiedProcessDeclarationusage: all call sites now use the 5-argument signature.lang/grammar/MinecraftMetascriptLexer.interp (1)
39-41: Split composite tokens into separate symbols
At lang/grammar/MinecraftMetascriptLexer.interp (lines 39–41, 140–142), the lexer currently emits'Noise('and'.Amplitudes('as single tokens, preventing valid inputs likeNoise (1)or.Amplitudes ( … ). Update Noise.g4 so identifiers (e.g.Noise,.Amplitudes) and(are distinct tokens, regenerate the lexer, and verify that whitespace variants parse correctly.lang/constructs/worldgen/noise/Noise.go (1)
28-60: LGTM: solid Noise construct and declaration handlingParsing, diagnostics, and symbol export look good. JSON shape matches expectations.
lang/constructs/worldgen/surface_rules/Condition_Noise.go (2)
64-76: LGTM: JSON field names align with NoiseThreshold specField names noise_threshold, min_threshold, max_threshold look consistent.
30-33: No ctx type inconsistency in ConstructRegistry.Register
Allsurface_rulesregistrations usereflect.TypeFor[T]()—which returns the pointer type—and then cast viactx.(*T), so the registered type matches the asserted pointer context in every case.lang/grammar/MinecraftMetascript.tokens (2)
71-71: Rename from ‘Noise’ to ‘NoiseThreshold’ looks correct and resolves ambiguity.Placement before the ‘Noise’ token ensures ‘NoiseThreshold’ is matched as a single token, not split. LGTM.
40-48: Confirm no raw 40–48 token ID usage. Ripgrep shows only named constants (MinecraftMetascriptLexerStoneDepthMode…LineComment) inminecraftmetascript_lexer.go, but verify no code, tests, or external consumers hardcode literals 40–48.lang/grammar/minecraftmetascript_lexer.go (2)
56-58: Symbolic names update aligns with inserted T__36–T__38; no issues.
340-351: Literal token cases safe; no changes required
RG search found onlycase 1–3in Reference.go and the generated parser, which map toT__0–T__2and remain unchanged after addingT__36–T__38. No cases using literals ≥ 37 were found, so downstream code isn’t affected by the reindexing.lang/grammar/MinecraftMetascript.interp (2)
25-25: Surface condition rename to ‘NoiseThreshold’ is consistent and desirable.The rename avoids confusion with the new Noise DSL. Good change.
158-158: ATN table changed due to grammar edits—ack.Generated delta only; nothing actionable here.
lang/grammar/minecraftmetascript_base_listener.go (2)
49-54: Surface→SurfaceBlock rename is API-breaking—verify downstream listeners/traversals are updatedLooks consistent with the grammar change; just ensure any custom listeners no longer reference Surface/SurfaceContext.
187-217: NoiseBuilder→NoiseThreshold rename propagated—confirm consumers migratedUpdate any code using:
- SurfaceCondition_NoiseBuilder_{Min,Max} → SurfaceCondition_NoiseThresholdBuilder_{Min,Max}
- SurfaceCondition_NoiseBuilder → SurfaceCondition_NoiseThresholdBuilder
- SurfaceCondition_Noise → SurfaceCondition_NoiseThreshold
lang/grammar/minecraftmetascript_listener.go (3)
79-90: NoiseThreshold method renames—ensure traversal implements new hooksMap and update:
- EnterSurfaceCondition_NoiseBuilder_{Min,Max} → EnterSurfaceCondition_NoiseThresholdBuilder_{Min,Max}
- EnterSurfaceCondition_NoiseBuilder → EnterSurfaceCondition_NoiseThresholdBuilder
- EnterSurfaceCondition_Noise → EnterSurfaceCondition_NoiseThreshold
151-165: New Noise productions (Enter)—wire into traversal and add parser tests*Add handling for NoiseBlock/Declaration/Builder/Amplitudes and a couple of DSL samples to tests.
307-320: New Noise productions (Exit)—LGTM*
| if conditionCtx.GetChildCount() > 0 { | ||
| return traversal.ConstructRegistry.Construct( | ||
| conditionCtx.GetChild(0).(antlr.ParserRuleContext), | ||
| currentNamespace, | ||
| scope, | ||
| ) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Avoid panic: first child may be a terminal, not a ParserRuleContext.
Casting GetChild(0) to antlr.ParserRuleContext can panic when the first child is a token (e.g., !). Scan for the first rule child instead.
- if conditionCtx.GetChildCount() > 0 {
- return traversal.ConstructRegistry.Construct(
- conditionCtx.GetChild(0).(antlr.ParserRuleContext),
- currentNamespace,
- scope,
- )
- }
+ if conditionCtx.GetChildCount() > 0 {
+ var inner antlr.ParserRuleContext
+ for i := 0; i < conditionCtx.GetChildCount(); i++ {
+ if c, ok := conditionCtx.GetChild(i).(antlr.ParserRuleContext); ok {
+ inner = c
+ break
+ }
+ }
+ if inner != nil {
+ return traversal.ConstructRegistry.Construct(inner, currentNamespace, scope)
+ }
+ }
return nil📝 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.
| if conditionCtx.GetChildCount() > 0 { | |
| return traversal.ConstructRegistry.Construct( | |
| conditionCtx.GetChild(0).(antlr.ParserRuleContext), | |
| currentNamespace, | |
| scope, | |
| ) | |
| } | |
| return nil | |
| if conditionCtx.GetChildCount() > 0 { | |
| var inner antlr.ParserRuleContext | |
| for i := 0; i < conditionCtx.GetChildCount(); i++ { | |
| if c, ok := conditionCtx.GetChild(i).(antlr.ParserRuleContext); ok { | |
| inner = c | |
| break | |
| } | |
| } | |
| if inner != nil { | |
| return traversal.ConstructRegistry.Construct( | |
| inner, | |
| currentNamespace, | |
| scope, | |
| ) | |
| } | |
| } | |
| return nil |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/SurfaceConditions.go around lines 36
to 43, the code casts conditionCtx.GetChild(0) to antlr.ParserRuleContext which
can panic if the first child is a terminal; instead iterate children (from 0 to
conditionCtx.GetChildCount()-1), find the first child that implements
antlr.ParserRuleContext, pass that child to traversal.Construct with
currentNamespace and scope, and return its result; if no rule-child is found
return nil.
| "'NoiseThreshold'", "'StoneDepth('", "'.SecondaryDepthRange('", "'VerticalGradient'", | ||
| "'.Top'", "'.Bottom'", "'AboveWater'", "'YAbove'", "'Block'", "'['", | ||
| "']'", "'Bandlands'", "'If'", "':'", | ||
| "']'", "'Bandlands'", "'If'", "':'", "'Noise'", "'Noise('", "'.Amplitudes('", | ||
| } |
There was a problem hiding this comment.
Typo in literal: ‘Bandlands’ → ‘Badlands’.
User-facing keyword is misspelled. This will confuse authors and break examples that use “Badlands”.
Apply in grammar sources (then regenerate):
--- a/lang/grammar/Surface.g4
+++ b/lang/grammar/Surface.g4
- 'Bandlands'
+ 'Badlands'If the rule name also encodes this (seen in interp as surfaceRule_Bandlands), rename it:
--- a/lang/grammar/Surface.g4
+++ b/lang/grammar/Surface.g4
- surfaceRule_Bandlands
+ surfaceRule_BadlandsI can push a follow-up commit that updates both grammar and regenerated artifacts.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lang/grammar/minecraftmetascript_lexer.go around lines 49 to 52, the
user-facing literal "'Bandlands'" is misspelled and should be "'Badlands'";
update the token/literal in the grammar source to "Badlands", search the grammar
and related source files for any rule names or identifiers encoding "Bandlands"
(e.g., surfaceRule_Bandlands) and rename them to use "Badlands", then regenerate
the lexer/parser/artifacts so all references and generated code match the
corrected spelling.
| // EnterSurfaceBlock is called when entering the surfaceBlock production. | ||
| EnterSurfaceBlock(c *SurfaceBlockContext) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Surface→SurfaceBlock interface rename: breaking change—document or bump module version
Good change, but it breaks any external implementers. Add migration notes or a semver-major bump.
Run to find outdated usages:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Old listener methods/contexts
rg -nP -g '!**/vendor/**' '\b(Enter|Exit)Surface\('
rg -nP -g '!**/vendor/**' '\bSurfaceContext\b'
# Old Noise* names
rg -nP -g '!**/vendor/**' '\bSurfaceCondition_NoiseBuilder(_Min|_Max)?\b|\bSurfaceCondition_Noise\b'
# Sanity: new names present
rg -nP -g '!**/vendor/**' '\bSurfaceBlockContext\b|\b(Enter|Exit)SurfaceBlock\('Length of output: 73
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "Searching for old listener methods (Enter/Exit Surface)..."
rg -nP -g '!**/vendor/**' '\b(Enter|Exit)Surface\(' || true
echo "Searching for old SurfaceContext uses..."
rg -nP -g '!**/vendor/**' '\bSurfaceContext\b' || true
echo "Searching for old Noise* type names..."
rg -nP -g '!**/vendor/**' '\bSurfaceCondition_NoiseBuilder(_Min|_Max)?\b|\bSurfaceCondition_Noise\b' || true
echo "Verifying new SurfaceBlock names are present..."
rg -nP -g '!**/vendor/**' '\bSurfaceBlockContext\b|\b(Enter|Exit)SurfaceBlock\(' || trueLength of output: 3203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f go.mod ]; then
echo "go.mod first lines:"
head -n 5 go.mod
else
echo "go.mod not found"
fiLength of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for CHANGELOG.md..."
if [ -f CHANGELOG.md ]; then head -n 20 CHANGELOG.md; else echo "No CHANGELOG.md found"; fi
echo "Checking README.md for migration notes..."
if [ -f README.md ]; then rg -n 'SurfaceBlock' README.md || echo "No mention of SurfaceBlock in README.md"; else echo "No README.md found"; fiLength of output: 328
Document breaking change or bump MAJOR version
Renaming EnterSurface/ExitSurface to EnterSurfaceBlock/ExitSurfaceBlock is a semver-major change. Add a migration note (e.g. in a new CHANGELOG.md) and bump the module’s major version accordingly.
🤖 Prompt for AI Agents
In lang/grammar/minecraftmetascript_listener.go around lines 22–24, the API was
changed by renaming EnterSurface/ExitSurface to
EnterSurfaceBlock/ExitSurfaceBlock which is a semver-major breaking change; add
a migration note (create or update CHANGELOG.md) documenting the renames and
code examples for migrating callers, then bump the module major version: update
the module path in go.mod to the next major (e.g. add /v2), update any internal
imports to the new path, run go mod tidy, and create the corresponding vN.0.0
git tag/release so downstream users can adopt the breaking change.
| T__36=37 | ||
| T__37=38 | ||
| T__38=39 | ||
| StoneDepthMode=40 | ||
| Int=41 | ||
| Float=42 | ||
| String=43 | ||
| NL=44 | ||
| WS=45 | ||
| Identifier=46 | ||
| BlockComment=47 | ||
| LineComment=48 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Token ID renumbering: verify no hard-coded numeric token checks
IDs shifted (new T__36..38), which can break code comparing GetTokenType() to integers. Ensure all comparisons use generated constants, not raw numbers.
Run to spot suspicious numeric token checks:
🏁 Script executed:
#!/bin/bash
# numeric token comparisons
rg -nP '\b(GetTokenType|GetToken)\s*\(\)\s*==\s*\d+'
# switch cases on numeric literals (scan likely parser utils)
rg -nP 'case\s+\d+\s*:' --iglob '!**/vendor/**'Length of output: 359
Replace hard-coded token numbers in lang/traversal/Reference.go
Switch on raw values (case 1, case 2) at lines 21 and 23 must use the generated token constants (e.g. MinecraftMetascriptLexer.T__1) instead of literals.
🤖 Prompt for AI Agents
In lang/traversal/Reference.go around lines 21 to 23, the switch statements use
hard-coded numeric token values (e.g., case 1, case 2); replace those numeric
literals with the corresponding generated token constants from
lang/grammar/MinecraftMetascriptLexer (for example
MinecraftMetascriptLexer.T__1, MinecraftMetascriptLexer.T__2) so the code
references named tokens instead of raw integers; update any import or package
qualifier if needed to reference the MinecraftMetascriptLexer symbols and run a
build to confirm the constants are resolved.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
lang/constructs/worldgen/surface_rules/Condition_Noise.go (1)
26-28: Fix incorrect label in Builder_GetFloat callThe second
Builder_GetFloatcall incorrectly uses "Min" as the label when setting theMaxfield. This will produce misleading error messages.Apply this diff to fix the label:
func(ctx *grammar.SurfaceCondition_NoiseThresholdBuilder_MaxContext, out *NoiseCondition, scope *traversal.Scope, _ string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { out.Max = v }, scope, "Min") + builder_chain.Builder_GetFloat(ctx, func(v float64) { out.Max = v }, scope, "Max") },
🧹 Nitpick comments (2)
lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go (1)
20-24: Nit: align diagnostic label with JSON/key namingConsider using "Secondary Depth Range" consistently with the JSON key or switch to "secondary depth range" for uniform messages.
lang/constructs/worldgen/surface_rules/Condition_Noise.go (1)
52-65: Consider using pointer type directly for Require callsSince
Requirenow handles pointer normalization internally, you could simplify the code by using the pointer type directly withoutreflect.TypeFor.You could simplify the type specifications:
builder_chain.Require( noiseBuildChain, noise, scope, - reflect.TypeFor[*grammar.SurfaceCondition_NoiseThresholdBuilder_MinContext](), + reflect.TypeOf((*grammar.SurfaceCondition_NoiseThresholdBuilder_MinContext)(nil)).Elem(), ".Min", )Though the current approach with
TypeForis actually clearer and more maintainable, so you may want to keep it as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cmd/build.go(1 hunks)examples/overworld-noise-settings/surface-rules/badlands.mms(2 hunks)grammar/Noise.g4(1 hunks)lang/builder_chain/BuilderChain.go(2 hunks)lang/constructs/worldgen/surface_rules/Condition_AboveWater.go(1 hunks)lang/constructs/worldgen/surface_rules/Condition_Noise.go(2 hunks)lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go(2 hunks)lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go(2 hunks)lang/constructs/worldgen/surface_rules/Condition_YAbove.go(2 hunks)lang/constructs/worldgen/surface_rules/Rule_If.go(3 hunks)lang/traversal/Symbol.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/build.go
- grammar/Noise.g4
- examples/overworld-noise-settings/surface-rules/badlands.mms
🧰 Additional context used
🧬 Code graph analysis (8)
lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go (3)
lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(75-84)Build(62-73)Invoke(20-41)lang/builder_chain/SharedBuilder.go (1)
Builder_GetVerticalAnchor(84-107)lang/constructs/primitives/VerticalAnchor.go (1)
VerticalAnchor(63-67)
lang/builder_chain/BuilderChain.go (1)
lang/traversal/Scope.go (1)
Scope(11-16)
lang/constructs/worldgen/surface_rules/Condition_YAbove.go (2)
lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(75-84)Build(62-73)Invoke(20-41)lang/builder_chain/SharedBuilder.go (2)
Builder_GetInt(39-59)SharedBuilder_Add(30-37)
lang/constructs/worldgen/surface_rules/Condition_AboveWater.go (1)
lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(75-84)Build(62-73)Invoke(20-41)
lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go (2)
lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(75-84)Build(62-73)Invoke(20-41)lang/builder_chain/SharedBuilder.go (2)
Builder_GetInt(39-59)SharedBuilder_Add(30-37)
lang/constructs/worldgen/surface_rules/Condition_Noise.go (3)
lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (4)
NewBuilderChain(75-84)Build(62-73)Invoke(20-41)Require(43-53)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)
lang/constructs/worldgen/surface_rules/Rule_If.go (2)
lang/traversal/Scope.go (1)
Scope(11-16)lang/traversal/Construct.go (1)
Construct(12-16)
lang/traversal/Symbol.go (4)
lang/traversal/Location.go (1)
TextLocation(14-21)lang/traversal/Construct.go (1)
Construct(12-16)lang/traversal/Reference.go (2)
Reference(30-36)Ref(46-48)lang/traversal/Scope.go (1)
Scope(11-16)
🔇 Additional comments (14)
lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go (1)
19-35: Builder-chain construction moved in-scope: LGTMCreating the chain inside the registration closure avoids shared state and aligns with other conditions.
lang/constructs/worldgen/surface_rules/Condition_YAbove.go (1)
20-31: In-scope builder-chain refactor: LGTMLocalizing yAboveBuilder and field wiring look correct.
lang/constructs/worldgen/surface_rules/Condition_AboveWater.go (1)
19-40: Builder chain localized: LGTMOffset/Add/Multiplier builders are correctly registered; float multiplier label is clear.
lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go (1)
20-31: Builder-chain refactor: LGTMTop/Bottom anchors via Builder_GetVerticalAnchor are wired correctly.
lang/constructs/worldgen/surface_rules/Rule_If.go (2)
104-105: Scope propagation in expanded rules: LGTMCarrying c.scope into nested IfRule and OR-sequence entries preserves late reference resolution context.
Also applies to: 118-119
16-16: No issues—pointer vs non-pointer handled uniformly
ConstructRegistry.Register strips pointers (ctxType.Elem()), and Construct() looks up by the underlying type, so registrations usingreflect.TypeFor[*…]or non-pointer both normalize correctly—no dispatch gaps.lang/builder_chain/BuilderChain.go (4)
20-24: Good runtime type normalization approachThe change from compile-time generic parameter to runtime type resolution with consistent pointer normalization is a solid improvement. This unifies dispatch semantics and handles both pointer and non-pointer contexts correctly.
30-38: Good duplicate detection with improved type handlingThe duplicate builder call detection now correctly uses the normalized non-pointer type, which prevents false negatives when the same context is passed with different pointer indirections.
44-46: Consistent type normalization in Require methodGood addition of pointer normalization in the
Requiremethod to match the changes inInvoke. This ensures that requirement checking uses the same type resolution logic.
63-68: Well-implemented registration with type normalizationThe
Buildfunction correctly derives and normalizes the context type at registration time, ensuring consistency with the runtime dispatch logic inInvoke.lang/traversal/Symbol.go (4)
14-14: Clean implementation of symbol type trackingThe addition of
GetType()to the Symbol interface and its implementation in BaseSymbol provides a clean way to track and expose symbol types, which will be useful for type-aware operations and JSON serialization.Also applies to: 22-27
43-45: Good addition of type parameter to ProcessDeclarationAdding the
typeNameparameter allows callers to specify the semantic type of symbols at creation time, improving type awareness throughout the system.
56-56: Correct error context for missing value diagnosticGood fix - using the declaration context (
ctx) instead of the value context (valueCtx) for the missing value error provides better error location information to users.
91-92: Proper JSON serialization of new type fieldThe MarshalJSON implementation correctly includes the new Type field and properly serializes the reference as a pointer, maintaining backward compatibility while exposing the new type information.
Also applies to: 97-98
| child := r.GetChild(0) | ||
| if child == nil { | ||
| continue | ||
| } | ||
| builder_chain.Invoke(waterBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) |
There was a problem hiding this comment.
Avoid panic: guard child type before cast
Protect the cast and surface a warning for unexpected nodes.
Apply:
for _, r := range aboveWater.AllSurfaceCondition_AboveWaterBuilder() {
child := r.GetChild(0)
if child == nil {
continue
}
- builder_chain.Invoke(waterBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace)
+ if childCtx, ok := child.(antlr.ParserRuleContext); ok {
+ builder_chain.Invoke(waterBuildChain, childCtx, out, scope, namespace)
+ } else {
+ scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r)
+ }
}📝 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.
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| builder_chain.Invoke(waterBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) | |
| for _, r := range aboveWater.AllSurfaceCondition_AboveWaterBuilder() { | |
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| if childCtx, ok := child.(antlr.ParserRuleContext); ok { | |
| builder_chain.Invoke(waterBuildChain, childCtx, out, scope, namespace) | |
| } else { | |
| scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r) | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_AboveWater.go around lines
45 to 49, the code directly type-asserts child.(antlr.ParserRuleContext) which
can panic if the node is not the expected type; instead perform a safe type
assertion (childAsCtx, ok := child.(antlr.ParserRuleContext)), and if ok is
false emit a warning (using the existing logger or fmt.Errorf/LogWarn) that
includes the unexpected child type and its index, then continue the loop without
invoking builder_chain; only call builder_chain.Invoke when the assertion
succeeds.
| child := r.GetChild(0) | ||
| if child == nil { | ||
| continue | ||
| } | ||
| builder_chain.Invoke(noiseBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil check before type assertion
The code performs a type assertion on child.(antlr.ParserRuleContext) without verifying that the child can be cast to this type. While the nil check on line 46 prevents nil pointer issues, it doesn't guarantee the type assertion will succeed.
Apply this diff to add proper type checking:
for _, r := range noise.AllSurfaceCondition_NoiseThresholdBuilder() {
child := r.GetChild(0)
- if child == nil {
- continue
+ if childCtx, ok := child.(antlr.ParserRuleContext); ok && childCtx != nil {
+ builder_chain.Invoke(noiseBuildChain, childCtx, out, scope, namespace)
}
- builder_chain.Invoke(noiseBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace)
}📝 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.
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| builder_chain.Invoke(noiseBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) | |
| for _, r := range noise.AllSurfaceCondition_NoiseThresholdBuilder() { | |
| child := r.GetChild(0) | |
| if childCtx, ok := child.(antlr.ParserRuleContext); ok && childCtx != nil { | |
| builder_chain.Invoke(noiseBuildChain, childCtx, out, scope, namespace) | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_Noise.go around lines 45 to
49, the code does a direct type assertion child.(antlr.ParserRuleContext) which
can panic if the runtime type is different; change it to perform a safe type
assertion using the comma-ok form (e.g., pr, ok :=
child.(antlr.ParserRuleContext)) and only call
builder_chain.Invoke(noiseBuildChain, pr, out, scope, namespace) when ok is true
(otherwise continue); keep the existing nil check if desired but ensure you
guard the type cast before invoking.
| child := r.GetChild(0) | ||
| if child == nil { | ||
| continue | ||
| } | ||
| builder_chain.Invoke(stoneDepthBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) | ||
| } |
There was a problem hiding this comment.
Avoid panic: guard child type before cast
child may be a terminal; direct cast to antlr.ParserRuleContext can panic. Add a safe assertion and warn.
Apply:
for _, r := range stoneDepth.AllSurfaceCondition_StoneDepthBuilder() {
child := r.GetChild(0)
if child == nil {
continue
}
- builder_chain.Invoke(stoneDepthBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace)
+ if childCtx, ok := child.(antlr.ParserRuleContext); ok {
+ builder_chain.Invoke(stoneDepthBuildChain, childCtx, out, scope, namespace)
+ } else {
+ scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r)
+ }
}📝 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.
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| builder_chain.Invoke(stoneDepthBuildChain, child.(antlr.ParserRuleContext), out, scope, namespace) | |
| } | |
| for _, r := range stoneDepth.AllSurfaceCondition_StoneDepthBuilder() { | |
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| if childCtx, ok := child.(antlr.ParserRuleContext); ok { | |
| builder_chain.Invoke(stoneDepthBuildChain, childCtx, out, scope, namespace) | |
| } else { | |
| scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r) | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go around lines
46 to 51, the code unconditionally casts child to antlr.ParserRuleContext which
can panic if child is a terminal; replace the direct cast with a safe type
assertion (ctx, ok := child.(antlr.ParserRuleContext)), and if !ok log or warn
about unexpected terminal type and continue the loop; only call
builder_chain.Invoke(stoneDepthBuildChain, ctx, out, scope, namespace) when ok
is true.
| child := r.GetChild(0) | ||
| if child == nil { | ||
| continue | ||
| } | ||
| builder_chain.Invoke( | ||
| verticalGradientBuilder, r, out, scope, ns, | ||
| verticalGradientBuilder, child.(antlr.ParserRuleContext), out, scope, ns, | ||
| ) |
There was a problem hiding this comment.
Avoid panic: guard child type before cast
Same safety improvement as other files.
Apply:
child := r.GetChild(0)
if child == nil {
continue
}
-builder_chain.Invoke(
- verticalGradientBuilder, child.(antlr.ParserRuleContext), out, scope, ns,
-)
+if childCtx, ok := child.(antlr.ParserRuleContext); ok {
+ builder_chain.Invoke(
+ verticalGradientBuilder, childCtx, out, scope, ns,
+ )
+} else {
+ scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r)
+}📝 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.
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| builder_chain.Invoke( | |
| verticalGradientBuilder, r, out, scope, ns, | |
| verticalGradientBuilder, child.(antlr.ParserRuleContext), out, scope, ns, | |
| ) | |
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| if childCtx, ok := child.(antlr.ParserRuleContext); ok { | |
| builder_chain.Invoke( | |
| verticalGradientBuilder, childCtx, out, scope, ns, | |
| ) | |
| } else { | |
| scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r) | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go around
lines 46 to 52, the code directly type-asserts child.(antlr.ParserRuleContext)
which can panic if the child is not that type; change to a safe assertion: call
child := r.GetChild(0), keep the nil check, then do ctx, ok :=
child.(antlr.ParserRuleContext) and if !ok { continue } before invoking
builder_chain.Invoke with ctx, out, scope, ns so the function no longer panics
on unexpected child types.
| func(ctx *grammar.SharedBuilder_MulIntContext, target *YAboveCondition, scope *traversal.Scope, namespace string) { | ||
| builder_chain.Builder_GetInt(ctx, func(v int) { target.Multiplier = v }, scope, "Top") | ||
| }, |
There was a problem hiding this comment.
Fix mislabeled diagnostic: "Top" → "Multiplier"
This error label is confusing for users when validating the multiplier value.
Apply:
- builder_chain.Builder_GetInt(ctx, func(v int) { target.Multiplier = v }, scope, "Top")
+ builder_chain.Builder_GetInt(ctx, func(v int) { target.Multiplier = v }, scope, "Multiplier")📝 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.
| func(ctx *grammar.SharedBuilder_MulIntContext, target *YAboveCondition, scope *traversal.Scope, namespace string) { | |
| builder_chain.Builder_GetInt(ctx, func(v int) { target.Multiplier = v }, scope, "Top") | |
| }, | |
| func(ctx *grammar.SharedBuilder_MulIntContext, target *YAboveCondition, scope *traversal.Scope, namespace string) { | |
| builder_chain.Builder_GetInt(ctx, func(v int) { target.Multiplier = v }, scope, "Multiplier") | |
| }, |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_YAbove.go around lines 22 to
24, the diagnostic label passed to builder_chain.Builder_GetInt is incorrectly
"Top" and should be "Multiplier"; update the fourth argument of Builder_GetInt
from "Top" to "Multiplier" so the validation/error messages correctly refer to
the Multiplier field.
| child := r.GetChild(0) | ||
| if child == nil { | ||
| continue | ||
| } | ||
| builder_chain.Invoke(yAboveBuilder, child.(antlr.ParserRuleContext), out, scope, ns) |
There was a problem hiding this comment.
Avoid panic: guard child type before cast
Same risk as other conditions; protect the cast.
Apply:
for _, r := range yAbove.AllSurfaceCondition_YAboveBuilder() {
child := r.GetChild(0)
if child == nil {
continue
}
- builder_chain.Invoke(yAboveBuilder, child.(antlr.ParserRuleContext), out, scope, ns)
+ if childCtx, ok := child.(antlr.ParserRuleContext); ok {
+ builder_chain.Invoke(yAboveBuilder, childCtx, out, scope, ns)
+ } else {
+ scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r)
+ }
}📝 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.
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| builder_chain.Invoke(yAboveBuilder, child.(antlr.ParserRuleContext), out, scope, ns) | |
| for _, r := range yAbove.AllSurfaceCondition_YAboveBuilder() { | |
| child := r.GetChild(0) | |
| if child == nil { | |
| continue | |
| } | |
| if childCtx, ok := child.(antlr.ParserRuleContext); ok { | |
| builder_chain.Invoke(yAboveBuilder, childCtx, out, scope, ns) | |
| } else { | |
| scope.DiagnoseSemanticWarning("Ignoring unexpected builder node; expected rule context", r) | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Condition_YAbove.go around lines 45 to
49, the code currently casts child to antlr.ParserRuleContext without verifying
its dynamic type which can panic; change the cast to a safe type assertion (e.g.
ctx, ok := child.(antlr.ParserRuleContext)) and only call
builder_chain.Invoke(yAboveBuilder, ctx, out, scope, ns) when ok is true,
otherwise continue; keep the existing nil check for child.
| scope: scope, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden reference resolver against nil scope
The resolver closure captures scope; if nil, dereference will panic at runtime. Use out.scope with a nil check.
Update the resolver (shown here for clarity; change occurs where SetResolver is defined):
ref.Ref.SetResolver(func() error {
if out.scope == nil {
return nil
}
if next, ok := out.scope.Get(*ref.Ref); ok {
out.Condition = next.GetValue()
}
return nil
})🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/Rule_If.go around lines 20-21, the
resolver closure captures scope and can panic if scope is nil; modify the
resolver passed to SetResolver to reference out.scope and first check if
out.scope == nil, returning early if so, otherwise perform the lookup
(out.scope.Get) and update out.Condition when found; ensure the closure returns
nil on early exit and after successful resolution.
1 participant
Description by Korbit AI
What change is being made?
Add a new 'Noise' feature to the codebase with associated grammar and implementation, and refactor the surface condition related to noise conditions to use 'NoiseThreshold'.
Why are these changes being made?
The addition of the 'Noise' feature enhances the world generation capabilities by defining customizable noise settings that can affect surface rules. Refactoring to use 'NoiseThreshold' aligns with the updated specification for distinguishing noise conditions, improving readability and maintainability within the code.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation