Repository navigation
Fix NoiseSettings value completion (empty field values, material_rule lists) - #146
Merged
Merged
Conversation
`default_block =` followed by `default_fluid = water` on the next line parsed as `default_block = default_fluid`, then failed on the second '='. Completion at the empty value then saw a field-key position and offered sibling fields instead of block states. Recovery now inserts a synthetic MISSING token after an '=' whose value is absent: the next token closes a bracket or is a comma, or the next line starts another `key =`. MISSING is a primary that becomes Bad, so the field keeps a Bad value, the next field parses intact, and the parser reports one "expected a value after '='" error. A grammar predicate was tried first, but ANTLR evaluates predicates in nested blocks at the outer decision's position during prediction, which broke inline function blocks; preprocessing the token stream avoids that. Function checking no longer adds "not a value a function can compute with" on a Bad value, since the parser already reported it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AX3TsxdA6e59tBa7W4Fhh4
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Add missing-value parser token engine/syntax/grammar/*, engine/syntax/parser/* |
The grammar and generated parser recognize the recovery-only MISSING token as a primary expression. Generated token IDs and parser metadata are updated. |
Insert missing values and report diagnostics engine/syntax/recover.go, engine/syntax/parse.go, engine/sema/fn.go, engine/syntax/testdata/recovery/* |
The parser inserts synthetic tokens after selected omitted values and records syntax/missing-value diagnostics. Semantic analysis does not add another error for syntax.Bad expressions. Recovery fixtures and expected outputs are updated. |
Update list-item completion engine/sema/ide.go, engine/ide/ide.go, engine/ide/complete_matrix_test.go |
ListItemInfo selects a normalized tMaterialRule member from a union. Completion returns no candidates when a list item has no type information. Matrix tests add empty-value and surface-rule list cases. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Sequence Diagram(s)
sequenceDiagram
participant Parse
participant markMissingValues
participant MMSParser
participant FileDiagnostics
Parse->>markMissingValues: scan tokens and return inserted tokens and diagnostics
markMissingValues-->>Parse: synthetic MISSING tokens and diagnostics
Parse->>MMSParser: parse tokens containing MISSING
Parse->>FileDiagnostics: append missing-value diagnostics
Suggested reviewers: claude
Merge Risk: 
844fd
-
The supplied evidence identifies no actionable issue blocking merge; normal checks remain appropriate.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main completion changes for empty field values and material_rule lists. It is concise and directly related to the changeset. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 8 files. (10 skipped: 10 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the tokens in a row,
A missing value gets a marker to show.
The parser keeps the next field in sight,
Completions sort their choices right.
I twitch my nose and hop away.
`material_rule = [ | ]` in NoiseSettings offered the object's own fields (aquifers, debug_functions). The field's type is a union of a rule id and an inline MaterialRule, so ListItemInfo did not see the Sequence sugar it already handles for a bare SurfaceRule, and completion fell through to the enclosing block's fields. ListItemInfo now picks the MaterialRule member of a union, and completion inside a list stops there instead of offering the enclosing object's fields when the list's item type is unknown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AX3TsxdA6e59tBa7W4Fhh4
ItsMeBrianD
changed the title
`sea_level = {` offered NoiseSettings' own fields inside the braces,
because completion fell through from the inner block (no object type) to
the enclosing one. An object literal whose type has no fields now offers
nothing. Inline definitions such as `parent = { ... }` still complete
their kind's fields.
Found by probing every field of every declaration kind, two levels deep,
with `= |`, `= [|]`, a multi-line list and a multi-line object.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AX3TsxdA6e59tBa7W4Fhh4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Brian · project thread
Two autocomplete bugs in
NoiseSettingsbodies, both offering the object's own field names where a value belongs.1. Empty value before the next field
Typing
default_block =on a line abovedefault_fluid = wateroffered sibling field names (default_fluid,aquifers, ...) instead of block states, and put a red squiggle on the next field. The parser read the two lines asdefault_block = default_fluid, then failed on the second=.syntax/recover.go,markMissingValues) inserts a syntheticMISSINGtoken after an=whose value is absent: the next token is a closer or a comma, or the next line starts anotherkey =. No expression contains a bare=, so a key followed by=on a later line is always a new field or statement.MISSINGis declared in the lexer'stokens {}and is aprimaryalternative, so it becomesBad. The field keeps aBadvalue, the next field parses intact, and there is onesyntax/missing-valueerror on the=. It is hidden from "expecting {...}" messages.Badvalue, since the parser already reported it.2. Inside
material_rule = [ ... ]A list there is Sequence sugar, but completion offered
aquifersanddebug_functions. The field's type is a union of a rule id and an inlineMaterialRule, soListItemInfodid not see the sugar it already handles for a bareSurfaceRule, and completion fell through to the enclosing block's fields.ListItemInfonow picks theMaterialRulemember of a union.Judgement calls
declBody'svalueFollows). ANTLR evaluates predicates found inside nested blocks during prediction at the outer decision's position, and that broke inline function blocks (on_right_click = { /... }). Preprocessing the token stream avoids predicates entirely.tokens { MISSING }renumbers every token type in the generated parser, which is why the generated diff is large. Nothing outside the generated code uses raw numbers.Ore(size =)and{ a = }now also report "expected a value after '='" instead of a generic parse error.Tests
}, and before a quoted key; a multi-linematerial_rule = [list, and its second item.field_missing_value_next_field.mmscovers nested objects, helper args, and function bodies. Two existing goldens now show the clearer message.go test ./...inengine/passes, andgen.shoutput is checked in.🤖 Generated with Claude Code
https://claude.ai/code/session_01AX3TsxdA6e59tBa7W4Fhh4
Summary by CodeRabbit