[csharp/v8-spec] Continuation of improvements.#4799
Merged
Conversation
…toring. Three rules — null_coalescing_expression, expression, and range_expression — had alternatives sharing a long common prefix (unary_expression or conditional_or_expression), forcing ANTLR4's SLL simulation to scan up to 30 tokens before resolving the decision. Left-factor each rule so the shared prefix is consumed unconditionally and only a single token of lookahead is needed for the optional suffix. Add design docs explaining the problem and fix for each rule. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (decision 31)
Extend IsConstantPatternAhead() to return false when LT(1) is '(' and a
comma appears at parenthesis depth 1 — indicating a tuple-positional pattern
such as ("rock", "scissors") or (0, 0), which cannot be a compile-time
constant. Add IsPositionalPatternAhead() as its complement and gate the
positional_pattern alternative in the grammar with it, making alts 2 and 4
mutually exclusive. Add design/pattern_positional.md documenting the
ambiguity and the fix.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… to all targets Apply the same tuple-comma check and IsPositionalPatternAhead complement to all remaining language targets: Antlr4ng, Cpp (header + impl), Dart, Go, Java, JavaScript, Python3, and TypeScript. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues performance-oriented refactoring of the csharp/v8-spec ANTLR grammar, primarily by left-factoring ambiguous rules to reduce SLL lookahead (max-k) and by adding/adjusting semantic predicates for pattern disambiguation. It also adds design notes documenting the underlying ambiguity/perf issues and the chosen fixes.
Changes:
- Left-factored several expression-related rules (
range_expression,null_coalescing_expression,expression, plus a refactor ofconditional_expression) to reduce max-k. - Introduced
IsPositionalPatternAheadand expandedIsConstantPatternAheadacross all targets, and gatedpositional_patterninpatternto addressconstant_patternvspositional_patternambiguity. - Added multiple design documents explaining the root causes and refactor strategy; small template/metadata tweaks.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| csharp/v8-spec/design/range_expression.md | New design note explaining SLL max-k issue and left-factoring fix for range_expression. |
| csharp/v8-spec/design/null_coalescing_expression.md | New design note explaining SLL max-k issue and left-factoring fix for null_coalescing_expression. |
| csharp/v8-spec/design/expression.md | New design note explaining SLL max-k issue and left-factoring fix for expression. |
| csharp/v8-spec/design/pattern_positional.md | New design note describing constant_pattern vs positional_pattern disambiguation strategy. |
| csharp/v8-spec/design/primary_expression_mlr.md | New design note describing removal of mutual left-recursion in primary_expression. |
| csharp/v8-spec/desc.xml | Removes a comment from the test descriptor (no functional change). |
| csharp/v8-spec/CSharpParser.g4 | Adds predicate gate for positional_pattern; left-factors range_expression, null_coalescing_expression, conditional_expression, and expression. |
| csharp/v8-spec/CSharp/CSharpParserBase.cs | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Java/CSharpParserBase.java | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Go/CSharpParserBase.go | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Dart/CSharpParserBase.dart | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/JavaScript/CSharpParserBase.js | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/TypeScript/CSharpParserBase.ts | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Antlr4ng/CSharpParserBase.ts | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Python3/CSharpParserBase.py | Extends IsConstantPatternAhead and adds IsPositionalPatternAhead. |
| csharp/v8-spec/Cpp/CSharpParserBase.h | Declares IsPositionalPatternAhead. |
| csharp/v8-spec/Cpp/CSharpParserBase.cpp | Extends IsConstantPatternAhead and defines IsPositionalPatternAhead. |
| _scripts/templates/CSharp/st.Test.cs | Updates ambig flag parsing and adjusts start-rule lookup placeholder in the template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ional_expression, doc cleanup
- All targets: fix IsConstantPatternAhead() to detect type-headed positional
patterns like Point(0,0) where LT(1) is an identifier, not '('.
C#/Java/Python3/Cpp use speculative type_() parse; Antlr4ng/TS/JS/Go/Dart
use LT(1)==identifier && LT(2)=='(' heuristic.
- CSharpParser.g4: rewrite conditional_expression to use explicit '?'
optional instead of the empty-alt form.
- design/primary_expression_mlr.md: fix stray backslashes in code blocks
(change '\' to '//' to match commenting style).
- design/pattern_positional.md: correct last table row — alt 2 is false
(speculative type_() + '(' suppresses it), alt 4 is true.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… before left-factoring Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
|
@kaby76 thanks! |
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.
This is a continuation on the improvements for the csharp/v8-spec/ grammar. The changes here refactor a few more rules to reduce max-k's. The parser now completes the test suite twice as fast as compared to the grammar from the specification.
There are still quite a few performance issues, but I'm committing this group of changes as a checkpoint.
@teverett All set for merge. Ty.