Conversation
WalkthroughAdds a new DensityFunctions ANTLR grammar and integrates it into the language and parser; expands Core_Lang with many builder rules and lexical tokens; refactors Noise and Surface grammars; implements numerous Go constructs for worldgen density functions, JSON export, traversal wiring, and updates vendor dependencies and tooling config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Parser
participant ANTLR as Grammar
participant Registry as ConstructRegistry
participant Scope
participant DF as DensityFn Constructors
participant Export as Exporter
User->>Parser: Parse(file)
Parser->>ANTLR: build AST (ScriptContext)
Parser-->>User: return ScriptContext
Parser->>Parser: ExitNamespace
loop for each contentBlock
Parser->>Registry: Construct(childPRC, namespace, scope)
alt densityFnBlock
Registry->>DF: build declarations & values
DF->>Scope: diagnose errors
DF-->>Registry: constructed density function
else other blocks
Registry-->>Registry: existing constructors
end
end
DF->>Export: exportDensityFunction(symbol, rootDir)
Export-->>DF: write JSON file
sequenceDiagram
autonumber
participant DF as DensityFn_Noise
participant NoiseDef as NoiseDefinition / ResourceRef
participant Inline as ExtractInlinedNoise
participant Chain as BuilderChain
DF->>NoiseDef: parse noise (inline or resource)
alt inline
DF->>Inline: register inline noise symbol
Inline-->>DF: traversal.Reference
else resource
DF-->>DF: use resource traversal.Reference
end
DF->>Chain: apply builders (XZScale, YScale, ShiftX/Y/Z, ...)
Chain-->>DF: populated fields
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (26)
wasm/mms.d.ts (1)
36-36: Prefer a genericvalueoverobjectto avoid breaking changes and improve typingNarrowing
unknown→objectcan break callers that stored primitives; a generic preserves flexibility with strong typing.Apply:
-export type MmsSymbol = { +export type MmsSymbol<T = unknown> = { nameLocation: MmsTextLocation, contentLocation: MmsTextLocation, - value: object, + value: T, ref: MmsReference type: string }Optionally, if this is always JSON-like, use
T = Record<string, unknown> | unknown[].grammar/Core_Lang.g4 (1)
7-7: LGTM; supports .5/- .5. Consider exponent support as a follow-upThe relaxed Float rule is good. If needed later, extend to allow scientific notation (e.g.,
1e-3,.5e2).test.mms (1)
1-6: Augment sample to exercise new float forms and non-default paramsAdd a case with decimals lacking a leading zero and some non-default args to validate the new Float rule and JSON output.
Example:
namespace xyz { DensityFn { old = OldBlendedNoise(xzScale = .25, yScale = -.5) c = Constant(-.125) } }lang/traversal/Construct.go (1)
49-50: No-op refactor; OK to keep for breakpointing/instrumentationAssigning to
valbefore return is functionally identical and can aid debugging.grammar/MinecraftMetascript.g4 (1)
3-3: Tiny formatting nit in import listAdd a space after the comma for consistency/readability.
-import Surface,DensityFunctions, Noise, Core_Lang; +import Surface, DensityFunctions, Noise, Core_Lang;lang/constructs/worldgen/surface_rules/SurfaceConditions.go (1)
5-5: Drop unused fmt import after switching to diagnosticsOnce stdout logging is removed, fmt can be removed from imports.
import ( "encoding/json" - "fmt" "reflect"lang/traversal/Symbol.go (1)
29-36: NewSymbol invocations updated
All existing usages (e.g. NoiseFn.go) now include the required_typeargument. Optional: introduce aNewTypedSymbolwrapper to centralize type‐setting logic.lang/traversal/Parser.go (3)
31-36: Remove or gate debug printsUnconditional stdout logs (“Parsing…”, “Done parsing”) are noisy in libraries/CLIs. Prefer a logger behind a verbosity flag or remove.
func (p *Parser) Parse() (err error) { - fmt.Println("Parsing...") if p.parser.Script() == nil { return errors.New("failed to parse") } - fmt.Println("Done parsing") return nil }
39-46: Add type/length guards to prevent a hard cast panicCasting inner to antlr.ParserRuleContext without checks can panic if grammar changes or listeners introduce non-rule children. Cheap guard improves resilience.
func (p *Parser) ExitNamespace(ctx *grammar.NamespaceContext) { for _, contentBlockCtx := range ctx.AllContentBlocks() { if contentBlock, ok := contentBlockCtx.(*grammar.ContentBlocksContext); ok { - inner := contentBlock.GetChild(0) - ConstructRegistry.Construct(inner.(antlr.ParserRuleContext), p.namespace, p.scope) + if contentBlock.GetChildCount() == 0 { + continue + } + if inner, ok := contentBlock.GetChild(0).(antlr.ParserRuleContext); ok { + ConstructRegistry.Construct(inner, p.namespace, p.scope) + } } } }
5-5: Drop fmt import if prints are removedKeep imports minimal.
import ( "errors" - "fmt" "github.com/minecraftmetascript/mms/lang/grammar"lang/constructs/worldgen/density_functions/NoiseFn.go (4)
25-27: Fix mislabeled builder label for YScale.The label string is "XzScale" while setting YScale; fix for clarity and consistent diagnostics.
- func(ctx *grammar.SharedBuilder_YScaleContext, target *NoiseDensityFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "XzScale") - }, + func(ctx *grammar.SharedBuilder_YScaleContext, target *NoiseDensityFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "YScale") + },
52-55: Avoid panicking on symbol registration failures.Panic during construction will take down the whole traversal; prefer recording a diagnostic and continuing or returning a sentinel construct.
If you point me to the scope’s diagnostic API, I can wire a non-panicking path.
67-73: Avoid potential panic on type assertion for builder child.Be defensive to avoid a crash if the child isn’t a ParserRuleContext.
- builder_chain.Invoke(noiseFnBuilder, child.(antlr.ParserRuleContext), out, scope, currentNamespace) + if ctx, ok := child.(antlr.ParserRuleContext); ok { + builder_chain.Invoke(noiseFnBuilder, ctx, out, scope, currentNamespace) + }
41-43: Consider de-duping inline reference ids across files.The id uses only line/column with a fixed namespace ("mms_inline"). If scopes span files, two files with the same coords can collide. Add a file-scoped salt (e.g., a hash of CurrentFile).
lang/constructs/worldgen/density_functions/ConstantFn.go (1)
33-39: Optional: use compact JSON for artifacts.If these files are for machine consumption, consider json.Marshal to reduce size; keep MarshalIndent if human-readability is preferred.
lang/constructs/worldgen/density_functions/FunctionRef.go (2)
53-55: Nit: correct error text.- return nil, errors.New("reference is nil in ReferenceRule") + return nil, errors.New("reference is nil in ReferenceFunction")
60-62: Nit: prefer fmt.Errorf over errors.New(fmt.Sprintf(...)).- return nil, errors.New( - fmt.Sprintf("unable to resolve reference %s", c.Ref.String()), - ) + return nil, fmt.Errorf("unable to resolve reference %s", c.Ref.String())lang/constructs/worldgen/density_functions/OldNoise.go (2)
25-26: Copy-paste label mistakes in Builder_GetFloat.Labels for diagnostics are all "XzScale". Align them to the actual field.
Apply:
- builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "XzScale") + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "YScale") @@ - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzFactor = v }, scope, "XzScale") + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzFactor = v }, scope, "XzFactor") @@ - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YFactor = v }, scope, "XzScale") + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YFactor = v }, scope, "YFactor") @@ - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.SmearMultiplier = v }, scope, "XzScale") + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.SmearMultiplier = v }, scope, "Smear")Also applies to: 30-31, 35-36, 40-41
56-62: Guard the type assertion on child to avoid panic.GetChild(0) isn’t guaranteed to be a ParserRuleContext.
Apply:
- builder_chain.Invoke(noiseFnBuilder, child.(antlr.ParserRuleContext), out, scope, currentNamespace) + if pctx, ok := child.(antlr.ParserRuleContext); ok { + builder_chain.Invoke(noiseFnBuilder, pctx, out, scope, currentNamespace) + } else { + scope.DiagnoseSemanticError("Invalid OldBlendedNoise builder child", ctx) + }lang/constructs/worldgen/density_functions/SingleInput.go (1)
37-72: Consider a lookup map for kind resolution.The long switch is fine but a map[string]SingleInputDensityFnKind would simplify maintenance.
lang/constructs/worldgen/density_functions/DualInput.go (2)
31-43: Kind resolution OK; consider shared helper with SingleInput.Both files duplicate token→kind mapping; a shared helper would reduce drift.
45-60: Minor consistency nit: explicit cast like SingleInput.Optional: cast DensityFn(i) to antlr.ParserRuleContext for symmetry with SingleInput.
Apply:
- val := traversal.ConstructRegistry.Construct(firstArg, currentNamespace, scope) + val := traversal.ConstructRegistry.Construct(firstArg.(antlr.ParserRuleContext), currentNamespace, scope)lang/constructs/worldgen/density_functions/DensityFn.go (1)
115-127: Consider adding error handling for MkDir and MkFile operationsThe directory and file creation operations don't check for errors. While the
FileTreeLikeinterface might handle errors internally, it's good practice to verify that these operations succeed.Additionally, consider extracting the hardcoded path structure to constants for better maintainability:
+const ( + worldgenDir = "worldgen" + densityFunctionDir = "density_function" +) + func exportDensityFunction(symbol traversal.Symbol, rootDir *lib.FileTreeLike, condition json.Marshaler) error { data, err := json.MarshalIndent(condition, "", " ") if err != nil { return err } rootDir. MkDir(symbol.GetReference().GetNamespace(), nil). - MkDir("worldgen", nil). - MkDir("density_function", nil). + MkDir(worldgenDir, nil). + MkDir(densityFunctionDir, nil). MkFile(symbol.GetReference().GetName()+".json", string(data), nil) return nil }grammar/DensityFunctions.g4 (3)
52-53: Inconsistent casing in cache kind rule nameThe rule
DensityFn_CacheKinduses PascalCase while other similar rules use camelCase. Consider maintaining consistent naming conventions.-DensityFn_CacheKind: '2d' | 'Once' | 'All'; -densityFn_Cache: 'Cache' NL* '(' NL* DensityFn_CacheKind NL* ',' NL* densityFn NL* ')'; +densityFn_CacheKind: '2d' | 'Once' | 'All'; +densityFn_Cache: 'Cache' NL* '(' NL* densityFn_CacheKind NL* ',' NL* densityFn NL* ')';
55-58: Leading OR operator in choice ruleThe
densityFn_DualInputrule has an unnecessary leading|operator which could be confusing.densityFn_DualInput: ( - | 'Min' + 'Min' | 'Max' ) NL* '(' NL* densityFn ',' NL* densityFn NL* ')';
77-84: Consider implementing TODO featuresThe comment block lists several unimplemented density function features. These appear to be important for complete worldgen support.
Would you like me to help implement any of these missing density function types or create issues to track them? The listed functions include:
- weird_scaled_sampler
- shifted_noise
- range_choice
- clamp
- spline
- y_clamped_gradient
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
lang/grammar/MinecraftMetascript.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascript.tokensis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.tokensis excluded by!lang/grammar/*lang/grammar/minecraftmetascript_base_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_lexer.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_parser.gois excluded by!lang/grammar/*
📒 Files selected for processing (18)
grammar/Core_Lang.g4(1 hunks)grammar/DensityFunctions.g4(1 hunks)grammar/MinecraftMetascript.g4(1 hunks)lang/Project.go(1 hunks)lang/constructs/worldgen/density_functions/ConstantFn.go(1 hunks)lang/constructs/worldgen/density_functions/DensityFn.go(1 hunks)lang/constructs/worldgen/density_functions/DualInput.go(1 hunks)lang/constructs/worldgen/density_functions/FunctionRef.go(1 hunks)lang/constructs/worldgen/density_functions/NoiseFn.go(1 hunks)lang/constructs/worldgen/density_functions/OldNoise.go(1 hunks)lang/constructs/worldgen/density_functions/SingleInput.go(1 hunks)lang/constructs/worldgen/noise/Noise.go(1 hunks)lang/constructs/worldgen/surface_rules/SurfaceConditions.go(2 hunks)lang/traversal/Construct.go(1 hunks)lang/traversal/Parser.go(2 hunks)lang/traversal/Symbol.go(1 hunks)test.mms(1 hunks)wasm/mms.d.ts(1 hunks)
🔇 Additional comments (8)
lang/Project.go (1)
11-11: Side-effect import for density functions looks rightInit-time registration via blank import is appropriate. Verify there’s no init-order coupling with noise/surface_rules.
grammar/MinecraftMetascript.g4 (1)
10-10: Density functions are now first-class content blocks — looks goodThe new alternative integrates cleanly with existing surface/noise blocks.
lang/constructs/worldgen/noise/Noise.go (1)
16-27: LGTM: recursive descent for NoiseBlockClean, minimal traversal that delegates to the registry for child rules. Matches the SurfaceBlock pattern.
lang/constructs/worldgen/density_functions/OldNoise.go (1)
68-74: Struct shape and JSON keys look correct.Fields and JSON tags match the expected xz_scale/y_scale/xz_factor/y_factor/smear_scale_multiplier.
Also applies to: 79-84
lang/constructs/worldgen/density_functions/SingleInput.go (1)
96-108: LGTM on JSON and export integration.Emits {type, argument} and delegates export correctly.
lang/constructs/worldgen/density_functions/DualInput.go (1)
80-94: LGTM on JSON structure.Uses {type, argument1, argument2} as expected.
lang/constructs/worldgen/density_functions/DensityFn.go (1)
26-36: LGTM! Clean JSON marshaling implementationThe custom MarshalJSON implementation properly formats the output with appropriate field names and indentation.
grammar/DensityFunctions.g4 (1)
14-23: LGTM! Well-structured density function grammarThe grammar structure is clean and modular, with appropriate use of optional math operations and clear separation of different density function types.
| import ( | ||
| "encoding/json" | ||
| "reflect" | ||
|
|
||
| "github.com/antlr4-go/antlr/v4" | ||
| "github.com/minecraftmetascript/mms/lang/builder_chain" | ||
| "github.com/minecraftmetascript/mms/lang/grammar" | ||
| "github.com/minecraftmetascript/mms/lang/traversal" | ||
| "github.com/minecraftmetascript/mms/lib" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reject NaN/Inf constants before marshaling.
encoding/json can’t marshal NaN/Inf; guard to produce a clear error.
import (
"encoding/json"
+ "fmt"
+ "math"
"reflect"
@@
func (c ConstantDensityFn) MarshalJSON() ([]byte, error) {
+ if math.IsNaN(c.Value) || math.IsInf(c.Value, 0) {
+ return nil, fmt.Errorf("invalid constant value: %v", c.Value)
+ }
return json.MarshalIndent(struct {
Type string `json:"type"`
Argument float64 `json:"argument"`Also applies to: 32-40
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/ConstantFn.go around lines 3-12
and 32-40, the code may attempt to json.Marshal NaN/Inf which encoding/json
cannot marshal; add a guard that checks the constant float64 using math.IsNaN
and math.IsInf (import math) before marshaling and return or propagate a clear
error (e.g. "constant value is NaN/Inf") instead of attempting to marshal; apply
the same validation in both places where the constant is serialized so callers
receive a meaningful error rather than a panic from json.Marshal.
| var kind MathDensityFnKind | ||
| if operation := math.GetChild(0); operation != nil { | ||
| switch operation.(antlr.TerminalNode).GetText() { | ||
| case "*": | ||
| kind = MathDensityFn_Mul | ||
| case "+": | ||
| kind = MathDensityFn_Add | ||
| default: | ||
| scope.DiagnoseSemanticError("Unknown operator", ctx) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil check for GetChild(0) result
The code casts operation to antlr.TerminalNode without checking if the cast is successful. This could panic if the child is not a terminal node.
var kind MathDensityFnKind
if operation := math.GetChild(0); operation != nil {
- switch operation.(antlr.TerminalNode).GetText() {
+ if termNode, ok := operation.(antlr.TerminalNode); ok {
+ switch termNode.GetText() {
case "*":
kind = MathDensityFn_Mul
case "+":
kind = MathDensityFn_Add
default:
scope.DiagnoseSemanticError("Unknown operator", ctx)
}
+ } else {
+ scope.DiagnoseSemanticError("Invalid operator node", ctx)
+ }
}📝 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.
| var kind MathDensityFnKind | |
| if operation := math.GetChild(0); operation != nil { | |
| switch operation.(antlr.TerminalNode).GetText() { | |
| case "*": | |
| kind = MathDensityFn_Mul | |
| case "+": | |
| kind = MathDensityFn_Add | |
| default: | |
| scope.DiagnoseSemanticError("Unknown operator", ctx) | |
| } | |
| } | |
| var kind MathDensityFnKind | |
| if operation := math.GetChild(0); operation != nil { | |
| if termNode, ok := operation.(antlr.TerminalNode); ok { | |
| switch termNode.GetText() { | |
| case "*": | |
| kind = MathDensityFn_Mul | |
| case "+": | |
| kind = MathDensityFn_Add | |
| default: | |
| scope.DiagnoseSemanticError("Unknown operator", ctx) | |
| } | |
| } else { | |
| scope.DiagnoseSemanticError("Invalid operator node", ctx) | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/DensityFn.go around lines 92 to
102, the code assumes math.GetChild(0) is an antlr.TerminalNode and casts
without checking which can panic; change the logic to perform a safe type
assertion (e.g., node, ok := operation.(antlr.TerminalNode)) and only call
GetText() when ok is true, otherwise call scope.DiagnoseSemanticError("Expected
terminal node for operator", ctx) (or set a sensible default/return) so
non-terminal children are handled safely.
| out.Ref.SetResolver(func() error { | ||
| if next, ok := scope.Get(*r); ok { | ||
| val := next.GetValue().(traversal.Construct) | ||
|
|
||
| out.Value = &val | ||
| } | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make resolver robust (missing symbol/type mismatch).
The current type assertion can panic, and missing symbols return nil error. Return precise errors instead.
- out.Ref.SetResolver(func() error {
- if next, ok := scope.Get(*r); ok {
- val := next.GetValue().(traversal.Construct)
-
- out.Value = &val
- }
- return nil
- })
+ out.Ref.SetResolver(func() error {
+ next, ok := scope.Get(*r)
+ if !ok {
+ return fmt.Errorf("reference not found: %s", r.String())
+ }
+ val, ok := next.GetValue().(traversal.Construct)
+ if !ok {
+ return fmt.Errorf("symbol %s is not a density function", r.String())
+ }
+ out.Value = &val
+ 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.
| out.Ref.SetResolver(func() error { | |
| if next, ok := scope.Get(*r); ok { | |
| val := next.GetValue().(traversal.Construct) | |
| out.Value = &val | |
| } | |
| return nil | |
| }) | |
| } | |
| out.Ref.SetResolver(func() error { | |
| next, ok := scope.Get(*r) | |
| if !ok { | |
| return fmt.Errorf("reference not found: %s", r.String()) | |
| } | |
| val, ok := next.GetValue().(traversal.Construct) | |
| if !ok { | |
| return fmt.Errorf("symbol %s is not a density function", r.String()) | |
| } | |
| out.Value = &val | |
| return nil | |
| }) |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/FunctionRef.go around lines 31-39,
the resolver currently uses a direct type assertion that can panic and always
returns nil on missing symbols; change it to check scope.Get(*r) and then
perform a safe type assertion (v, ok := next.GetValue().(traversal.Construct)),
returning a descriptive error when the symbol is missing or the type assertion
fails (e.g., fmt.Errorf("symbol %q not found", *r) or fmt.Errorf("expected
traversal.Construct for %q, got %T", *r, next.GetValue())), and only assign
out.Value when the assertion succeeds. Ensure the resolver returns the specific
error instead of nil so callers can handle lookup/type errors.
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "reflect" | ||
|
|
||
| "github.com/antlr4-go/antlr/v4" | ||
| "github.com/minecraftmetascript/mms/lang/builder_chain" | ||
| "github.com/minecraftmetascript/mms/lang/grammar" | ||
| "github.com/minecraftmetascript/mms/lang/traversal" | ||
| "github.com/minecraftmetascript/mms/lib" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing noise reference during JSON export.
If no inline noise or resource ref is provided, JSON will contain an empty noise id. Fail fast with a clear error.
import (
"encoding/json"
+ "errors"
"fmt"
"reflect"
@@
func (c NoiseDensityFn) MarshalJSON() ([]byte, error) {
+ if c.Noise.String() == "" {
+ return nil, errors.New("noise reference is empty in NoiseDensityFn")
+ }
return json.MarshalIndent(struct {
Type string `json:"type"`
Noise string `json:"noise"`Also applies to: 86-98
| YFactor float64 `json:"y_factor"` | ||
| Smear float64 `json:"smear_scale_multiplier"` | ||
| }{ | ||
| Type: "minecraft:noise", |
There was a problem hiding this comment.
Wrong JSON type for OldBlendedNoise.
Should be minecraft:old_blended_noise, not minecraft:noise.
Apply:
- Type: "minecraft:noise",
+ Type: "minecraft:old_blended_noise",🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/OldNoise.go around line 85, the
JSON type for OldBlendedNoise is incorrectly set to "minecraft:noise"; change
the Type value to "minecraft:old_blended_noise" so the emitted JSON uses the
correct discriminator for OldBlendedNoise entries; update the string literal and
run any formatter/tests to ensure the change compiles and serializes as
expected.
| traversal.ConstructRegistry.Register( | ||
| reflect.TypeFor[*grammar.SurfaceBlockContext](), | ||
| func(ctx antlr.ParserRuleContext, currentNamespace string, scope *traversal.Scope) traversal.Construct { | ||
| blockCtx := ctx.(*grammar.SurfaceBlockContext) | ||
| for _, statementCtx := range blockCtx.GetChildren() { | ||
| statement, ok := statementCtx.(antlr.ParserRuleContext) | ||
| if !ok { | ||
| // TODO: Diagnose? | ||
| continue | ||
| } | ||
| targetCtx, ok := statement.GetChild(0).(antlr.ParserRuleContext) | ||
| if !ok { | ||
| // TODO: Diagnose? | ||
| continue | ||
| } | ||
|
|
||
| if traversal.ConstructRegistry.Construct(targetCtx, currentNamespace, scope) == nil { | ||
| fmt.Println("Failed to construct ", ctx.GetText(), reflect.TypeOf(targetCtx).Elem().Name()) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential panic + diagnostics to stdout — guard child access and use the diagnostics sink
- Line 43 calls GetChild(0) without checking GetChildCount(); this can panic on empty nodes.
- Lines 49–51 print to stdout. Route failures through scope.DiagnoseSemanticError to surface them uniformly.
Apply:
func init() {
traversal.ConstructRegistry.Register(
reflect.TypeFor[*grammar.SurfaceBlockContext](),
func(ctx antlr.ParserRuleContext, currentNamespace string, scope *traversal.Scope) traversal.Construct {
blockCtx := ctx.(*grammar.SurfaceBlockContext)
for _, statementCtx := range blockCtx.GetChildren() {
statement, ok := statementCtx.(antlr.ParserRuleContext)
if !ok {
// TODO: Diagnose?
continue
}
- targetCtx, ok := statement.GetChild(0).(antlr.ParserRuleContext)
+ if statement.GetChildCount() == 0 {
+ // TODO: Diagnose?
+ continue
+ }
+ child := statement.GetChild(0)
+ targetCtx, ok := child.(antlr.ParserRuleContext)
if !ok {
// TODO: Diagnose?
continue
}
- if traversal.ConstructRegistry.Construct(targetCtx, currentNamespace, scope) == nil {
- fmt.Println("Failed to construct ", ctx.GetText(), reflect.TypeOf(targetCtx).Elem().Name())
- }
+ if traversal.ConstructRegistry.Construct(targetCtx, currentNamespace, scope) == nil {
+ scope.DiagnoseSemanticError(
+ "Failed to construct "+reflect.TypeOf(targetCtx).Elem().Name(),
+ targetCtx,
+ )
+ }
}
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.
| traversal.ConstructRegistry.Register( | |
| reflect.TypeFor[*grammar.SurfaceBlockContext](), | |
| func(ctx antlr.ParserRuleContext, currentNamespace string, scope *traversal.Scope) traversal.Construct { | |
| blockCtx := ctx.(*grammar.SurfaceBlockContext) | |
| for _, statementCtx := range blockCtx.GetChildren() { | |
| statement, ok := statementCtx.(antlr.ParserRuleContext) | |
| if !ok { | |
| // TODO: Diagnose? | |
| continue | |
| } | |
| targetCtx, ok := statement.GetChild(0).(antlr.ParserRuleContext) | |
| if !ok { | |
| // TODO: Diagnose? | |
| continue | |
| } | |
| if traversal.ConstructRegistry.Construct(targetCtx, currentNamespace, scope) == nil { | |
| fmt.Println("Failed to construct ", ctx.GetText(), reflect.TypeOf(targetCtx).Elem().Name()) | |
| } | |
| } | |
| return nil | |
| }, | |
| ) | |
| func init() { | |
| traversal.ConstructRegistry.Register( | |
| reflect.TypeFor[*grammar.SurfaceBlockContext](), | |
| func(ctx antlr.ParserRuleContext, currentNamespace string, scope *traversal.Scope) traversal.Construct { | |
| blockCtx := ctx.(*grammar.SurfaceBlockContext) | |
| for _, statementCtx := range blockCtx.GetChildren() { | |
| statement, ok := statementCtx.(antlr.ParserRuleContext) | |
| if !ok { | |
| // TODO: Diagnose? | |
| continue | |
| } | |
| if statement.GetChildCount() == 0 { | |
| // TODO: Diagnose? | |
| continue | |
| } | |
| child := statement.GetChild(0) | |
| targetCtx, ok := child.(antlr.ParserRuleContext) | |
| if !ok { | |
| // TODO: Diagnose? | |
| continue | |
| } | |
| if traversal.ConstructRegistry.Construct(targetCtx, currentNamespace, scope) == nil { | |
| scope.DiagnoseSemanticError( | |
| "Failed to construct "+reflect.TypeOf(targetCtx).Elem().Name(), | |
| targetCtx, | |
| ) | |
| } | |
| } | |
| return nil | |
| }, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/surface_rules/SurfaceConditions.go around lines 33
to 55, the code calls statement.GetChild(0) without checking GetChildCount()
(risking a panic on empty nodes) and prints errors to stdout; update the code to
first check statement.GetChildCount() > 0 before accessing child 0, handle the
unexpected shape by calling scope.DiagnoseSemanticError with a clear message
(including ctx.GetText() and the target node type) instead of fmt.Println, and
remove or replace the TODO comments with this diagnostics path so failures are
reported uniformly.
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grammar/Surface.g4 (1)
85-88: Enable legacy Block and Sequence forms in grammar/Surface.g4Update the surfaceRule_Block and surfaceRule_Sequence rules to accept both the new and legacy syntax:
-surfaceRule_Block: 'Block' '(' resourceReference ')'; -surfaceRule_Sequence: '[' NL* (surfaceRule NL*)* NL* ']'; +surfaceRule_Block: 'Block' NL* ( '(' NL* resourceReference NL* ')' | resourceReference ) NL*; +surfaceRule_Sequence: ('Sequence' NL*)? '[' NL* (surfaceRule NL*)* NL* ']';Run the surface-rule parser tests to ensure no regressions and that both
Block(resource)andBlock resourceas well asSequence […]and[…]parse successfully.
♻️ Duplicate comments (2)
lang/constructs/worldgen/density_functions/DensityFn.go (2)
104-105: Remove debug print; rely on diagnosticsThis leftover print leaks into stdout.
- fmt.Println("Failed to process declaration") return nil
126-146: Math operator handling: validate Arg2, safe operator cast, and graceful fallbackAvoid panics, diagnose issues, and return base value when math is invalid.
- if math := fnCtx.DensityFn_Math(); math != nil { + if math := fnCtx.DensityFn_Math(); math != nil { // This has attached maths - arg2 := traversal.ConstructRegistry.Construct(math.DensityFn(), currentNamespace, scope) + arg2 := traversal.ConstructRegistry.Construct(math.DensityFn(), currentNamespace, scope) + if arg2 == nil { + scope.DiagnoseSemanticError("Missing or invalid second operand for math density function", math) + return value + } var kind MathDensityFnKind - if operation := math.GetChild(0); operation != nil { - switch operation.(antlr.TerminalNode).GetText() { + if operation := math.GetChild(0); operation != nil { + if term, ok := operation.(antlr.TerminalNode); ok { + switch term.GetText() { case "*": kind = MathDensityFn_Mul case "+": kind = MathDensityFn_Add default: scope.DiagnoseSemanticError("Unknown operator", ctx) + return value } + } else { + scope.DiagnoseSemanticError("Invalid operator node", ctx) + return value } return &MathDensityFn{ Type: kind, Arg1: value, Arg2: arg2, } }
🧹 Nitpick comments (19)
lang/constructs/worldgen/noise/Noise.go (1)
61-79: Improve amplitude parsing UX and pre-size the slice
- Include the offending token in the diagnostic to aid debugging.
- Optionally pre-size the amplitudes slice to avoid reallocations for large lists.
Apply this diff for clearer diagnostics:
- if err != nil { - scope.DiagnoseSemanticError( - "Invalid amplitude value", - amplitude, - ) + if err != nil { + scope.DiagnoseSemanticError( + "Invalid amplitude value: "+amplitude.GetText(), + amplitude, + )Optional pre-sizing (micro-optimization):
- out := &Noise{ - Amplitudes: make([]float64, 0), - } + // Pre-size amplitudes to reduce reallocations. + totalAmps := 0 + for _, r := range noise.AllNoise_Builder() { + if amps := r.Builder_Amplitudes(); amps != nil { + totalAmps += len(amps.AllNumber()) + } + } + out := &Noise{ + Amplitudes: make([]float64, 0, totalAmps), + }Optional guard after the loop:
if len(out.Amplitudes) == 0 { scope.DiagnoseSemanticError("Noise must define at least one amplitude", noise) }lang/builder_chain/SharedBuilder.go (1)
31-31: Rename helper to match new builder_ naming*Now that it accepts Builder_AddContext, consider renaming SharedBuilder_Add → Builder_AddFlag (or similar) to avoid mixed conventions.
lang/constructs/worldgen/surface_rules/Condition_Noise.go (1)
56-65: Add semantic guard: require Min <= MaxPrevent invalid ranges early with a diagnostic.
Apply this diff right after the existing Require checks:
builder_chain.Require( noiseBuildChain, noise, scope, reflect.TypeFor[*grammar.Builder_MaxContext](), ".Max", ) + if out.Min > out.Max { + scope.DiagnoseSemanticError("Min must be <= Max", noise) + }lang/constructs/worldgen/density_functions/ClampFn.go (2)
18-29: Rename builder var and fix user-facing diagnostics ("range choice" → "clamp").Minor naming drift and copy/paste error in messages. Tighten for clarity.
- rangeChoiceBuilder := builder_chain.NewBuilderChain[ClampDensityFn]( + clampBuilder := builder_chain.NewBuilderChain[ClampDensityFn]( @@ - for _, builderWrap := range densityFn.AllDensityFn_ClampBuilder() { - builder_chain.Invoke(rangeChoiceBuilder, builderWrap.GetChild(0).(antlr.ParserRuleContext), out, scope, currentNamespace) - } + for _, builderWrap := range densityFn.AllDensityFn_ClampBuilder() { + builder_chain.Invoke(clampBuilder, builderWrap.GetChild(0).(antlr.ParserRuleContext), out, scope, currentNamespace) + } @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + clampBuilder, @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + clampBuilder,- if input == nil { - scope.DiagnoseSemanticError("Missing input to range choice", ctx) - } + if input == nil { + scope.DiagnoseSemanticError("Missing input to clamp", ctx) + } @@ - if out.Input == nil { - scope.DiagnoseSemanticError("Invalid input to range choice", ctx) - } + if out.Input == nil { + scope.DiagnoseSemanticError("Invalid input to clamp", ctx) + }Also applies to: 42-44, 46-59
46-59: Optional: validate clamp bounds.Warn/error if Min > Max to catch obvious config mistakes.
builder_chain.Require( clampBuilder, densityFn, scope, reflect.TypeFor[*grammar.Builder_MaxContext](), ".Max", ) + if out.Min > out.Max { + scope.DiagnoseSemanticError("Clamp.Min cannot be greater than Clamp.Max", densityFn) + }lang/constructs/worldgen/density_functions/YClampedGradientFn.go (2)
18-39: Rename builder var for accuracy; keep messages consistent.Pure readability; aligns with the construct’s name.
- rangeChoiceBuilder := builder_chain.NewBuilderChain[YClampedGradientDensityFn]( + yClampedGradientBuilder := builder_chain.NewBuilderChain[YClampedGradientDensityFn]( @@ - for _, builderWrap := range densityFn.AllDensityFn_YClampedGradientBuilder() { - builder_chain.Invoke(rangeChoiceBuilder, builderWrap.GetChild(0).(antlr.ParserRuleContext), out, scope, currentNamespace) - } + for _, builderWrap := range densityFn.AllDensityFn_YClampedGradientBuilder() { + builder_chain.Invoke(yClampedGradientBuilder, builderWrap.GetChild(0).(antlr.ParserRuleContext), out, scope, currentNamespace) + } @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + yClampedGradientBuilder, @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + yClampedGradientBuilder, @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + yClampedGradientBuilder, @@ - builder_chain.Require( - rangeChoiceBuilder, + builder_chain.Require( + yClampedGradientBuilder,Also applies to: 44-46, 48-75
48-75: Optional: validate ranges (Bottom <= Top, Min <= Max).Prevents inverted gradients and surprising outputs.
builder_chain.Require( yClampedGradientBuilder, densityFn, scope, reflect.TypeFor[*grammar.Builder_TopLiteralContext](), ".Top", ) + if out.Bottom > out.Top { + scope.DiagnoseSemanticError("YClampedGradient.Bottom cannot exceed .Top", densityFn) + } + if out.Min > out.Max { + scope.DiagnoseSemanticError("YClampedGradient.Min cannot exceed .Max", densityFn) + }lang/constructs/worldgen/density_functions/SingleInput.go (3)
25-27: Confirm schema for Shift/ShiftA/ShiftB — likely need extra fields beyond just "argument".If these map to Minecraft’s shift functions, they typically require noise inputs in addition to the child argument. Emitting only {type, argument} may produce invalid JSON. Please verify against the target game version and either:
- split these into dedicated structs with required noise fields, or
- extend this type to carry the extra fields conditionally.
65-66: Include offending token in the diagnostic.Improves debuggability by showing what was parsed.
- scope.DiagnoseSemanticError("Invalid density function kind", ctx) + scope.DiagnoseSemanticError("Invalid density function kind: "+k.GetText(), ctx)
76-83: Use “Invalid … target” when construction fails.Differentiate missing vs. failed construction to guide users better.
- if val == nil { - scope.DiagnoseSemanticError("Missing density function target", ctx) + if val == nil { + scope.DiagnoseSemanticError("Invalid density function target", ctx)lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go (1)
15-17: Spelling of context type (“Wierd”) — confirm grammar rule name.If the grammar is later corrected to “Weird…Context”, this registration will break. Consider adding a second registration when/if the rule gets renamed to maintain compatibility.
lang/constructs/worldgen/density_functions/OldNoiseFn.go (1)
20-42: Incorrect labels passed to Builder_GetFloat.All labels are “XzScale”; fix to reflect the actual fields to improve diagnostics.
- func(ctx *grammar.Builder_XZScaleContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzScale = v }, scope, "XzScale") + func(ctx *grammar.Builder_XZScaleContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzScale = v }, scope, "XzScale") }, ), builder_chain.Build( - func(ctx *grammar.Builder_YScaleContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "XzScale") + func(ctx *grammar.Builder_YScaleContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YScale = v }, scope, "YScale") }, ), builder_chain.Build( - func(ctx *grammar.Builder_XZFactorContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzFactor = v }, scope, "XzScale") + func(ctx *grammar.Builder_XZFactorContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.XzFactor = v }, scope, "XzFactor") }, ), builder_chain.Build( - func(ctx *grammar.Builder_YFactorContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YFactor = v }, scope, "XzScale") + func(ctx *grammar.Builder_YFactorContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.YFactor = v }, scope, "YFactor") }, ), builder_chain.Build( - func(ctx *grammar.Builder_SmearContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { - builder_chain.Builder_GetFloat(ctx, func(v float64) { target.SmearMultiplier = v }, scope, "XzScale") + func(ctx *grammar.Builder_SmearContext, target *OldBlendedNoiseFn, scope *traversal.Scope, namespace string) { + builder_chain.Builder_GetFloat(ctx, func(v float64) { target.SmearMultiplier = v }, scope, "SmearScaleMultiplier") }, ),lang/constructs/worldgen/density_functions/RangeChoiceFn.go (1)
18-49: Use the passed namespace param inside builders for consistencyThe builders receive
namespace stringbut use the outercurrentNamespace. Prefer the param to avoid accidental capture bugs.Example:
- fn := traversal.ConstructRegistry.Construct(ctx.DensityFn(), currentNamespace, scope) + fn := traversal.ConstructRegistry.Construct(ctx.DensityFn(), namespace, scope)Apply similarly in both InRange and OutRange builders.
lang/constructs/worldgen/density_functions/DensityFn.go (1)
75-79: Optional: avoid hard cast on NoiseDefinitionType-asserting
NoiseDefinition()directly can panic if grammar changes. Consider a checked assertion.- if noiseInlineCtx := inlineCtx.Noise(); noiseInlineCtx != nil { - return ExtractInlinedNoise(noiseInlineCtx.NoiseDefinition().(*grammar.NoiseDefinitionContext), currentNamespace, scope) + if noiseInlineCtx := inlineCtx.Noise(); noiseInlineCtx != nil { + if nd, ok := noiseInlineCtx.NoiseDefinition().(*grammar.NoiseDefinitionContext); ok { + return ExtractInlinedNoise(nd, currentNamespace, scope) + } + scope.DiagnoseSemanticError("Invalid inline noise definition node", inlineCtx) }grammar/Surface.g4 (1)
69-74: Retain legacy “AboveWater 2 1.5 Add” form (optional)Current rule requires
AboveWater()before builders; older tests/examples omit(). To avoid breaking, allow both.-surfaceCondition_AboveWater: 'AboveWater' '(' ')' NL* (surfaceCondition_AboveWaterBuilder NL*)*; +surfaceCondition_AboveWater: + ('AboveWater' '(' ')' | 'AboveWater') + NL* (surfaceCondition_AboveWaterBuilder NL*)*;grammar/DensityFunctions.g4 (2)
71-76: Fix typo: Wierd → Weird (rule names).Rule names use “Wierd” while the literal is “WeirdScaledSampler”. Standardize to “Weird” for consistency and to avoid confusing generated context type names.
Be sure to update any Go handlers that reference the generated context names.
-densityFn_WierdScaledSampler: 'WeirdScaledSampler' NL* '(' NL* densityFn NL* ')' NL* (densityFn_WierdScaledSamplerBuilder NL*)*; +densityFn_WeirdScaledSampler: 'WeirdScaledSampler' NL* '(' NL* densityFn NL* ')' NL* (densityFn_WeirdScaledSamplerBuilder NL*)*; -densityFn_WierdScaledSamplerBuilder: builder_Type1 | builder_Type2 | builder_Noise; +densityFn_WeirdScaledSamplerBuilder: builder_Type1 | builder_Type2 | builder_Noise;
8-24: Allow chaining math operations; current grammar permits only one trailing op.
densityFn_Math?limits expressions to a single+/*. If chaining is desired (a + b + c,a * b * c), switch to repetition and allow optional NL between operator and rhs.-) densityFn_Math?; +) (densityFn_Math)*; @@ -densityFn_Math: ('+' | '*') densityFn; +densityFn_Math: ('+' | '*') NL* densityFn;Also applies to: 93-93
lang/constructs/worldgen/density_functions/SplineFn.go (2)
31-35: Remove debug print leaking to stdout.
fmt.Println(locationCtx)is leftover debug noise.- fmt.Println(locationCtx)
100-105: Validate non-empty spline points.Empty
Pointsproduces a valid-looking JSON but unusable spline. Consider erroring if no points were added.I can wire a check after the loop that diagnoses an error when
len(out.Points)==0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
lang/grammar/MinecraftMetascript.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascript.tokensis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.tokensis excluded by!lang/grammar/*lang/grammar/minecraftmetascript_base_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_lexer.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_parser.gois excluded by!lang/grammar/*
📒 Files selected for processing (24)
grammar/Core_Lang.g4(1 hunks)grammar/DensityFunctions.g4(1 hunks)grammar/Noise.g4(1 hunks)grammar/Surface.g4(2 hunks)lang/builder_chain/BuilderChain.go(1 hunks)lang/builder_chain/SharedBuilder.go(1 hunks)lang/constructs/worldgen/density_functions/ClampFn.go(1 hunks)lang/constructs/worldgen/density_functions/DensityFn.go(1 hunks)lang/constructs/worldgen/density_functions/NoiseFn.go(1 hunks)lang/constructs/worldgen/density_functions/OldNoiseFn.go(1 hunks)lang/constructs/worldgen/density_functions/RangeChoiceFn.go(1 hunks)lang/constructs/worldgen/density_functions/ShiftedNoiseFn.go(1 hunks)lang/constructs/worldgen/density_functions/SingleInput.go(1 hunks)lang/constructs/worldgen/density_functions/SplineFn.go(1 hunks)lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go(1 hunks)lang/constructs/worldgen/density_functions/YClampedGradientFn.go(1 hunks)lang/constructs/worldgen/noise/Noise.go(1 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(1 hunks)lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go(1 hunks)lang/constructs/worldgen/surface_rules/Condition_YAbove.go(1 hunks)lang/traversal/Symbol.go(2 hunks)test.mms(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test.mms
- lang/traversal/Symbol.go
- lang/constructs/worldgen/density_functions/NoiseFn.go
🧰 Additional context used
🧬 Code graph analysis (16)
lang/constructs/worldgen/density_functions/YClampedGradientFn.go (6)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (4)
NewBuilderChain(76-85)Build(63-74)Invoke(20-42)Require(44-54)lang/builder_chain/SharedBuilder.go (2)
Builder_GetInt(39-59)Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/density_functions/RangeChoiceFn.go (6)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (4)
NewBuilderChain(76-85)Build(63-74)Invoke(20-42)Require(44-54)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/surface_rules/Condition_Noise.go (2)
lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/builder_chain/BuilderChain.go (2)
Build(63-74)Require(44-54)
lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go (3)
lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/SharedBuilder.go (1)
Builder_GetInt(39-59)lang/builder_chain/BuilderChain.go (1)
Build(63-74)
lang/constructs/worldgen/surface_rules/Condition_YAbove.go (2)
lang/builder_chain/SharedBuilder.go (1)
Builder_GetInt(39-59)lang/builder_chain/BuilderChain.go (1)
Build(63-74)
lang/constructs/worldgen/surface_rules/Condition_AboveWater.go (2)
lang/builder_chain/SharedBuilder.go (2)
Builder_GetInt(39-59)SharedBuilder_Add(30-37)lang/builder_chain/BuilderChain.go (1)
Build(63-74)
lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go (2)
lang/builder_chain/SharedBuilder.go (1)
Builder_GetVerticalAnchor(84-107)lang/builder_chain/BuilderChain.go (1)
Build(63-74)
lang/constructs/worldgen/density_functions/ClampFn.go (6)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (4)
NewBuilderChain(76-85)Build(63-74)Invoke(20-42)Require(44-54)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/density_functions/SingleInput.go (5)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)lang/constructs/worldgen/surface_rules/Rule_Sequence.go (1)
Type(47-50)
lang/constructs/worldgen/density_functions/ShiftedNoiseFn.go (5)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(76-85)Build(63-74)Invoke(20-42)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)
lang/constructs/worldgen/density_functions/SplineFn.go (5)
lang/traversal/Scope.go (1)
Scope(11-16)lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/density_functions/OldNoiseFn.go (6)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/builder_chain/BuilderChain.go (3)
NewBuilderChain(76-85)Build(63-74)Invoke(20-42)lang/builder_chain/SharedBuilder.go (1)
Builder_GetFloat(61-82)lang/traversal/Symbol.go (1)
Symbol(9-15)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go (5)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Scope.go (1)
Scope(11-16)lang/traversal/Reference.go (1)
Reference(30-36)lang/constructs/worldgen/density_functions/DensityFn.go (1)
ExtractInlinedNoise(42-62)wasm/mms.d.ts (1)
FileTreeLike(10-13)
lang/constructs/worldgen/noise/Noise.go (4)
lang/traversal/Construct.go (2)
ConstructRegistry(55-57)Construct(12-16)lang/traversal/Symbol.go (1)
ProcessDeclaration(44-69)lang/builder_chain/SharedBuilder.go (1)
Builder_GetInt(39-59)lang/constructs/worldgen/surface_rules/SurfaceConditions.go (1)
init(31-52)
lang/constructs/worldgen/density_functions/DensityFn.go (5)
lang/traversal/Construct.go (2)
Construct(12-16)ConstructRegistry(55-57)lang/traversal/Symbol.go (3)
Symbol(9-15)NewSymbol(29-37)ProcessDeclaration(44-69)wasm/mms.d.ts (1)
FileTreeLike(10-13)lang/traversal/Reference.go (2)
Reference(30-36)NewReference(38-43)lang/traversal/Location.go (1)
RuleLocation(42-57)
grammar/Surface.g4 (2)
lang/surface_rules_additional_test.go (2)
TestSurfaceRule_Sequence_ExportJSON(8-54)TestSurfaceRule_If_AboveWater_JSON(56-104)lang/parsing_and_export_test.go (1)
TestParseValidSurfaceRule_Block_AndExport(19-57)
🔇 Additional comments (19)
lang/constructs/worldgen/noise/Noise.go (1)
16-27: LGTM: traversal-only registration for NoiseBlockContext is correctRecurses into child ParserRuleContexts and intentionally returns nil. This matches the pattern elsewhere for block/aggregate nodes.
lang/builder_chain/BuilderChain.go (1)
26-26: Remove noisy stdout print from Invoke
Confirmed this is the onlyfmt.Println(and the soleos.Stdout/os.Stderrreference) in the repo; remove it or guard it behind a debug logger/flag.
Apply this diff:- fmt.Println(ctxType.Name())lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go (1)
26-29: LGTM: builder context type migration is correctType updates to Builder_OffsetContext and Builder_AddContext align with grammar changes; logic unchanged.
Also applies to: 31-34
lang/constructs/worldgen/surface_rules/Condition_YAbove.go (1)
22-25: *LGTM: Builder_Context updatesMulInt/Add closures correctly migrated; behavior preserved.
Also applies to: 27-30
lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go (1)
22-25: LGTM: Top/Bottom builder migrationsConcrete Builder_TopContext/Builder_BottomContext usage matches grammar; extraction helpers remain correct.
Also applies to: 27-30
lang/constructs/worldgen/surface_rules/Condition_Noise.go (1)
21-24: LGTM: Min/Max builder type updatesSwitch to Builder_MinContext/Builder_MaxContext is consistent with Core_Lang.g4.
Also applies to: 26-29
grammar/Noise.g4 (2)
8-12: LGTM: modularize with noiseDefinition and builder_AmplitudesRefactor keeps observable grammar intact and aligns with builder_* rules.
8-12: Rename verification successful
No stale references tonoise_Builder_Amplitudeswere found;builder_Amplitudesremains correctly defined and consumed.lang/constructs/worldgen/surface_rules/Condition_AboveWater.go (1)
21-24: LGTM: builder context renames and value extractionOffset/Add/Mul migrations are consistent; parsing helpers used correctly.
Also applies to: 26-29, 31-39
lang/constructs/worldgen/density_functions/YClampedGradientFn.go (1)
88-102: JSON field mapping LGTM.Tags correctly map to Minecraft schema: from_value/to_value and from_y/to_y.
lang/constructs/worldgen/density_functions/RangeChoiceFn.go (1)
123-125: LGTM: export path integrationExport hooks correctly delegate to shared exporter.
grammar/Surface.g4 (5)
45-46: Builder switch to builder_Max/Min looks goodConsistent with Core_Lang builder_* scheme.
54-56: StoneDepth builders updated correctlyOffset/Add via builder_* aligns with the new shared builders.
63-65: VerticalGradient builder update is consistentTop/Bottom via builder_* is okay.
71-73: AboveWater builders aligned with builder_ tokens*OK. Consider backward compatibility for syntax (see next comment).
76-76: YAbove builders updatedMulInt/Add via builder_* is fine.
lang/constructs/worldgen/density_functions/SplineFn.go (1)
69-75: No changes needed. go.mod declares Go 1.23, which already supports reflect.TypeFor (added in Go 1.20).grammar/Core_Lang.g4 (2)
54-56: Literal ‘2d’ token vs Int/Identifier—double-check lexer precedence.Using
'2d'as a literal alongsideIntandIdentifiercan be tricky in ANTLR. It usually works, but verify generation produces a single token for'2d'and notInt '2'+Identifier 'd'.I recommend running the grammar generation and ensuring no token ambiguity/warnings are emitted for
DensityFn_CacheKind.
17-18: Remove unreachable-alt suggestion: builder_MulInt is reachable
builder_MulInt is a separate parser rule invoked by surfaceCondition_YAboveBuilder in Surface.g4:76; it isn’t shadowed by builder_Mul and should be retained.Likely an incorrect or invalid review comment.
| @@ -1,10 +1,38 @@ | |||
| grammar Core_Lang; | |||
|
|
|||
| import Noise, DensityFunctions, Surface; | |||
There was a problem hiding this comment.
Break circular grammar import (Core_Lang ↔ DensityFunctions).
Core_Lang imports DensityFunctions, and DensityFunctions imports Core_Lang. ANTLR import cycles are not supported and will fail generation. Move densityFn-dependent builders out of Core_Lang and drop the import.
-import Noise, DensityFunctions, Surface;
+import Noise, Surface;📝 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.
| import Noise, DensityFunctions, Surface; | |
| import Noise, Surface; |
| builder_Smear: 'Smear' '(' number ')'; | ||
| builder_Type1: '.Type1' '(' ')'; | ||
| builder_Type2: '.Type2' '(' ')'; | ||
| builder_Shift: '.Shift' '(' Axis ',' densityFn ')'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove densityFn-dependent builders from Core_Lang.
These reference densityFn (defined in DensityFunctions), causing the cycle. Define them in DensityFunctions.g4 instead (see paired diff there).
-builder_Shift: '.Shift' '(' Axis ',' densityFn ')';
@@
-builder_InRange: '.InRange' '(' densityFn ')';
-builder_OutRange: '.OutRange' '(' densityFn ')';Also applies to: 25-26
🤖 Prompt for AI Agents
In grammar/Core_Lang.g4 around lines 13 (and also lines 25-26), there are
builder rules that reference densityFn (e.g., builder_Shift and the other
densityFn-dependent builders) which create a grammar cycle; remove these
densityFn-dependent builder rules from Core_Lang.g4 and instead add equivalent
rule definitions into DensityFunctions.g4 (so DensityFunctions owns
densityFn-related builders). Update Core_Lang.g4 by deleting those specific
lines/rules and ensure any imports or parser references still point to the new
rules in DensityFunctions.g4.
| ('Noise' NL* '(' NL* (resourceReference) NL* ')') | densityFn_InlineNoise | ||
| ) NL* (densityFn_NoiseBuilder NL*)*; | ||
|
|
||
| densityFn_NoiseBuilder: builder_XZScale | builder_YScale; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize densityFn-dependent builders here to break grammar cycle (paired with Core_Lang.g4 change).
These builders reference densityFn; defining them in Core_Lang creates a circular import (Core_Lang ↔ DensityFunctions). Add them here and remove from Core_Lang.
densityFn_NoiseBuilder: builder_XZScale | builder_YScale;
+
+// densityFn-scoped builders (moved from Core_Lang to avoid circular import)
+builder_Shift: '.Shift' '(' Axis ',' densityFn ')';
+builder_InRange: '.InRange' '(' densityFn ')';
+builder_OutRange: '.OutRange' '(' densityFn ')';📝 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.
| densityFn_NoiseBuilder: builder_XZScale | builder_YScale; | |
| densityFn_NoiseBuilder: builder_XZScale | builder_YScale; | |
| // densityFn-scoped builders (moved from Core_Lang to avoid circular import) | |
| builder_Shift: '.Shift' '(' Axis ',' densityFn ')'; | |
| builder_InRange: '.InRange' '(' densityFn ')'; | |
| builder_OutRange: '.OutRange' '(' densityFn ')'; |
🤖 Prompt for AI Agents
In grammar/DensityFunctions.g4 around line 52, localize the densityFn-dependent
builder rules (specifically builder_XZScale and builder_YScale) here instead of
in Core_Lang to break the circular grammar dependency; move the full rule
definitions for those builders from Core_Lang into this file, update any rule
references to point to these local definitions, and remove their definitions
from Core_Lang so Core_Lang only references densityFn (no reciprocal import).
Ensure rule names and token references remain identical and run the grammar
build to confirm no unresolved references.
| densityFn_DualInput: ( | ||
| | 'Min' | ||
| | 'Max' | ||
| ) NL* '(' NL* densityFn ',' NL* densityFn NL* ')'; |
There was a problem hiding this comment.
Remove stray empty alternative in densityFn_DualInput (parsing bug).
The leading | introduces an empty alternative, making ( valid without Min/Max. This will parse invalid input and likely breaks intent.
Apply:
-densityFn_DualInput: (
- | 'Min'
- | 'Max'
-) NL* '(' NL* densityFn ',' NL* densityFn NL* ')';
+densityFn_DualInput: (
+ 'Min'
+ | 'Max'
+) NL* '(' NL* densityFn ',' NL* densityFn NL* ')';📝 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.
| densityFn_DualInput: ( | |
| | 'Min' | |
| | 'Max' | |
| ) NL* '(' NL* densityFn ',' NL* densityFn NL* ')'; | |
| densityFn_DualInput: ( | |
| 'Min' | |
| | 'Max' | |
| ) NL* '(' NL* densityFn ',' NL* densityFn NL* ')'; |
🤖 Prompt for AI Agents
In grammar/DensityFunctions.g4 around lines 57 to 60, the densityFn_DualInput
rule contains a leading '|' which creates an empty alternative allowing '('
without 'Min' or 'Max'; remove the stray leading '|' so the alternatives are
enumerated properly (e.g. replace the block so the first alternative is 'Min'
and the second is 'Max') and keep the surrounding NL* and punctuation unchanged
to restore correct parsing.
| func ExtractInlinedNoise(noiseCtx *grammar.NoiseDefinitionContext, currentNamespace string, scope *traversal.Scope) *traversal.Reference { | ||
| noiseDef := traversal.ConstructRegistry.Construct(noiseCtx, currentNamespace, scope) | ||
| noiseRef := traversal.NewReference( | ||
| fmt.Sprintf("densityfn_noise_%d_%d", noiseCtx.GetStart().GetLine(), noiseCtx.GetStart().GetColumn()), | ||
| "mms_inline", | ||
| ) | ||
| noiseSymbol := traversal.NewSymbol( | ||
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | ||
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | ||
| noiseDef, | ||
| noiseRef, | ||
| "Noise", | ||
| ) | ||
| if _, ok := scope.Get(*noiseRef); !ok { | ||
| if err := scope.Register(noiseSymbol); err != nil { | ||
| // TODO: Improve behavior | ||
| panic(err) | ||
| } | ||
| } | ||
| return noiseRef |
There was a problem hiding this comment.
Don’t panic on user input; validate inline noise and diagnose
Panic on register error will crash parsing; also handle nil noiseDef.
func ExtractInlinedNoise(noiseCtx *grammar.NoiseDefinitionContext, currentNamespace string, scope *traversal.Scope) *traversal.Reference {
- noiseDef := traversal.ConstructRegistry.Construct(noiseCtx, currentNamespace, scope)
+ noiseDef := traversal.ConstructRegistry.Construct(noiseCtx, currentNamespace, scope)
+ if noiseDef == nil {
+ scope.DiagnoseSemanticError("Invalid inline noise definition", noiseCtx)
+ return nil
+ }
noiseRef := traversal.NewReference(
fmt.Sprintf("densityfn_noise_%d_%d", noiseCtx.GetStart().GetLine(), noiseCtx.GetStart().GetColumn()),
"mms_inline",
)
noiseSymbol := traversal.NewSymbol(
traversal.RuleLocation(noiseCtx, scope.CurrentFile),
traversal.RuleLocation(noiseCtx, scope.CurrentFile),
noiseDef,
noiseRef,
"Noise",
)
if _, ok := scope.Get(*noiseRef); !ok {
- if err := scope.Register(noiseSymbol); err != nil {
- // TODO: Improve behavior
- panic(err)
- }
+ if err := scope.Register(noiseSymbol); err != nil {
+ scope.DiagnoseSemanticError(fmt.Sprintf("Failed to register inline noise: %v", err), noiseCtx)
+ return nil
+ }
}
return noiseRef
}📝 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 ExtractInlinedNoise(noiseCtx *grammar.NoiseDefinitionContext, currentNamespace string, scope *traversal.Scope) *traversal.Reference { | |
| noiseDef := traversal.ConstructRegistry.Construct(noiseCtx, currentNamespace, scope) | |
| noiseRef := traversal.NewReference( | |
| fmt.Sprintf("densityfn_noise_%d_%d", noiseCtx.GetStart().GetLine(), noiseCtx.GetStart().GetColumn()), | |
| "mms_inline", | |
| ) | |
| noiseSymbol := traversal.NewSymbol( | |
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | |
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | |
| noiseDef, | |
| noiseRef, | |
| "Noise", | |
| ) | |
| if _, ok := scope.Get(*noiseRef); !ok { | |
| if err := scope.Register(noiseSymbol); err != nil { | |
| // TODO: Improve behavior | |
| panic(err) | |
| } | |
| } | |
| return noiseRef | |
| func ExtractInlinedNoise(noiseCtx *grammar.NoiseDefinitionContext, currentNamespace string, scope *traversal.Scope) *traversal.Reference { | |
| noiseDef := traversal.ConstructRegistry.Construct(noiseCtx, currentNamespace, scope) | |
| if noiseDef == nil { | |
| scope.DiagnoseSemanticError("Invalid inline noise definition", noiseCtx) | |
| return nil | |
| } | |
| noiseRef := traversal.NewReference( | |
| fmt.Sprintf("densityfn_noise_%d_%d", noiseCtx.GetStart().GetLine(), noiseCtx.GetStart().GetColumn()), | |
| "mms_inline", | |
| ) | |
| noiseSymbol := traversal.NewSymbol( | |
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | |
| traversal.RuleLocation(noiseCtx, scope.CurrentFile), | |
| noiseDef, | |
| noiseRef, | |
| "Noise", | |
| ) | |
| if _, ok := scope.Get(*noiseRef); !ok { | |
| if err := scope.Register(noiseSymbol); err != nil { | |
| scope.DiagnoseSemanticError(fmt.Sprintf("Failed to register inline noise: %v", err), noiseCtx) | |
| return nil | |
| } | |
| } | |
| return noiseRef | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/DensityFn.go around lines 42 to
61, the function panics on scope.Register errors and doesn't handle a nil
noiseDef; change it to validate noiseDef first (if noiseDef == nil return nil),
and replace the panic on Register error with non-fatal handling: log the error
(or use scope's logger if available) and return nil so parsing doesn't crash;
keep the rest of the registration logic the same and ensure the function
consistently returns nil on any failure to create or register the inline noise
reference.
| scope.DiagnoseSemanticError("Missing or invalid Axis", ctx) | ||
| return | ||
| } | ||
| childFnCtx := ctx.DensityFn() | ||
| if childFnCtx == nil { | ||
| scope.DiagnoseSemanticError("Missing or invalid Density Function", ctx) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early-return on invalid density function; normalize error text/case.
Avoid assigning nil shifts after reporting errors; standardize messages.
Included in the diff above:
- Return/continue after diagnostics for missing/invalid density function.
- Lower-case axis normalization via strings.ToLower.
- Consistent error strings (“axis”, “density function”).
Also applies to: 39-42, 44-57
| derivativeCtx := point.GetChild(2) | ||
| if derivative, ok := derivativeCtx.(*grammar.NumberContext); ok { | ||
| val, err := strconv.ParseFloat(derivative.GetText(), 64) | ||
| if err != nil { | ||
| scope.DiagnoseSemanticError("Invalid spline derivative", point) | ||
| return nil | ||
| } | ||
| out.Derivative = val | ||
| } else { | ||
| scope.DiagnoseSemanticError("Invalid spline derivative", point) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Derivative parsing uses the wrong node (reads location instead of derivative).
GetChild(2) is the third child in the raw parse tree (tokens included), not the 3rd Number. For .Point(loc, value, deriv) this sets Derivative = loc. Parse the last Number instead.
- derivativeCtx := point.GetChild(2)
- if derivative, ok := derivativeCtx.(*grammar.NumberContext); ok {
- val, err := strconv.ParseFloat(derivative.GetText(), 64)
- if err != nil {
- scope.DiagnoseSemanticError("Invalid spline derivative", point)
- return nil
- }
- out.Derivative = val
- } else {
- scope.DiagnoseSemanticError("Invalid spline derivative", point)
- return nil
- }
+ // derivative is always the last Number in: .Point '(' number ',' (number|spline|ref) ',' number ')'
+ if nums := point.AllNumber(); len(nums) >= 2 {
+ if val, err := strconv.ParseFloat(nums[len(nums)-1].GetText(), 64); err == nil {
+ out.Derivative = val
+ } else {
+ scope.DiagnoseSemanticError("Invalid spline derivative", point)
+ return nil
+ }
+ } else {
+ scope.DiagnoseSemanticError("Missing spline derivative", point)
+ 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.
| derivativeCtx := point.GetChild(2) | |
| if derivative, ok := derivativeCtx.(*grammar.NumberContext); ok { | |
| val, err := strconv.ParseFloat(derivative.GetText(), 64) | |
| if err != nil { | |
| scope.DiagnoseSemanticError("Invalid spline derivative", point) | |
| return nil | |
| } | |
| out.Derivative = val | |
| } else { | |
| scope.DiagnoseSemanticError("Invalid spline derivative", point) | |
| return nil | |
| } | |
| // derivative is always the last Number in: .Point '(' number ',' (number|spline|ref) ',' number ')' | |
| if nums := point.AllNumber(); len(nums) >= 2 { | |
| if val, err := strconv.ParseFloat(nums[len(nums)-1].GetText(), 64); err == nil { | |
| out.Derivative = val | |
| } else { | |
| scope.DiagnoseSemanticError("Invalid spline derivative", point) | |
| return nil | |
| } | |
| } else { | |
| scope.DiagnoseSemanticError("Missing spline derivative", point) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/SplineFn.go around lines 51–62,
the code incorrectly uses point.GetChild(2) (a raw token-based child) to read
the derivative, which can pick up the location token; instead scan the children
from the end and pick the last child that is a *grammar.NumberContext (or
otherwise find the last Number node), parse its text with strconv.ParseFloat,
set out.Derivative, and keep the existing semantic error handling if parsing or
type assertion fails.
| for _, builderWrap := range densityFn.AllDensityFn_WierdScaledSamplerBuilder() { | ||
| builder := builderWrap.GetChild(0) | ||
| if builder == nil { | ||
| // TODO: Diagnose | ||
| continue | ||
| } | ||
| if _, ok := builder.(*grammar.Builder_Type1Context); ok { | ||
| out.ValueMapper = "type_1" | ||
| } | ||
| if _, ok := builder.(*grammar.Builder_Type2Context); ok { | ||
| out.ValueMapper = "type_2" | ||
| } | ||
| if noise, ok := builder.(*grammar.Builder_NoiseContext); ok { | ||
| if noise.ResourceReference() != nil { | ||
| ref := traversal.ConstructRegistry.Construct(noise.ResourceReference(), currentNamespace, scope) | ||
| if r, ok := ref.(*traversal.Reference); ok { | ||
| out.Noise = *r | ||
| } else { | ||
| scope.DiagnoseSemanticError("Invalid noise reference", noise) | ||
| // TODO: Diagnose | ||
| continue | ||
| } | ||
| } else if inline := noise.NoiseDefinition(); inline != nil { | ||
| ref := ExtractInlinedNoise(inline.(*grammar.NoiseDefinitionContext), currentNamespace, scope) | ||
| if ref != nil { | ||
| out.Noise = *ref | ||
| } else { | ||
| // TODO: Diagnose | ||
| scope.DiagnoseSemanticError("Invalid inline noise definition.", noise) | ||
| continue | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce required fields and guard duplicate mapper builders.
Currently, ValueMapper and Noise can be omitted without diagnostics, yielding invalid JSON. Also, both type_1 and type_2 can be specified, silently overriding. Add checks and a simple duplicate guard.
- for _, builderWrap := range densityFn.AllDensityFn_WierdScaledSamplerBuilder() {
+ seenMapper := false
+ for _, builderWrap := range densityFn.AllDensityFn_WierdScaledSamplerBuilder() {
builder := builderWrap.GetChild(0)
if builder == nil {
// TODO: Diagnose
continue
}
- if _, ok := builder.(*grammar.Builder_Type1Context); ok {
- out.ValueMapper = "type_1"
- }
- if _, ok := builder.(*grammar.Builder_Type2Context); ok {
- out.ValueMapper = "type_2"
- }
+ if _, ok := builder.(*grammar.Builder_Type1Context); ok {
+ if seenMapper {
+ scope.DiagnoseSemanticWarning("Duplicate rarity_value_mapper; last one wins", builder)
+ }
+ out.ValueMapper = "type_1"
+ seenMapper = true
+ } else if _, ok := builder.(*grammar.Builder_Type2Context); ok {
+ if seenMapper {
+ scope.DiagnoseSemanticWarning("Duplicate rarity_value_mapper; last one wins", builder)
+ }
+ out.ValueMapper = "type_2"
+ seenMapper = true
+ }
if noise, ok := builder.(*grammar.Builder_NoiseContext); ok {
if noise.ResourceReference() != nil {
ref := traversal.ConstructRegistry.Construct(noise.ResourceReference(), currentNamespace, scope)
if r, ok := ref.(*traversal.Reference); ok {
out.Noise = *r
} else {
scope.DiagnoseSemanticError("Invalid noise reference", noise)
// TODO: Diagnose
continue
}
} else if inline := noise.NoiseDefinition(); inline != nil {
ref := ExtractInlinedNoise(inline.(*grammar.NoiseDefinitionContext), currentNamespace, scope)
if ref != nil {
out.Noise = *ref
} else {
// TODO: Diagnose
scope.DiagnoseSemanticError("Invalid inline noise definition.", noise)
continue
}
}
}
}
+ if out.ValueMapper == "" {
+ scope.DiagnoseSemanticError("Missing rarity_value_mapper (type_1 or type_2)", densityFn)
+ }
+ if out.Noise.String() == "" {
+ scope.DiagnoseSemanticError("Missing noise reference or inline noise", densityFn)
+ }
+
return out📝 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.
| for _, builderWrap := range densityFn.AllDensityFn_WierdScaledSamplerBuilder() { | |
| builder := builderWrap.GetChild(0) | |
| if builder == nil { | |
| // TODO: Diagnose | |
| continue | |
| } | |
| if _, ok := builder.(*grammar.Builder_Type1Context); ok { | |
| out.ValueMapper = "type_1" | |
| } | |
| if _, ok := builder.(*grammar.Builder_Type2Context); ok { | |
| out.ValueMapper = "type_2" | |
| } | |
| if noise, ok := builder.(*grammar.Builder_NoiseContext); ok { | |
| if noise.ResourceReference() != nil { | |
| ref := traversal.ConstructRegistry.Construct(noise.ResourceReference(), currentNamespace, scope) | |
| if r, ok := ref.(*traversal.Reference); ok { | |
| out.Noise = *r | |
| } else { | |
| scope.DiagnoseSemanticError("Invalid noise reference", noise) | |
| // TODO: Diagnose | |
| continue | |
| } | |
| } else if inline := noise.NoiseDefinition(); inline != nil { | |
| ref := ExtractInlinedNoise(inline.(*grammar.NoiseDefinitionContext), currentNamespace, scope) | |
| if ref != nil { | |
| out.Noise = *ref | |
| } else { | |
| // TODO: Diagnose | |
| scope.DiagnoseSemanticError("Invalid inline noise definition.", noise) | |
| continue | |
| } | |
| } | |
| } | |
| } | |
| // Enforce a single mapper and track if one has been seen. | |
| seenMapper := false | |
| for _, builderWrap := range densityFn.AllDensityFn_WierdScaledSamplerBuilder() { | |
| builder := builderWrap.GetChild(0) | |
| if builder == nil { | |
| // TODO: Diagnose | |
| continue | |
| } | |
| // Handle rarity value mappers, warning on duplicates. | |
| if _, ok := builder.(*grammar.Builder_Type1Context); ok { | |
| if seenMapper { | |
| scope.DiagnoseSemanticWarning("Duplicate rarity_value_mapper; last one wins", builder) | |
| } | |
| out.ValueMapper = "type_1" | |
| seenMapper = true | |
| } else if _, ok := builder.(*grammar.Builder_Type2Context); ok { | |
| if seenMapper { | |
| scope.DiagnoseSemanticWarning("Duplicate rarity_value_mapper; last one wins", builder) | |
| } | |
| out.ValueMapper = "type_2" | |
| seenMapper = true | |
| } | |
| // Handle noise reference or inline noise definition. | |
| if noise, ok := builder.(*grammar.Builder_NoiseContext); ok { | |
| if noise.ResourceReference() != nil { | |
| ref := traversal.ConstructRegistry.Construct(noise.ResourceReference(), currentNamespace, scope) | |
| if r, ok := ref.(*traversal.Reference); ok { | |
| out.Noise = *r | |
| } else { | |
| scope.DiagnoseSemanticError("Invalid noise reference", noise) | |
| // TODO: Diagnose | |
| continue | |
| } | |
| } else if inline := noise.NoiseDefinition(); inline != nil { | |
| ref := ExtractInlinedNoise(inline.(*grammar.NoiseDefinitionContext), currentNamespace, scope) | |
| if ref != nil { | |
| out.Noise = *ref | |
| } else { | |
| // TODO: Diagnose | |
| scope.DiagnoseSemanticError("Invalid inline noise definition.", noise) | |
| continue | |
| } | |
| } | |
| } | |
| } | |
| // Ensure both a mapper and a noise source were provided. | |
| if out.ValueMapper == "" { | |
| scope.DiagnoseSemanticError("Missing rarity_value_mapper (type_1 or type_2)", densityFn) | |
| } | |
| if out.Noise.String() == "" { | |
| scope.DiagnoseSemanticError("Missing noise reference or inline noise", densityFn) | |
| } | |
| return out |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go around
lines 32-66, add validation to ensure ValueMapper and Noise are required and
detect duplicate mapper builders: track whether a mapper was already set when
iterating builders and if a second mapper (type_1 or type_2) is encountered call
scope.DiagnoseSemanticError("Duplicate value mapper", builder) and
skip/continue; ensure that when processing noise you set out.Noise only once and
diagnose on invalid references as already partly done; after the loop validate
that out.ValueMapper and out.Noise have been set and if either is missing call
scope.DiagnoseSemanticError("Missing required field: ValueMapper" or "Missing
required field: Noise", the original context) and handle the error
(return/continue) so invalid JSON cannot be produced.
| noise := declaration.Noise() | ||
| if noise == nil { | ||
| // TODO: Diagnose? | ||
| return nil | ||
| } | ||
| noiseDef := noise.NoiseDefinition() | ||
| if noiseDef == nil { | ||
| // TODO: Diagnose? | ||
| return nil | ||
| } | ||
| s := traversal.ProcessDeclaration(declaration, noiseDef, scope, currentNamespace, "Noise") | ||
| if s == nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Diagnose missing noise/noiseDefinition instead of silently returning
Silent returns make failures hard to debug. Emit semantic errors when either node is absent.
Apply this diff:
- noise := declaration.Noise()
- if noise == nil {
- // TODO: Diagnose?
- return nil
- }
- noiseDef := noise.NoiseDefinition()
- if noiseDef == nil {
- // TODO: Diagnose?
- return nil
- }
+ noise := declaration.Noise()
+ if noise == nil {
+ scope.DiagnoseSemanticError("Missing noise in declaration", declaration)
+ return nil
+ }
+ noiseDef := noise.NoiseDefinition()
+ if noiseDef == nil {
+ scope.DiagnoseSemanticError("Missing noise definition", noise)
+ 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.
| noise := declaration.Noise() | |
| if noise == nil { | |
| // TODO: Diagnose? | |
| return nil | |
| } | |
| noiseDef := noise.NoiseDefinition() | |
| if noiseDef == nil { | |
| // TODO: Diagnose? | |
| return nil | |
| } | |
| s := traversal.ProcessDeclaration(declaration, noiseDef, scope, currentNamespace, "Noise") | |
| if s == nil { | |
| noise := declaration.Noise() | |
| if noise == nil { | |
| scope.DiagnoseSemanticError("Missing noise in declaration", declaration) | |
| return nil | |
| } | |
| noiseDef := noise.NoiseDefinition() | |
| if noiseDef == nil { | |
| scope.DiagnoseSemanticError("Missing noise definition", noise) | |
| return nil | |
| } | |
| s := traversal.ProcessDeclaration(declaration, noiseDef, scope, currentNamespace, "Noise") | |
| if s == nil { |
🤖 Prompt for AI Agents
In lang/constructs/worldgen/noise/Noise.go around lines 32 to 43, the code
currently returns nil silently when declaration.Noise() or
noise.NoiseDefinition() is nil; change this to emit semantic errors instead of
returning silently: when noise is nil, call the project's semantic error
reporting mechanism (e.g. report/append a semantic error on scope or
currentNamespace) with a clear message like "missing Noise node on declaration";
when noiseDef is nil, emit a semantic error like "missing NoiseDefinition on
Noise node"; after reporting the error(s) return nil as before.
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lang/constructs/worldgen/surface_rules/Rule_Reference.go (1)
20-37: Avoid pointer-to-interface and unchecked type assertion in resolver
next.GetValue().(traversal.Construct)can panic. Also storing*traversal.Constructis an unnecessary pointer-to-interface. Use a safe assertion and keepValueas an interface.@@ type ReferenceRule struct { traversal.Construct Ref *traversal.Reference - Value *traversal.Construct + Value traversal.Construct } @@ if ref != nil { ref.SetResolver(func() error { if next, ok := scope.Get(*ref); ok { - val := next.GetValue().(traversal.Construct) - out.Value = &val + if v, ok := next.GetValue().(traversal.Construct); ok { + out.Value = v + } } return nil }) }lang/constructs/worldgen/density_functions/FunctionRef.go (1)
1-63: Use pointer grammar context types in ConstructRegistry.Register calls
Replace everyreflect.TypeFor[grammar.*Context]()withreflect.TypeFor[*grammar.*Context]()in yourConstructRegistry.Registerinvocations (e.g. FunctionRef.go:21, Condition_AboveSurface.go:16, Rule_Block.go:16, and all other non-pointer registrations) to ensure proper context matching.
♻️ Duplicate comments (8)
lang/constructs/worldgen/density_functions/FunctionRef.go (2)
21-21: Fix registry key to pointer context type (prevents constructor dispatch).Use the pointer type for the grammar context to match other registrations. This is a hard break otherwise.
- reflect.TypeFor[grammar.DensityFn_ReferenceContext](), + reflect.TypeFor[*grammar.DensityFn_ReferenceContext](),
3-11: Harden resolver: nil scope, missing symbol, and unsafe type assertion can panic.Add proper error returns and avoid pointer-to-interface assignment (see next comment). Also import fmt.
import ( + "fmt" "errors" "reflect" @@ - out.Ref.SetResolver(func() error { - if next, ok := scope.Get(*r); ok { - val := next.GetValue().(traversal.Construct) - - out.Value = &val - } - return nil - }) + out.Ref.SetResolver(func() error { + if scope == nil { + return fmt.Errorf("nil scope while resolving %s", r.String()) + } + next, ok := scope.Get(*r) + if !ok { + return fmt.Errorf("reference not found: %s", r.String()) + } + val, ok := next.GetValue().(traversal.Construct) + if !ok { + return fmt.Errorf("symbol %s is not a density function (%T)", r.String(), next.GetValue()) + } + out.Value = val + return nil + })Also applies to: 30-37
lang/constructs/worldgen/density_functions/RangeChoiceFn.go (4)
45-46: Fix diagnostic message: OutRange error mentions InRange- scope.DiagnoseSemanticError("Missing or invalid InRange Function", ctx) + scope.DiagnoseSemanticError("Missing or invalid OutRange Function", ctx)
53-58: Guard input cast to avoid panic and diagnose invalid input- input := densityFn.DensityFn() - if input == nil { - scope.DiagnoseSemanticError("Missing input to range choice", ctx) - } else { - out.Input = traversal.ConstructRegistry.Construct(input.(antlr.ParserRuleContext), currentNamespace, scope) - } + input := densityFn.DensityFn() + if input == nil { + scope.DiagnoseSemanticError("Missing input to range choice", ctx) + } else if rule, ok := input.(antlr.ParserRuleContext); ok { + out.Input = traversal.ConstructRegistry.Construct(rule, currentNamespace, scope) + if out.Input == nil { + scope.DiagnoseSemanticError("Invalid input density function", ctx) + } + } else { + scope.DiagnoseSemanticError("Invalid input node", ctx) + }
60-62: Defensive cast for builder child to avoid panics- for _, builderWrap := range densityFn.AllDensityFn_RangeChoiceBuilder() { - builder_chain.Invoke(rangeChoiceBuilder, builderWrap.GetChild(0).(antlr.ParserRuleContext), out, scope, currentNamespace) - } + for _, builderWrap := range densityFn.AllDensityFn_RangeChoiceBuilder() { + child := builderWrap.GetChild(0) + if child == nil { + scope.DiagnoseSemanticWarning("Unexpected empty builder node", builderWrap) + continue + } + if rule, ok := child.(antlr.ParserRuleContext); ok { + builder_chain.Invoke(rangeChoiceBuilder, rule, out, scope, currentNamespace) + } else { + scope.DiagnoseSemanticWarning("Unexpected builder node", builderWrap) + } + }
93-93: Validate range invariants (Min ≤ Max) before returning- return out + if out.Min > out.Max { + scope.DiagnoseSemanticError("Min must be less than or equal to Max for range_choice", densityFn) + } + return outgrammar/Core_Lang.g4 (1)
3-3: Break ANTLR import cycle with DensityFunctions.Core_Lang imports DensityFunctions and defines rules that reference densityFn; if DensityFunctions also imports Core_Lang, generation will fail. Move densityFn-dependent builders into DensityFunctions and drop the import here.
-import Noise, DensityFunctions, Surface; +import Noise, Surface; @@ -builder_ShiftX: '.ShiftX' '(' densityFn ')'; -builder_ShiftY: '.ShiftY' '(' densityFn ')'; -builder_ShiftZ: '.ShiftZ' '(' densityFn ')'; @@ -builder_InRange: '.InRange' '(' densityFn ')'; -builder_OutRange: '.OutRange' '(' densityFn ')';Define the removed builders in DensityFunctions.g4 where densityFn is defined.
Also applies to: 13-15, 27-28
grammar/DensityFunctions.g4 (1)
57-60: Fix empty alternative in DualInput (parsing bug).Leading "|" introduces an empty alt, allowing "(" without "Min"/"Max".
Apply:
-densityFn_DualInput: ( - | 'Min' - | 'Max' -) NL* '(' NL* densityFn ',' NL* densityFn NL* ')'; +densityFn_DualInput: + ( 'Min' | 'Max' ) + NL* '(' NL* densityFn ',' NL* densityFn NL* ')';
🧹 Nitpick comments (91)
vendor/github.com/lucasb-eyer/go-colorful/.gitignore (1)
5-8: Optional: drop .vscode exceptions to avoid editor files creeping inThe un-ignores for tasks.json/launch.json would allow those to be tracked if present under this path. Not a blocker, but if you’re curating vendor contents, consider removing the exceptions. If this mirrors upstream, keep as-is.
.vscode/* -!.vscode/tasks.json -!.vscode/launch.jsonvendor/github.com/mattn/go-isatty/go.test.sh (2)
6-12: Make package discovery deterministic and root-relative.Running this script from inside vendor limits scope to that subtree. Optionally resolve and run from repo root; also avoid
grep -v vendorby filtering at the Go level.-for d in $(go list ./... | grep -v vendor); do +repo_root=$(git rev-parse --show-toplevel 2>/dev/null || pwd) +cd "$repo_root" +# Exclude vendor packages via go list pattern; quote iteration safely +mapfile -t pkgs < <(go list ./... | grep -vE '/vendor(/|$)') +for d in "${pkgs[@]}"; do go test -race -coverprofile=profile.out -covermode=atomic "$d"
1-12: Remove executable scripts from vendor
Verified thatvendor/github.com/mattn/go-isatty/go.test.shisn’t referenced by any CI or tooling (ranrg -n --hidden -C2 'go\.test\.sh|vendor/github\.com/.*/go-isatty/.*\.sh'with no matches). Relocate this script to a repo-localscripts/directory to prevent accidental execution and keep the vendored tree strictly third-party.vendor/github.com/gorilla/websocket/Makefile (3)
28-30: Add standard entry points (all/clean) to satisfy checkmake and improve UXProvide "all" and "clean" targets; wire "all" to verification.
.PHONY: verify verify: golangci-lint gosec govulncheck + +.PHONY: all clean +all: verify +clean: + rm -f coverage.coverprofile
31-34: Consider not running tests for vendored deps hereRunning tests in vendor can be slow/flaky and is usually handled upstream. If you keep it, at least gate it.
-.PHONY: test -test: +.PHONY: vendor-test +vendor-test: @echo "##### Running tests" go test -race -cover -coverprofile=coverage.coverprofile -covermode=atomic -v ./...And avoid calling this target from top-level CI.
1-35: Confirm intent to maintain build tooling inside vendorFiles in vendor are overwritten by
go mod vendor. If you need these targets long-term, move them to a top-level tooling dir and exclude ./vendor from project-wide scanners.Happy to propose a top-level Makefile/CI job that runs these tools against your code (not vendor) and keeps versions pinned. Want me to draft it?
vendor/github.com/iancoleman/strcase/acronyms.go (3)
7-13: Normalize acronym keys to a canonical case (and document direction).Lookups in camel.go are exact; normalizing keys prevents misses due to casing and clarifies intent that values should be lowercase “skeletons” (e.g., "ID" -> "id").
Apply:
import ( - "sync" + "strings" + "sync" ) // ConfigureAcronym allows you to add additional words which will be considered acronyms func ConfigureAcronym(key, val string) { - uppercaseAcronym.Store(key, val) + // store with canonical uppercase key; val should be lowercase form (e.g., "ID" -> "id") + uppercaseAcronym.Store(strings.ToUpper(key), val) }
7-7: Prefer a zero-value declaration for sync.Map.
var uppercaseAcronym sync.Mapis idiomatic and avoids an unnecessary composite literal.-var uppercaseAcronym = sync.Map{} +var uppercaseAcronym sync.Map
8-8: Clean up stray in-line example comment.The indented
//"ID": "id",is confusing beside a non-composite literal. Move it up into the doc comment for ConfigureAcronym.vendor/github.com/iancoleman/strcase/camel.go (4)
38-41: Acronym handling only triggers on full-string matches; tokens like "user_id" won’t preserve “ID”.Today you lookup
Load(s)once. To honor acronyms at token boundaries, pre-normalize tokens with a case-insensitive map lookup, preserving separators.Apply within this block:
- a, hasAcronym := uppercaseAcronym.Load(s) - if hasAcronym { - s = a.(string) - } + // prefer token-level normalization; still allow full-string override + if a, ok := uppercaseAcronym.Load(strings.ToUpper(s)); ok { + s = a.(string) + } else { + s = normalizeAcronyms(s) + }Add this helper in the same package (outside this function):
// normalizeAcronyms replaces tokens (split on _ . - space) using the acronym map, case-insensitively. func normalizeAcronyms(s string) string { if s == "" { return s } var b strings.Builder b.Grow(len(s)) start := 0 for i := 0; i <= len(s); i++ { sep := i == len(s) || s[i] == '_' || s[i] == ' ' || s[i] == '-' || s[i] == '.' if sep { if i > start { tok := s[start:i] if a, ok := uppercaseAcronym.Load(strings.ToUpper(tok)); ok { tok = a.(string) } b.WriteString(tok) } if i < len(s) { b.WriteByte(s[i]) // keep separator } start = i + 1 } } return b.String() }
47-48: Avoid allocation from []byte(s) conversion.Index the string directly; same ASCII semantics without an extra slice.
- for i, v := range []byte(s) { + for i := 0; i < len(s); i++ { + v := s[i]
32-77: Document ASCII-only behavior and separators.Call out that only A–Z/a–z/0–9 are handled and separators are
_ - . space, which is fine but worth noting for callers expecting Unicode/other delimiters.
26-31: Vendor customization caveat.Since this is a vendored fork with runtime configuration, consider upstreaming or wrapping to reduce churn on future
go mod vendorupdates.vendor/github.com/muesli/termenv/.golangci.yml (1)
12-13: Unlimited issue caps may spam CI logsConsider modest caps (e.g., max-issues-per-linter: 50, max-same-issues: 10) to avoid noisy CI if this file ever takes effect.
- max-issues-per-linter: 0 - max-same-issues: 0 + max-issues-per-linter: 50 + max-same-issues: 10vendor/github.com/muesli/termenv/.golangci-soft.yml (1)
15-35: Style-heavy linters in vendor can create noiseEnabling many stylistic linters for third-party code risks distracting findings if ever scanned. Prefer excluding vendor at the root (see sibling comment) rather than tuning here.
vendor/github.com/mattn/go-runewidth/README.md (1)
27-27: Silence markdownlint for vendored docs instead of mutating upstream textBare URL warning (MD034) originates in vendor. Prefer ignoring vendor paths in lint config to avoid churn.
Add a repo-level config:
+--- /dev/null ++++ .markdownlint-cli2.yaml +ignores: + - "vendor/**"lang/constructs/worldgen/surface_rules/Rule_Reference.go (1)
54-65: Simplify marshaling with interface field; keep ref fallbackWith
Valueas an interface, nil-check is enough.func (c *ReferenceRule) MarshalJSON() ([]byte, error) { if c.Ref == nil { return nil, errors.New("reference is nil in ReferenceRule") } if err := c.Ref.Resolve(); err != nil { return nil, err } - if c.Value == nil { - return c.Ref.MarshalJSON() - } - return (*c.Value).MarshalJSON() + if c.Value == nil { + return c.Ref.MarshalJSON() + } + return c.Value.MarshalJSON() }vendor/github.com/gorilla/websocket/README.md (1)
13-13: Ignore markdownlint for vendor to avoid heading-level noiseMD001 comes from vendored README. Recommend excluding vendor in linter config rather than editing upstream docs.
(See suggested .markdownlint-cli2.yaml in prior comment.)
vendor/github.com/mattn/go-isatty/README.md (1)
12-30: Silence markdownlint for vendored docs instead of editing upstream contentLint findings (tabs, fenced language, bare URL) stem from upstream README. Prefer excluding vendor from markdownlint to prevent churn.
Apply either approach:
Option A — markdownlint-cli2 config:
+ # .markdownlint-cli2.yaml + ignores: + - "vendor/**"Option B — markdownlint ignore file:
+ # .markdownlintignore + vendor/**Also applies to: 34-36, 50-50
.helix/languages.toml (2)
1-3: Prefer module-root run to avoid path fragilityUsing “main.go” assumes a file at editor CWD. Running the module root is more robust.
-command = "go" -args = ["run", "main.go", "lsp"] +command = "go" +args = ["run", ".", "lsp"]
5-11: Option: ship a prebuilt LSP binary for faster startup“go run” recompiles on each launch; consider a small wrapper script or installing an mms-lsp binary on PATH to reduce editor startup latency.
vendor/github.com/lucasb-eyer/go-colorful/CHANGELOG.md (1)
38-38: Avoid linting vendor docsMarkdown link-fragment warnings in vendored CHANGELOGs are noise. Exclude vendor/** from markdownlint in CI.
Example (markdownlint-cli2 config):
+# .markdownlint-cli2.cjs +export default { + "ignores": ["vendor/**"] +}vendor/github.com/petermattis/goid/goid_go1.5_amd64.s (1)
18-29: Drop go1.23 variant recommendation—go1.5build tag applies to all Go ≥1.5
The existinggoid_go1.5_amd64(.go/.s)files are selected by thego1.5build tag on any Go version ≥1.5 (including Go 1.23), so no additional version‐specific files are needed. Usage ofgoid.Get()is limited tosasha-s/go-deadlock; you may optionally build-tag that package for debug-only builds.vendor/github.com/gorilla/websocket/proxy.go (4)
60-66: Library code should not log close errors on error pathsLogging here is noisy and leaks into consumer logs; prefer best-effort close without logging.
Apply:
- if err := conn.Close(); err != nil { - log.Printf("httpProxyDialer: failed to close connection: %v", err) - } + _ = conn.Close()Also applies to: 71-76
37-43: Fallback if proxy host:port cannot be derivedGuard against an empty hostPort to avoid a nil-dial target.
Apply:
- hostPort, _ := hostPortNoPort(hpd.proxyURL) + hostPort, _ := hostPortNoPort(hpd.proxyURL) + if hostPort == "" { + hostPort = hpd.proxyURL.Host + }
44-51: Consider sending Basic auth when only username is presentMany proxies accept an empty password; building "user:" can improve compatibility. If you prefer strictness, ignore.
Possible tweak:
- if user := hpd.proxyURL.User; user != nil { - proxyUser := user.Username() - if proxyPassword, passwordSet := user.Password(); passwordSet { - credential := base64.StdEncoding.EncodeToString([]byte(proxyUser + ":" + proxyPassword)) - connectHeader.Set("Proxy-Authorization", "Basic "+credential) - } - } + if user := hpd.proxyURL.User; user != nil { + u := user.Username() + p, _ := user.Password() // empty if not set + credential := base64.StdEncoding.EncodeToString([]byte(u + ":" + p)) + connectHeader.Set("Proxy-Authorization", "Basic "+credential) + }
67-69: Fix typo in comment“becaue” → “because”.
Apply:
- // Read response. It's OK to use and discard buffered reader here becaue + // Read response. It's OK to use and discard buffered reader here becausevendor/github.com/lucasb-eyer/go-colorful/README.md (1)
1-483: Exclude vendor docs from markdown lintThird-party README triggers many markdownlint findings; exclude vendor/** to keep CI signal clean.
Add a markdownlint ignore:
+# .markdownlintignore +vendor/**Or in markdownlint-cli2 config:
# .markdownlint-cli2.yaml globsIgnore: - "vendor/**"vendor/github.com/mattn/go-isatty/isatty_windows.go (1)
78-83: Nit: fix typos in comments.“undocomented” → “undocumented”; “windows Vista”/“Windows XP” capitalization for consistency.
-// getFileNameByHandle use the undocomented ntdll NtQueryObject to get file full name from file handler -// since GetFileInformationByHandleEx is not available under windows Vista and still some old fashion -// guys are using Windows XP, this is a workaround for those guys, it will also work on system from +// getFileNameByHandle uses the undocumented ntdll NtQueryObject to get file full name from a file handle. +// Since GetFileInformationByHandleEx is not available under Windows Vista and some old-fashioned +// users are still on Windows XP, this is a workaround; it also works on systems fromvendor/github.com/iancoleman/strcase/snake.go (1)
66-115: ASCII-only semantics; consider documenting or guarding non-ASCII input.Looping over []byte(s) and doing A–Z/a–z math assumes ASCII; multi-byte runes will be split and may produce odd results. If this is exposed to end users, either document ASCII-only support or switch to rune iteration with unicode helpers.
If you intend to support Unicode, I can draft a rune-safe version (using unicode.IsLetter/IsNumber and cases). Want me to open a follow-up?
vendor/github.com/gorilla/websocket/compression.go (4)
47-56: Guard against invalid compression levels before indexing the poolIf
compressNoContextTakeoveris called with an out-of-rangelevel(e.g., handshake path not going throughSetCompressionLevel),level-minCompressionLevelcan index out of bounds. Add a defensive check/fallback.Apply:
func compressNoContextTakeover(w io.WriteCloser, level int) io.WriteCloser { + if !isValidCompressionLevel(level) { + level = defaultCompressionLevel + } p := &flateWriterPools[level-minCompressionLevel] tw := &truncWriter{w: w} fw, _ := p.Get().(*flate.Writer)
108-123: Close underlying writer even on tail-mismatch to avoid resource leaksOn unexpected trailer bytes,
Closereturns early without closingw.tw.w. Prefer closing the underlying writer regardless, while preserving the most relevant error.func (w *flateWriteWrapper) Close() error { if w.fw == nil { return errWriteClosed } err1 := w.fw.Flush() w.p.Put(w.fw) w.fw = nil - if w.tw.p != [4]byte{0, 0, 0xff, 0xff} { - return errors.New("websocket: internal error, unexpected bytes at end of flate stream") - } - err2 := w.tw.w.Close() + err2 := w.tw.w.Close() + if w.tw.p != [4]byte{0, 0, 0xff, 0xff} { + if err1 != nil { + return err1 + } + return errors.New("websocket: internal error, unexpected bytes at end of flate stream") + } if err1 != nil { return err1 } return err2 }
36-41: Avoid panic on Reset failure; propagate as errorPanicking in library code can bring down the host process. Consider returning a no-op reader with an error instead of
panic(err).fr, _ := flateReaderPool.Get().(io.ReadCloser) if err := fr.(flate.Resetter).Reset(io.MultiReader(r, strings.NewReader(tail)), nil); err != nil { - panic(err) + _ = fr.Close() + return io.NopCloser(io.MultiReader(strings.NewReader(""), strings.NewReader(""))) // or fallback to r }Note: since this is vendored, only adopt if you’re intentionally diverging from upstream behavior.
139-141: Library logging can be noisyThe
log.Printfon close error may spam application logs in hot paths. Consider gating logs behind a package-level logger or removing in your fork if noise becomes an issue.vendor/github.com/gorilla/websocket/conn.go (2)
246-291: Set sane read limits at call sites
Conndefaults to unlimited message size unlessSetReadLimitis called. Recommend configuring a bounded limit (e.g., 4–16 MiB) where this is used to mitigate memory DoS.Example (outside vendor):
c.SetReadLimit(8 << 20) // 8 MiB
941-947: Close on oversize frames is correct; consider observabilityWhen exceeding
readLimit, a Close frame is sent, thenErrReadLimitreturned. If you need metrics, wrap reads to count these events in your server code.vendor/github.com/gorilla/websocket/join.go (1)
12-17: Reader UX: consider an io.ReadCloser variant
JoinMessagesreturns anio.Readerwith noClose. Callers must stop reading to break out. If you need explicit cancellation, consider exposing a variant that also closes the underlying connection or the current inner reader onClose. Keep as a local helper outside vendor to avoid forking.Also applies to: 25-42
vendor/github.com/gorilla/websocket/json.go (1)
49-60: ReadJSON EOF mapping is appropriate; consider wrapper if you need stricter decoding.Mapping empty messages (EOF) to io.ErrUnexpectedEOF is sane. If you later need UseNumber/DisallowUnknownFields, prefer adding thin wrappers in your codebase rather than modifying vendor.
vendor/github.com/gorilla/websocket/client.go (2)
253-255: Nit: parameter name typo in type signature (add → addr).Purely cosmetic; avoid touching vendor unless you’re carrying a patch anyway.
-var netDial func(network, add string) (net.Conn, error) +var netDial func(network, addr string) (net.Conn, error)
168-176: Minor shadowing of err within TLS block; harmless but can confuse readers.Consider avoiding short var re-declaration there in non-vendored forks.
Also applies to: 345-366
lang/File.go (2)
13-14: Storing the parse tree is useful; document lifecycle to avoid stale ASTs.A brief comment on when Script is (re)assigned/cleared will help future maintainers.
16-29: Reset Script field before parsing
Clearing f.Script up front prevents retaining a stale script if parser.Parse errors—and is safe since f.Script is only set here.func (f *File) Parse() error { - err := f.Project.GlobalScope.PurgeFile(f.Path) - f.Diagnostics = make([]traversal.Diagnostic, 0) + // Reset per-parse state + f.Script = nil + err := f.Project.GlobalScope.PurgeFile(f.Path) + f.Diagnostics = make([]traversal.Diagnostic, 0) @@ - script, err := parser.Parse() + script, err := parser.Parse() if err != nil { return err } f.Script = scriptvendor/github.com/muesli/termenv/termenv_posix.go (1)
10-17: Harden equality against historical IoctlGetInt big-endian quirkOn some 64-bit big-endian Linux arches, IoctlGetInt(TIOCGPGRP) used to return the PGID shifted into the upper 32 bits, causing false negatives. Casting both sides to 32 bits avoids that edge case without changing behavior elsewhere. See prior discussion in golang.org/x/sys/unix issue 60429. (github.com)
Apply within this range:
- return pgrp == unix.Getpgrp() + // Compare as 32-bit pid_t width to avoid historical BE ioctl quirk. + return int32(pgrp) == int32(unix.Getpgrp())vendor/github.com/muesli/termenv/hyperlink.go (1)
8-11: Guard against empty links; optionally sanitize name to prevent control-sequence injectionEmitting OSC8 with an empty link produces stray escapes; and untrusted names could inject control sequences.
func (o *Output) Hyperlink(link, name string) string { - return OSC + "8;;" + link + ST + name + OSC + "8;;" + ST + if link == "" { + return name + } + // Optional: strip BEL/ESC from name to avoid terminal injection. + // name = strings.Map(func(r rune) rune { if r == 0x07 || r == 0x1b { return -1 }; return r }, name) + return OSC + "8;;" + link + ST + name + OSC + "8;;" + ST }If you want the sanitization, I can wire the strings import for you.
vendor/github.com/petermattis/goid/goid.go (1)
31-35: Use a slightly larger buffer for safety64 bytes is usually enough for the header, but 128–256 avoids edge truncation without meaningful cost.
func getSlow() int64 { - var buf [64]byte + var buf [256]byte return ExtractGID(buf[:runtime.Stack(buf[:], false)]) }vendor/github.com/mattn/go-runewidth/runewidth_posix.go (1)
53-56: Make locale prefix checks case-insensitive
strings.HasPrefix(locale, ...)can miss uppercase locale codes. Use a lowercased copy.- if max > 1 && (charset[0] != 'u' || - strings.HasPrefix(locale, "ja") || - strings.HasPrefix(locale, "ko") || - strings.HasPrefix(locale, "zh")) { + lower := strings.ToLower(locale) + if max > 1 && (charset[0] != 'u' || + strings.HasPrefix(lower, "ja") || + strings.HasPrefix(lower, "ko") || + strings.HasPrefix(lower, "zh")) { return true }vendor/github.com/petermattis/goid/goid_go1.4.go (2)
16-17: Add modern //go:build guard alongside +build.Future-proof the build constraints by adding the new-style guard; keep the legacy +build for backward compatibility.
+//go:build go1.4 && !go1.5 // +build go1.4,!go1.5
22-23: Name the magic offset for clarity.Make the 16-field offset explicit to aid future audits.
var pointerSize = unsafe.Sizeof(uintptr(0)) +const goidFieldIndex = 16 // index of goid in runtime.G for Go 1.4And:
- return *(*int64)(unsafe.Pointer(getg() + 16*pointerSize)) + return *(*int64)(unsafe.Pointer(getg() + uintptr(goidFieldIndex)*pointerSize))vendor/github.com/muesli/termenv/notification.go (1)
8-11: Sanitize/escape OSC777 args and surface write errors.Unescaped title/body can break the control sequence (e.g., contain ';', BEL, or ST) and ignored errors hinder observability. Either sanitize inputs or document that only trusted/plain text is permitted; consider returning error from the method.
Proposed minimal change (vendor-local):
-package termenv +package termenv + +import "strings" @@ -func (o *Output) Notify(title, body string) { - _, _ = o.WriteString(OSC + "777;notify;" + title + ";" + body + ST) +func (o *Output) Notify(title, body string) error { + t := sanitizeOSCArg(title) + b := sanitizeOSCArg(body) + _, err := o.WriteString(OSC + "777;notify;" + t + ";" + b + ST) + return err }Add helper outside the range:
func sanitizeOSCArg(s string) string { // Remove ST terminators and BEL; escape semicolons to avoid field breaks. s = strings.ReplaceAll(s, ST, "") s = strings.ReplaceAll(s, "\a", "") s = strings.ReplaceAll(s, ";", `\;`) return s }And keep the package-level wrapper backwards compatible:
-func Notify(title, body string) { - output.Notify(title, body) +func Notify(title, body string) { + _ = output.Notify(title, body) }If you prefer not to modify vendor code, add a sanitized wrapper in our codebase and keep passing trusted strings only.
vendor/github.com/muesli/termenv/ansicolors.go (1)
23-281: Enforce palette length and immutability.To prevent accidental edits, consider either:
- switching to a fixed-size array [256]string, or
- adding a length guard in init().
Option A (compile-time shape):
-var ansiHex = []string{ +var ansiHex = [...]string{ // entries… }Option B (runtime assert):
func init() { const want = 256 if n := len(ansiHex); n != want { panic("termenv: ansiHex length mismatch; got " + strconv.Itoa(n)) } }If changing vendor code is undesirable, add a unit test in our tree to assert the expected length at build time.
vendor/github.com/lucasb-eyer/go-colorful/happy_palettegen.go (1)
13-15: Optional: deterministic RNG for reproducible palettesIf reproducibility matters in tests/exports, allow injecting a *rand.Rand or seed.
I can sketch an alternative API, e.g., FastHappyPaletteWithRand(n int, r *rand.Rand).
vendor/github.com/muesli/termenv/README.md (1)
371-371: markdownlint: duplicate footnote reference definition [^vte]There are two [^vte] definitions; markdownlint (MD053) flags this.
Prefer excluding vendor/ from markdownlint or deduplicate the footnote in this file. Example config:
+.markdownlint-cli2.yaml +globs: + - "**/*.md" +ignores: + - "vendor/**"vendor/github.com/muesli/termenv/copy.go (1)
15-16: Optional: return errors instead of swallowing themSilent failure makes troubleshooting hard, especially over SSH/muxers.
Consider
CopyE/CopyPrimaryEvariants returning error, and keep current as fire-and-forget wrappers.Also applies to: 25-26
lang/traversal/Parser.go (1)
30-37: Optional: simplify nil-check and avoid unsafe castantlr parsers typically return non-nil contexts even on error; consider removing the nil branch or guarding the type assertion.
ctx := p.parser.Script() sc, _ := ctx.(*grammar.ScriptContext) return sc, nil // diagnostics listener captures errorsvendor/github.com/lucasb-eyer/go-colorful/colorgens.go (2)
20-24: Optional: add a safety cap to the retry loopsWhile unlikely, an infinite loop is theoretically possible; cap iterations and fall back.
-func WarmColor() (c Color) { - for c = randomWarm(); !c.IsValid(); c = randomWarm() { - } - return -} +func WarmColor() (c Color) { + for i := 0; i < 1024; i++ { + if c = randomWarm(); c.IsValid() { + return c + } + } + return Hsv(0, 0, 0.5) // neutral fallback +} @@ -func HappyColor() (c Color) { - for c = randomPimp(); !c.IsValid(); c = randomPimp() { - } - return -} +func HappyColor() (c Color) { + for i := 0; i < 1024; i++ { + if c = randomPimp(); c.IsValid() { + return c + } + } + return Hsv(0, 0, 0.5) +}Also applies to: 44-48
11-15: Optional: expose seeded variants for reproducibilityConsider FastWarmColorWithRand/HappyColorWithRand to allow tests/export stability.
I can draft minimal wrappers that take *rand.Rand without touching existing APIs.
Also applies to: 35-39
lang/constructs/worldgen/density_functions/FunctionRef.go (3)
51-54: Fix error message typo.Class name is ReferenceFunction, not ReferenceRule.
- return nil, errors.New("reference is nil in ReferenceRule") + return nil, errors.New("reference is nil in ReferenceFunction")
25-42: Confirm grammar guarantees ResourceReference is present; otherwise return early with diagnostic.Currently we always return a non-nil construct even if Ref is nil, which guarantees a MarshalJSON error later. Prefer returning nil (or emit a diagnostic) when rr is missing.
Do you want me to wire a diagnostic into scope.Diagnostics when rr == nil and return nil here?
19-45: Add unit tests for resolution paths.Please add tests covering: (a) missing symbol, (b) wrong symbol type, (c) successful resolution and JSON of underlying value, (d) nil rr.
I can scaffold tests under lang/constructs/worldgen/density_functions with a fake Scope/Symbol.
vendor/github.com/muesli/termenv/templatehelper.go (1)
14-16: Avoid shared mutable FuncMap in ASCII modeReturn a shallow copy to prevent accidental mutation across templates.
func TemplateFuncs(p Profile) template.FuncMap { if p == Ascii { - return noopTemplateFuncs + return cloneFuncMap(noopTemplateFuncs) } @@ var noopTemplateFuncs = template.FuncMap{ @@ } + +func cloneFuncMap(m template.FuncMap) template.FuncMap { + out := make(template.FuncMap, len(m)) + for k, v := range m { + out[k] = v + } + return out +}Also applies to: 66-78
vendor/github.com/lucasb-eyer/go-colorful/hsluv.go (2)
118-126: Fix typos in doc comments-// DistanceHSLuv calculates Euclidan distance in the HSLuv colorspace. No idea +// DistanceHSLuv calculates Euclidean distance in the HSLuv color space. No idea @@ -// DistanceHPLuv calculates Euclidean distance in the HPLuv colorspace. No idea +// DistanceHPLuv calculates Euclidean distance in the HPLuv color space. No ideaAlso applies to: 129-137
63-76: Optional: clamp L in HPLuvToLuvLCh for symmetry with HSLuvToLuvLCh- return l / 100.0, c / 100.0, h + return clamp01(l / 100.0), c / 100.0, hvendor/github.com/muesli/termenv/termenv_unix.go (2)
160-167: Correct the comment: ST is ESC backslash.Doc says “ST (ESC)”; should be “ST (ESC )”.
Also applies to: 205-209
116-138: Select timeout handling may drift after EINTR.Select can mutate timeval; consider recomputing remaining timeout after EINTR for stricter timing. Low impact.
vendor/github.com/lucasb-eyer/go-colorful/soft_palettegen.go (3)
30-31: Typos in comment (“windows-stype”, tone).Consider “windows-style Foo/FooEx.” Optional polish for vendored code.
121-125: Potential infinite loop if all samples marked used within an iteration.Unlikely given len(samples) > colorsCount, but add a bounded retry or fallback to break ties.
Also applies to: 135-145
51-61: Capacity hint underestimates due to inclusive loops.Harmless; consider +1 factors to reduce reallocs when ManySamples=false.
vendor/github.com/lucasb-eyer/go-colorful/hexcolor.go (2)
33-35: Return SQL NULL for nil receiver in ValuePrevents a panic when a nil *HexColor is used and preserves “unset” semantics as NULL.
-func (hc *HexColor) Value() (driver.Value, error) { - return Color(*hc).Hex(), nil -} +func (hc *HexColor) Value() (driver.Value, error) { + if hc == nil { + return nil, nil + } + return Color(*hc).Hex(), nil +}
59-67: Nice: envconfig Decode helperUseful for config ingestion. Consider also implementing encoding.TextMarshaler/TextUnmarshaler to cover more encoders (YAML/TOML). I can draft if you want.
vendor/github.com/muesli/termenv/color.go (2)
138-141: Guard against malformed xterm strings to avoid slicing panicsAdd minimal validation after Split to ensure three components with length ≥2.
h := strings.Split(s, "/") - hex := fmt.Sprintf("#%s%s%s", h[0][:2], h[1][:2], h[2][:2]) + if len(h) != 3 || len(h[0]) < 2 || len(h[1]) < 2 || len(h[2]) < 2 { + return RGBColor(""), ErrInvalidColor + } + hex := fmt.Sprintf("#%s%s%s", h[0][:2], h[1][:2], h[2][:2]) return RGBColor(hex), nil
55-68: ConvertToRGB silently ignores errorsFor NoColor/unknown types we return zero color via ignored error. Consider explicit handling (return black or an error) to aid debugging.
If you want, I can scan call sites to confirm assumptions about allowed Color types.
vendor/github.com/muesli/termenv/style.go (1)
30-36: Constructor defaults to ANSI; consider profile-aware ctorMinor: environments without color may prefer auto-detected profile (e.g., Ascii) here to avoid escape codes by default. Optional API addition: New(profile Profile, s ...string).
vendor/github.com/gorilla/websocket/server.go (1)
362-364: Fix comment typo.“vio” → “via”.
-// writeHook is an io.Writer that records the last slice passed to it vio +// writeHook is an io.Writer that records the last slice passed to it via // io.Writer.Write.grammar/Core_Lang.g4 (3)
19-21: Consolidate Mul/MulInt to avoid duplicate constructs.Both rules accept integers; keep one rule with number and drop the redundant Int-only variant.
-builder_Mul: '.Mul(' number ')'; -builder_MulInt: '.Mul(' Int ')'; +builder_Mul: '.Mul(' number ')';
10-10: Normalize builder naming (dot prefix).All builders use a leading dot except Smear. Make it consistent or clarify intentional difference.
-builder_Smear: 'Smear' '(' number ')'; +builder_Smear: '.Smear' '(' number ')';
35-37: Improve literal robustness (optional).
- Float doesn’t allow “1.”; consider supporting it.
- String disallows escapes; consider allowing
\",\\, etc.-Float: ('-'? [0-9]* '.' [0-9]+); +Float: '-'? ( [0-9]+ '.' [0-9]* | '.' [0-9]+ ); @@ -String: '"' ~[\r\n]* '"'; +String: '"' ( '\\' . | ~["\r\n] )* '"';vendor/github.com/muesli/termenv/termenv.go (1)
27-39: Recognize Cygwin/MSYS terminals in TTY check.Include IsCygwinTerminal for broader Windows shell support.
- return isatty.IsTerminal(o.TTY().Fd()) + fd := o.TTY().Fd() + return isatty.IsTerminal(fd) || isatty.IsCygwinTerminal(fd)grammar/DensityFunctions.g4 (5)
16-17: Rename Wierd → Weird in rule names for consistency.Rule names use "Wierd" while the literal uses "WeirdScaledSampler". Align identifiers to avoid confusion and mismatches in generated context types.
Apply:
- | densityFn_WierdScaledSampler + | densityFn_WeirdScaledSampler-densityFn_WierdScaledSampler: 'WeirdScaledSampler' NL* '(' NL* densityFn NL* ')' NL* (densityFn_WierdScaledSamplerBuilder NL*)*; +densityFn_WeirdScaledSampler: 'WeirdScaledSampler' NL* '(' NL* densityFn NL* ')' NL* (densityFn_WeirdScaledSamplerBuilder NL*)*;-densityFn_WierdScaledSamplerBuilder: builder_Type1 | builder_Type2 | builder_Noise; +densityFn_WeirdScaledSamplerBuilder: builder_Type1 | builder_Type2 | builder_Noise;Note: This changes generated symbol names (e.g., DensityFn_WeirdScaledSamplerContext). Please update any Go code referencing the old names.
Also applies to: 71-74
8-23: Allow chaining math operations; current grammar permits only one.If chaining like “A + B * C + D” should be valid, switch from “?” to “*” and consider precedence if needed.
Apply minimal change:
-) densityFn_Math?; +) (NL* densityFn_Math)*;Optionally, introduce precedence/associativity with explicit expr rules if required.
Also applies to: 93-93
78-83: Normalize NL handling inside parens for RangeChoice/Clamp.To keep the DSL newline-agnostic, add NL* around inner tokens.
-densityFn_RangeChoice: 'RangeChoice' NL* '(' densityFn ')' NL* (densityFn_RangeChoiceBuilder NL*)*; +densityFn_RangeChoice: 'RangeChoice' NL* '(' NL* densityFn NL* ')' NL* (densityFn_RangeChoiceBuilder NL*)*; -densityFn_Clamp: 'Clamp' NL* '(' densityFn ')' NL* (densityFn_ClampBuilder NL*)*; +densityFn_Clamp: 'Clamp' NL* '(' NL* densityFn NL* ')' NL* (densityFn_ClampBuilder NL*)*;
79-79: Movebuilder_InRange/builder_OutRangeinto DensityFunctions.g4
- Remove their definitions from grammar/Core_Lang.g4 (lines 27–28).
- In grammar/DensityFunctions.g4, alongside the other densityFn builders, add:
builder_InRange: '.InRange' '(' NL* densityFn NL* ')'; builder_OutRange: '.OutRange' '(' NL* densityFn NL* ')';This ensures all densityFn-dependent builders live in one grammar and avoids a Core_Lang ↔ DensityFunctions import cycle.
46-53: No LL(*) ambiguity: alternatives are token-disjoint
The twodensityFn_Noisealternatives are separated by lookahead on the next token—resourceReferencerequires an Identifier, whilenoiseDefinitionstarts with an Int—so the parser will always pick the correct branch and no grammar warnings should occur. (Optional: factor out the commonNoise… into a shared rule in grammar/DensityFunctions.g4 for readability.)vendor/github.com/muesli/termenv/output.go (2)
9-12: Global default Output is mutable without synchronization.SetDefaultOutput writes a package-level var with no sync. If changed at runtime across goroutines, add sync/atomic or document single-threaded initialization only.
Also applies to: 60-62
195-197: Micro-opt: use io.StringWriter when available.Avoid []byte allocation on WriteString.
-func (o Output) WriteString(s string) (int, error) { - return o.Write([]byte(s)) -} +func (o Output) WriteString(s string) (int, error) { + if sw, ok := o.tty.(interface{ WriteString(string) (int, error) }); ok { + return sw.WriteString(s) + } + return o.Write([]byte(s)) +}vendor/github.com/aymanbagabas/go-osc52/v2/osc52.go (1)
210-213: Doc typos.“Settting” → “Setting”; “escapse” → “escape”.
-// Strings longer than the limit get ignored. Settting the limit to 0 or a -// negative value disables the limit. Each terminal defines its own escapse +// Strings longer than the limit get ignored. Setting the limit to 0 or a +// negative value disables the limit. Each terminal defines its own escape // sequence limit.vendor/github.com/mattn/go-runewidth/runewidth.go (4)
254-275: Wrap should respect grapheme clusters; current rune-based wrap can split emojis/combining marksUse uniseg like StringWidth/Truncate to avoid breaking clusters.
Apply:
-func (c *Condition) Wrap(s string, w int) string { - width := 0 - out := "" - for _, r := range s { - cw := c.RuneWidth(r) - if r == '\n' { - out += string(r) - width = 0 - continue - } else if width+cw > w { - out += "\n" - width = 0 - out += string(r) - width += cw - continue - } - out += string(r) - width += cw - } - return out -} +func (c *Condition) Wrap(s string, w int) string { + var out string + width := 0 + g := uniseg.NewGraphemes(s) + for g.Next() { + cluster := g.Str() + // Handle explicit newlines as hard breaks. + if cluster == "\n" { + out += cluster + width = 0 + continue + } + // Approximate cluster width by first non-zero rune width (same heuristic as StringWidth). + cw := 0 + for _, r := range g.Runes() { + cw = c.RuneWidth(r) + if cw > 0 { + break + } + } + if width+cw > w { + out += "\n" + width = 0 + } + out += cluster + width += cw + } + return out +}
351-353: Fix LUT size doc typo: 557055 → 557056 bytesMatches max/2 from 0x110000. Purely a doc nit.
Apply:
-// CreateLUT will create an in-memory lookup table of 557055 bytes for faster operation. +// CreateLUT will create an in-memory lookup table of 557056 bytes for faster operation.
291-293: Docstring typo: FillRight says “filled in left”Clarify direction for both method and top-level wrapper docs.
Apply:
-// FillRight return string filled in left by spaces in w cells +// FillRight returns the string right-padded with spaces to width w (cells).Also applies to: 346-349
155-174: Guard CreateLUT usage (thread-safety)
CreateLUT mutates Condition.combinedLut without synchronization while RuneWidth/StringWidth read it—calling CreateLUT concurrently with width calculations races. Initialize the LUT once (e.g. at startup on the main goroutine) and avoid concurrent CreateLUT calls.vendor/github.com/lucasb-eyer/go-colorful/colors.go (1)
77-81: Doc nit: awkward wordingTweak comment grammar for Clamped().
Apply:
-// Returns Clamps the color into valid range, clamping each value to [0..1] -// If the color is valid already, this is a no-op. +// Clamps the color into a valid range, restricting each channel to [0..1]. +// If the color is already valid, this is a no-op.vendor/github.com/muesli/termenv/screen.go (1)
155-158: Doc fix: CursorForward moves horizontally, not “up”Update comment to avoid confusion.
Apply:
-// CursorForward moves the cursor up a given number of lines. +// CursorForward moves the cursor forward (right) by a given number of cells.
| github.com/spf13/pflag v1.0.6 // indirect | ||
| github.com/tliron/commonlog v0.2.8 // indirect | ||
| github.com/tliron/kutil v0.3.11 // indirect | ||
| golang.org/x/crypto v0.15.0 // indirect |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: golang.org/x/crypto v0.15.0 has multiple CRITICAL vulns — bump to a safe version
Static analysis flags several critical advisories on v0.15.0. Please upgrade x/crypto to the latest patched release and re-tidy.
Run to determine and apply the latest version, then verify vuln closure:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show latest available
go list -m -u -json golang.org/x/crypto | jq -r '.Path, ("current="+.Version), ("latest="+(.Update.version // "n/a"))'
# Upgrade to latest safe (edit if you must pin)
go get golang.org/x/crypto@latest
go mod tidy
# Optional: check for known vulns across deps (requires osv-scanner)
# osv-scanner --skip-git -r .Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Bypass vendor directory for upgrade check
go list -m -u -json golang.org/x/crypto -mod=mod | jq -r '.Path, ("current="+.Version), ("latest="+(.Update.version // "n/a"))'
# Upgrade to latest bypassing vendor
go get golang.org/x/crypto@latest -mod=mod
go mod tidy -mod=mod
# Optional: re-run vulnerability scan if osv-scanner is installed
# osv-scanner --skip-git -r .Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Force module mode to bypass vendor directory
export GOFLAGS=-mod=mod
# Show current and latest versions of x/crypto
go list -m -u -json golang.org/x/crypto | jq -r '.Path, ("current="+.Version), ("latest="+(.Update.version // "n/a"))'
# Upgrade to the latest version and tidy
go get golang.org/x/crypto@latest
go mod tidyLength of output: 969
Critical: Upgrade applied but tidy failing due to missing grammar module
- golang.org/x/crypto successfully upgraded from v0.15.0 → v0.42.0 (vulnerabilities addressed)
go mod tidynow errors ongithub.com/minecraftmetascript/mms/lang/grammar: ensure that package path exists or update the import/version for the grammar module before merging
🧰 Tools
🪛 OSV Scanner (2.1.0)
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: Man-in-the-middle attacker can compromise integrity of secure channel in golang.org/x/crypto
(GO-2023-2402)
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: Misuse of connection.serverAuthenticate may cause authorization bypass in golang.org/x/crypto
(GO-2024-3321)
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: Potential denial of service in golang.org/x/crypto
(GO-2025-3487)
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: Prefix Truncation Attack against ChaCha20-Poly1305 and Encrypt-then-MAC aka Terrapin
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: golang.org/x/crypto Vulnerable to Denial of Service (DoS) via Slow or Incomplete Key Exchange
[CRITICAL] 28-28: golang.org/x/crypto 0.15.0: Misuse of ServerConfig.PublicKeyCallback may cause authorization bypass in golang.org/x/crypto
🤖 Prompt for AI Agents
In go.mod around line 28, after upgrading golang.org/x/crypto, go mod tidy fails
because the import github.com/minecraftmetascript/mms/lang/grammar cannot be
resolved; verify that the module path and version exist (or correct the path),
then update go.mod to point to a valid module/version (or add a replace
directive to the correct local or upstream module path), run `go get
github.com/minecraftmetascript/mms@<valid-version>` (or `go mod edit
-replace=...` if using a fork/local copy), and finally run `go mod tidy` to
remove or fix broken imports; if the grammar package is no longer used, remove
its imports from source instead.
| densityFn_Spline: 'Spline' NL* (('(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)* ) | '(' number ')'); | ||
| densityFn_SplinePoint: '.Point' '(' number ',' (number | densityFn_Spline | resourceReference) ',' number ')'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate Spline ambiguity (both alts match Spline(number)).
As written, Spline(5) matches both alts since number ∈ densityFn_Constant. Require at least one .Point when using the densityFn alt, or drop the number-only alternative.
Apply:
-densityFn_Spline: 'Spline' NL* (('(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)* ) | '(' number ')');
+densityFn_Spline:
+ 'Spline' NL* (
+ '(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)+
+ | '(' NL* number NL* ')'
+ );📝 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.
| densityFn_Spline: 'Spline' NL* (('(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)* ) | '(' number ')'); | |
| densityFn_SplinePoint: '.Point' '(' number ',' (number | densityFn_Spline | resourceReference) ',' number ')'; | |
| densityFn_Spline: | |
| 'Spline' NL* ( | |
| '(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)+ | |
| | '(' NL* number NL* ')' | |
| ); |
🤖 Prompt for AI Agents
In grammar/DensityFunctions.g4 around lines 88-89, the Spline rule is ambiguous
because Spline(number) matches both alternatives; change the first alternative
that accepts '(' NL* densityFn NL* ')' NL* (densityFn_SplinePoint NL*)* to
require at least one .Point by making the trailing points repetition one-or-more
(i.e., change the '*' for densityFn_SplinePoint to '+') so Spline(number) no
longer matches that alt (alternatively remove the standalone '(' number ')'
alternative if you prefer the simpler syntax).
| type ReferenceFunction struct { | ||
| traversal.Construct | ||
| Ref *traversal.Reference | ||
| Value *traversal.Construct | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid pointer-to-interface; store Construct directly.
Pointers to interface are an anti-pattern and complicate JSON marshaling.
type ReferenceFunction struct {
traversal.Construct
Ref *traversal.Reference
- Value *traversal.Construct
+ Value traversal.Construct
}
@@
- if c.Value == nil {
- return c.Ref.MarshalJSON()
- }
- return (*c.Value).MarshalJSON()
+ if c.Value == nil {
+ return c.Ref.MarshalJSON()
+ }
+ return c.Value.MarshalJSON()Also applies to: 58-61
🤖 Prompt for AI Agents
In lang/constructs/worldgen/density_functions/FunctionRef.go around lines 13-17
(and similarly for lines 58-61), the struct uses pointer-to-interface fields
which is an anti-pattern and breaks JSON marshaling; change the types to store
the interfaces directly (e.g., Ref traversal.Reference and Value
traversal.Construct) instead of *traversal.Reference and *traversal.Construct,
then update all code that constructs or accesses these fields to pass/assign the
interface value (or use & when you actually need an addressable concrete
implementation), remove or adapt nil checks to use the zero-value interface
checks, and adjust any JSON tags/encoding logic if necessary so marshaling works
with the direct interface fields.
| func (p *Parser) Parse() (*grammar.ScriptContext, error) { | ||
| fmt.Println("Parsing...") | ||
| if res := p.parser.Script(); res == nil { | ||
| return nil, errors.New("failed to parse") | ||
| } else { | ||
| fmt.Println("Done parsing") | ||
| return res.(*grammar.ScriptContext), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug prints from library code
Stdout printing during parse is noisy and breaks CLI/editor integrations.
-func (p *Parser) Parse() (*grammar.ScriptContext, error) {
- fmt.Println("Parsing...")
- if res := p.parser.Script(); res == nil {
+func (p *Parser) Parse() (*grammar.ScriptContext, error) {
+ if res := p.parser.Script(); res == nil {
return nil, errors.New("failed to parse")
} else {
- fmt.Println("Done parsing")
return res.(*grammar.ScriptContext), nil
}
}Also drop the unused import:
-import (
- "errors"
- "fmt"
- "github.com/minecraftmetascript/mms/lang/grammar"
- "github.com/antlr4-go/antlr/v4"
-)
+import (
+ "errors"
+ "github.com/minecraftmetascript/mms/lang/grammar"
+ "github.com/antlr4-go/antlr/v4"
+)📝 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 (p *Parser) Parse() (*grammar.ScriptContext, error) { | |
| fmt.Println("Parsing...") | |
| if res := p.parser.Script(); res == nil { | |
| return nil, errors.New("failed to parse") | |
| } else { | |
| fmt.Println("Done parsing") | |
| return res.(*grammar.ScriptContext), nil | |
| } | |
| package traversal | |
| import ( | |
| "errors" | |
| "github.com/minecraftmetascript/mms/lang/grammar" | |
| "github.com/antlr4-go/antlr/v4" | |
| ) | |
| func (p *Parser) Parse() (*grammar.ScriptContext, error) { | |
| if res := p.parser.Script(); res == nil { | |
| return nil, errors.New("failed to parse") | |
| } else { | |
| return res.(*grammar.ScriptContext), nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/traversal/Parser.go around lines 30 to 37, remove the debug stdout
prints (the fmt.Println("Parsing...") and fmt.Println("Done parsing")) so
parsing is silent for library use, and remove the now-unused "fmt" import from
the file's import block; keep the error handling as-is and return the parsed
context or error as before.
| func (p *Parser) ExitNamespace(ctx *grammar.NamespaceContext) { | ||
| for _, contentBlockCtx := range ctx.AllContentBlocks() { | ||
| if contentBlock, ok := contentBlockCtx.(*grammar.ContentBlocksContext); ok { | ||
| inner := contentBlock.GetChild(0) | ||
| ConstructRegistry.Construct(inner.(antlr.ParserRuleContext), p.namespace, p.scope) | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Don’t assume first child is a rule; iterate rule-children safely
GetChild(0) may be a terminal; the type-assert can panic and you’ll skip additional content in the block.
func (p *Parser) ExitNamespace(ctx *grammar.NamespaceContext) {
for _, contentBlockCtx := range ctx.AllContentBlocks() {
if contentBlock, ok := contentBlockCtx.(*grammar.ContentBlocksContext); ok {
- inner := contentBlock.GetChild(0)
- ConstructRegistry.Construct(inner.(antlr.ParserRuleContext), p.namespace, p.scope)
+ for i := 0; i < contentBlock.GetChildCount(); i++ {
+ if prc, ok := contentBlock.GetChild(i).(antlr.ParserRuleContext); ok {
+ ConstructRegistry.Construct(prc, p.namespace, p.scope)
+ }
+ }
}
}
}📝 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 (p *Parser) ExitNamespace(ctx *grammar.NamespaceContext) { | |
| for _, contentBlockCtx := range ctx.AllContentBlocks() { | |
| if contentBlock, ok := contentBlockCtx.(*grammar.ContentBlocksContext); ok { | |
| inner := contentBlock.GetChild(0) | |
| ConstructRegistry.Construct(inner.(antlr.ParserRuleContext), p.namespace, p.scope) | |
| } | |
| } | |
| func (p *Parser) ExitNamespace(ctx *grammar.NamespaceContext) { | |
| for _, contentBlockCtx := range ctx.AllContentBlocks() { | |
| if contentBlock, ok := contentBlockCtx.(*grammar.ContentBlocksContext); ok { | |
| for i := 0; i < contentBlock.GetChildCount(); i++ { | |
| if prc, ok := contentBlock.GetChild(i).(antlr.ParserRuleContext); ok { | |
| ConstructRegistry.Construct(prc, p.namespace, p.scope) | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In lang/traversal/Parser.go around lines 40–47, the code assumes
contentBlock.GetChild(0) is a parser rule and type-asserts it directly, which
can panic for terminal nodes and ignores other rule-children; fix by iterating
over all children of contentBlock (0 .. GetChildCount()-1), for each child check
its dynamic type (or assert to antlr.ParserRuleContext safely) and only call
ConstructRegistry.Construct for children that are parser rule nodes, skipping
terminal nodes.
| if n == 0 { | ||
| panic("read returned no data") | ||
| } | ||
|
|
||
| return b[0], nil | ||
| } |
There was a problem hiding this comment.
Don’t panic on zero-length read; return an error.
Panic in vendored terminal IO is risky; return io.EOF instead.
- if n == 0 {
- panic("read returned no data")
- }
+ if n == 0 {
+ return 0, io.EOF
+ }📝 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 n == 0 { | |
| panic("read returned no data") | |
| } | |
| return b[0], nil | |
| } | |
| func (o *Output) readNextByte() (byte, error) { | |
| if !o.unsafe { | |
| if err := o.waitForData(OSCTimeout); err != nil { | |
| return 0, err | |
| } | |
| } | |
| var b [1]byte | |
| n, err := o.TTY().Read(b[:]) | |
| if err != nil { | |
| return 0, err //nolint:wrapcheck | |
| } | |
| if n == 0 { | |
| return 0, io.EOF | |
| } | |
| return b[0], nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/muesli/termenv/termenv_unix.go around lines 153 to 158,
replace the panic on zero-length read with a proper error return: when n == 0
return a zero byte and io.EOF (i.e., return 0, io.EOF) instead of panicking;
ensure the io package is imported if not already.
| if oscResponse { | ||
| // OSC can be terminated by BEL (\a) or ST (ESC) | ||
| if b == BEL || strings.HasSuffix(response, string(ESC)) { | ||
| return response, true, nil | ||
| } | ||
| } else { | ||
| // cursor position response is terminated by 'R' | ||
| if b == 'R' { | ||
| return response, false, nil |
There was a problem hiding this comment.
Fix OSC ST termination parsing (must be ESC backslash, not ESC alone).
Current logic returns on plain ESC and drops the trailing ‘\’, risking malformed responses and sync issues.
- if oscResponse {
- // OSC can be terminated by BEL (\a) or ST (ESC)
- if b == BEL || strings.HasSuffix(response, string(ESC)) {
- return response, true, nil
- }
- } else {
+ if oscResponse {
+ // OSC is terminated by BEL (\a) or ST (ESC \)
+ if b == BEL {
+ return response, true, nil
+ }
+ if b == ESC {
+ b2, err := o.readNextByte()
+ if err != nil {
+ return "", false, err
+ }
+ response += string(b2)
+ if b2 == '\\' {
+ return response, true, nil
+ }
+ // continue accumulating otherwise
+ continue
+ }
+ } else {
// cursor position response is terminated by 'R'
if b == 'R' {
return response, false, nil
}
}🤖 Prompt for AI Agents
In vendor/github.com/muesli/termenv/termenv_unix.go around lines 205 to 213, the
OSC termination logic currently treats a lone ESC as the ST terminator and
returns early, which drops the required trailing backslash and corrupts the
response; change the check so it only accepts the two-character ST sequence ESC
followed by '\' (i.e. verify the response ends with ESC+'\' or that the last two
bytes are ESC and backslash) and only then return the full response, true, nil;
keep the BEL handling as-is and do not alter the cursor-position ('R') branch.
| // first, send OSC query, which is ignored by terminal which do not support it | ||
| fmt.Fprintf(tty, OSC+"%d;?"+ST, sequence) | ||
|
|
||
| // then, query cursor position, should be supported by all terminals | ||
| fmt.Fprintf(tty, CSI+"6n") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check write errors when emitting OSC/CSI.
If writes fail, subsequent reads will block until timeout.
- fmt.Fprintf(tty, OSC+"%d;?"+ST, sequence)
+ if _, err := fmt.Fprintf(tty, OSC+"%d;?"+ST, sequence); err != nil {
+ return "", fmt.Errorf("%s: %w", ErrStatusReport, err)
+ }
- fmt.Fprintf(tty, CSI+"6n")
+ if _, err := fmt.Fprintf(tty, CSI+"6n"); err != nil {
+ return "", fmt.Errorf("%s: %w", ErrStatusReport, err)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vendor/github.com/muesli/termenv/termenv_unix.go around lines 260 to 265, the
two fmt.Fprintf calls that emit the OSC and CSI sequences do not check their
returned error values; if the writes fail, subsequent reads can block until
timeout. Update both writes to capture the (n, err) result and handle err
immediately (e.g., return the error or propagate it to the caller) instead of
proceeding to the read step; ensure any partial writes are treated as failures
and you do not attempt the blocking read when an error was returned from the
write calls.
| func Get() int64 { | ||
| // The goid is the 16th field in the G struct where each field is a | ||
| // pointer, uintptr or padded to that size. See runtime.h from the | ||
| // Go sources. I'm not aware of a cleaner way to determine the | ||
| // offset. | ||
| return *(*int64)(unsafe.Pointer(getg() + 16*pointerSize)) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm necessity of vendoring Go 1.4 runtime backdoor.
This unsafe, runtime-layout-dependent code is only for go1.4. If the project doesn’t support Go 1.4, consider dropping this file from the vendor set to reduce risk surface and maintenance.
🏁 Script executed:
#!/bin/bash
# Verify supported Go versions to justify keeping go1.4-specific vendor code.
if fd go.mod >/dev/null 2>&1; then
echo "go.mod:"
cat go.mod | sed -n '1,80p' | sed -n '/^module/p;/^go [0-9]\+\.[0-9]\+/p;/^toolchain /p'
else
echo "No go.mod found; check CI config or build scripts for Go version pins."
fiLength of output: 57
Drop Go 1.4–specific vendored file
The module’s go.mod specifies Go 1.23, so vendor/github.com/petermattis/goid/goid_go1.4.go will never be built. Remove that file to reduce maintenance and minimize unsafe, runtime‐layout–dependent code.
🤖 Prompt for AI Agents
In vendor/github.com/petermattis/goid/goid_go1.4.go around lines 28 to 34: this
file is a Go 1.4-specific, unsafe runtime-layout dependent implementation that
is never built because go.mod targets Go 1.23; delete the file from the
repository (git rm vendor/github.com/petermattis/goid/goid_go1.4.go), update the
vendor tree (regenerate vendor with go mod vendor or run the repo's standard
vendoring step), run go vet/go build to confirm no remaining references, and
commit the removal to avoid maintaining obsolete unsafe code.
| func ExtractGID(s []byte) int64 { | ||
| s = s[len("goroutine "):] | ||
| s = s[:bytes.IndexByte(s, ' ')] | ||
| gid, _ := strconv.ParseInt(string(s), 10, 64) | ||
| return gid | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden parsing to avoid panics on malformed input
ExtractGID assumes a well-formed runtime.Stack header; malformed input panics on slicing. Return a sentinel on failure.
func ExtractGID(s []byte) int64 {
- s = s[len("goroutine "):]
- s = s[:bytes.IndexByte(s, ' ')]
- gid, _ := strconv.ParseInt(string(s), 10, 64)
- return gid
+ const prefix = "goroutine "
+ if !bytes.HasPrefix(s, []byte(prefix)) {
+ return -1
+ }
+ s = s[len(prefix):]
+ i := bytes.IndexByte(s, ' ')
+ if i < 0 {
+ return -1
+ }
+ gid, err := strconv.ParseInt(string(s[:i]), 10, 64)
+ if err != nil {
+ return -1
+ }
+ return gid
}📝 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 ExtractGID(s []byte) int64 { | |
| s = s[len("goroutine "):] | |
| s = s[:bytes.IndexByte(s, ' ')] | |
| gid, _ := strconv.ParseInt(string(s), 10, 64) | |
| return gid | |
| } | |
| func ExtractGID(s []byte) int64 { | |
| const prefix = "goroutine " | |
| if !bytes.HasPrefix(s, []byte(prefix)) { | |
| return -1 | |
| } | |
| s = s[len(prefix):] | |
| i := bytes.IndexByte(s, ' ') | |
| if i < 0 { | |
| return -1 | |
| } | |
| gid, err := strconv.ParseInt(string(s[:i]), 10, 64) | |
| if err != nil { | |
| return -1 | |
| } | |
| return gid | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/petermattis/goid/goid.go around lines 24 to 29, ExtractGID
slices and indexes the input without validating it, which can panic on malformed
runtime.Stack output; change it to defensively validate before slicing: check
that the input is at least len("goroutine ")+1 bytes, verify bytes.IndexByte(s,
' ') returns >=0, use those safe indices to slice, handle strconv.ParseInt
errors and return a sentinel value (e.g. -1) on any failure instead of ignoring
errors; ensure no panics by guarding every slice/index operation and returning
the sentinel on validation or parse failure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flake.nix (2)
94-95: Bug: copying “.*” will match “.” and “..” and can fail or behave unpredictably.Replace the two cp lines with a single robust copy that includes dotfiles.
- cp $src/wasm/* $out/js -r - cp $src/wasm/.* $out/js -r + cp -r "$src/wasm/." "$out/js"
24-26: Align grammar directory references
The rule at lines 24–26 (and similarly at 54–57) points togrammars/Main_Lexer.g4, but that file isn’t present (onlygrammar/MinecraftMetascript.g4exists). Update these paths to match thegrammar/directory or add the missinggrammars/files to prevent build failures.
🧹 Nitpick comments (1)
flake.nix (1)
69-69: Version bump is fine; dedupe the literal by introducing a single version var.Avoid drift between default and wasm. Add a let-bound
mmsVersionand reference it in both places.Apply this diff within the selected lines:
- version = "0.3.2"; + version = mmsVersion;- version = "0.3.2"; + version = mmsVersion;Add this binding outside the selected ranges (near the existing
let):let mmsVersion = "0.3.2"; pkgs = import nixpkgs { inherit system; }; … inAlso applies to: 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flake.nix(2 hunks)grammar/Core_Lang.g4(1 hunks)grammar/DensityFunctions.g4(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- grammar/DensityFunctions.g4
- grammar/Core_Lang.g4
1 participant
Implements #2
Summary by CodeRabbit
New Features
Refactor
Tests
Chores