Brackte first parsing in index fetching #120
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rycont, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily enhances the parsing logic to correctly handle index-based list access, especially when the index expression itself contains bracketed or complex arithmetic operations. It introduces a dedicated parsing function for this purpose and integrates it into the main parsing pipeline, ensuring robust interpretation of such language constructs. Additionally, it includes a minor version bump across all related components and adds a new test case to validate the new parsing behavior.
Highlights
- Enhanced Index Parsing: Introduced a new function,
parseIndexFetch, to accurately parse index-based list access. This function is designed to handle complex expressions within the indexing brackets, including recursive parsing of nested bracketed expressions. - Core Parsing Pipeline Integration: Integrated the new
parseIndexFetchlogic into the mainparsefunction withincore/prepare/parse/index.ts. This ensures that index-based access expressions are correctly processed as part of the overall parsing flow, applied after initial bracket parsing and before recursive child node parsing. - Version Increment: Bumped the project version from
2.2.0to2.2.1across the maindeno.jsonand component-specificdeno.jsonfiles (core, monaco-language-provider, quickjs). - New Test Case: Added a new test case in
test/codes/list.yakto specifically validate the correct parsing and evaluation of index-based list access with a complex arithmetic expression within the brackets. The corresponding expected output intest/codes/list.yak.outwas also updated.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes introduce a new parsing function for index-based list access. The implementation in core/prepare/parse/parse-index-fetch.ts has issues, including flawed logic for identifying index-fetch expressions, and a bug that can lead to infinite recursion. Addressing these will improve the robustness and correctness of the new parsing logic.
| if (closingPosition - indexingOpeningBracketIndex <= 2) { | ||
| return nodes | ||
| } |
There was a problem hiding this comment.
This check can lead to infinite recursion. If a [ is found that has 2 or fewer nodes inside (e.g., [a]), this function returns the original nodes array. The recursive call in return parseIndexFetch(newNodes, ...) will then operate on the exact same nodes array, find the same simple expression, and loop infinitely.
Instead of returning, the function should continue searching for another, more complex bracket expression to the left if the current one is too simple. This requires restructuring the function. A possible approach is to move the processing logic inside the for loop and use continue to skip simple expressions.
| for ( | ||
| indexingOpeningBracketIndex; | ||
| indexingOpeningBracketIndex >= 0; | ||
| indexingOpeningBracketIndex-- | ||
| ) { | ||
| const node = nodes[indexingOpeningBracketIndex] | ||
|
|
||
| if (!(node instanceof Expression) || node.value !== '[') { | ||
| continue | ||
| } | ||
|
|
||
| const lastOpeningBracketNode = nodes[indexingOpeningBracketIndex] | ||
| if ( | ||
| lastOpeningBracketNode instanceof Expression && | ||
| lastOpeningBracketNode.value === ')' | ||
| ) { | ||
| break | ||
| } | ||
|
|
||
| if (lastOpeningBracketNode instanceof ListLiteral) { | ||
| break | ||
| } | ||
|
|
||
| const lastOpeningBracketToken = lastOpeningBracketNode.tokens[0] | ||
| const lastOpeningBracketTokenIndex = tokens.indexOf( | ||
| lastOpeningBracketToken, | ||
| ) | ||
| const lastOpeningBracketPreToken = | ||
| tokens[lastOpeningBracketTokenIndex - 1] | ||
|
|
||
| if (!lastOpeningBracketPreToken) { | ||
| break | ||
| } | ||
|
|
||
| if (TOKEN_TYPE.IDENTIFIER === lastOpeningBracketPreToken.type) { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in this for loop for finding an index-fetch [ has several issues that make it incorrect. The conditions on lines 27-32 and 34-36 are unreachable because lastOpeningBracketNode is guaranteed to be an Expression with value: '['. They should be removed.
The condition if (!lastOpeningBracketPreToken) { break } on lines 45-47 incorrectly identifies a list literal at the start of a file (e.g., [1, 2, 3]) as an index fetch. An index fetch requires a value to be indexed.
The loop only considers [ preceded by an identifier as an index fetch. It misses other valid cases like (get_list())[0] (preceded by )) or list[0][1] (preceded by ]).
Consider refactoring this loop to be more explicit and correct. The goal is to find the rightmost [ that is part of an index fetch. An index fetch is typically preceded by an identifier, a closing parenthesis, or a closing bracket.
for (
indexingOpeningBracketIndex = nodes.length - 1;
indexingOpeningBracketIndex >= 0;
indexingOpeningBracketIndex--
) {
const node = nodes[indexingOpeningBracketIndex]
if (!(node instanceof Expression) || node.value !== '[') {
continue
}
const bracketToken = node.tokens[0]
const preToken = tokens[tokens.indexOf(bracketToken) - 1]
if (!preToken) {
// A `[` at the start of code is a list literal, not an index fetch.
continue
}
// Check if the bracket is for an index fetch.
// Index fetches are preceded by identifiers, closing parentheses, or closing brackets.
const isIndexFetch = preToken.type === TOKEN_TYPE.IDENTIFIER ||
preToken.type === TOKEN_TYPE.CLOSING_PARENTHESIS ||
preToken.type === TOKEN_TYPE.CLOSING_BRACKET
if (isIndexFetch) {
break // Found a candidate, break to process it.
}
}There was a problem hiding this comment.
이 for 루프의 인덱스-패치 [를 찾는 로직에 여러 가지 문제가 있어 정확하지 않습니다.
- 도달 불가능한 조건: 27-32행과 34-36행의 조건은
lastOpeningBracketNode가 항상value: '['를 가진Expression이기 때문에 도달할 수 없습니다. 이 부분은 제거해야 합니다. - 잘못된 리스트 리터럴 처리: 45-47행의
if (!lastOpeningBracketPreToken) { break }조건은 파일 시작 부분의 리스트 리터럴(예:[1, 2, 3])을 인덱스 패치로 잘못 식별합니다. 인덱스 패치는 인덱싱할 값이 필요합니다. - 누락된 유효한 케이스: 현재 루프는 식별자 앞에 오는
[만 인덱스 패치로 간주합니다.(get_list())[0](닫는 괄호 앞에 옴) 또는list[0][1](닫는 대괄호 앞에 옴)과 같은 다른 유효한 케이스를 놓치고 있습니다.
이 루프를 더 명확하고 정확하게 리팩토링하는 것을 고려해 주세요. 목표는 인덱스 패치의 일부인 가장 오른쪽에 있는 [를 찾는 것입니다. 인덱스 패치는 일반적으로 식별자, 닫는 괄호 또는 닫는 대괄호 앞에 옵니다.
제안했던 코드는 다음과 같습니다:
for (
indexingOpeningBracketIndex = nodes.length - 1;
indexingOpeningBracketIndex >= 0;
indexingOpeningBracketIndex--
) {
const node = nodes[indexingOpeningBracketIndex]
if (!(node instanceof Expression) || node.value !== '[') {
continue
}
const bracketToken = node.tokens[0]
const preToken = tokens[tokens.indexOf(bracketToken) - 1]
if (!preToken) {
// A `[` at the start of code is a list literal, not an index fetch.
continue
}
// Check if the bracket is for an index fetch.
// Index fetches are preceded by identifiers, closing parentheses, or closing brackets.
const isIndexFetch = preToken.type === TOKEN_TYPE.IDENTIFIER ||
preToken.type === TOKEN_TYPE.CLOSING_PARENTHESIS ||
preToken.type === TOKEN_TYPE.CLOSING_BRACKET
if (isIndexFetch) {
break // Found a candidate, break to process it.
}
}
There was a problem hiding this comment.
Pull Request Overview
This pull request adds bracket-first parsing functionality for index-based list access operations and bumps the project version to 2.2.1. The enhancement improves the parsing pipeline by handling bracketed expressions in index operations more effectively.
- Added a new
parseIndexFetchfunction to handle index-based list access with recursive bracket parsing - Integrated the new parsing function into the main parsing pipeline with conditional execution based on session flags
- Updated project version from 2.2.0 to 2.2.1 across all components
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/prepare/parse/parse-index-fetch.ts | New parsing function for index-based list access with bracket handling |
| core/prepare/parse/index.ts | Integration of parseIndexFetch into main parsing pipeline |
| test/codes/list.yak | Added test case for complex index expression parsing |
| test/codes/list.yak.out | Expected output for new test case |
| core/deno.json, deno.json, monaco-language-provider/deno.json, quickjs/deno.json | Version bump to 2.2.1 |
| const lastOpeningBracketNode = nodes[indexingOpeningBracketIndex] | ||
| if ( | ||
| lastOpeningBracketNode instanceof Expression && | ||
| lastOpeningBracketNode.value === ')' | ||
| ) { | ||
| break | ||
| } | ||
|
|
||
| if (lastOpeningBracketNode instanceof ListLiteral) { | ||
| break | ||
| } | ||
|
|
||
| const lastOpeningBracketToken = lastOpeningBracketNode.tokens[0] | ||
| const lastOpeningBracketTokenIndex = tokens.indexOf( | ||
| lastOpeningBracketToken, | ||
| ) | ||
| const lastOpeningBracketPreToken = | ||
| tokens[lastOpeningBracketTokenIndex - 1] |
There was a problem hiding this comment.
The variable name 'lastOpeningBracketNode' is misleading as it references the same node as line 20. Consider renaming to 'openingBracketNode' for clarity.
| const lastOpeningBracketNode = nodes[indexingOpeningBracketIndex] | |
| if ( | |
| lastOpeningBracketNode instanceof Expression && | |
| lastOpeningBracketNode.value === ')' | |
| ) { | |
| break | |
| } | |
| if (lastOpeningBracketNode instanceof ListLiteral) { | |
| break | |
| } | |
| const lastOpeningBracketToken = lastOpeningBracketNode.tokens[0] | |
| const lastOpeningBracketTokenIndex = tokens.indexOf( | |
| lastOpeningBracketToken, | |
| ) | |
| const lastOpeningBracketPreToken = | |
| tokens[lastOpeningBracketTokenIndex - 1] | |
| const openingBracketNode = nodes[indexingOpeningBracketIndex] | |
| if ( | |
| openingBracketNode instanceof Expression && | |
| openingBracketNode.value === ')' | |
| ) { | |
| break | |
| } | |
| if (openingBracketNode instanceof ListLiteral) { | |
| break | |
| } | |
| const openingBracketToken = openingBracketNode.tokens[0] | |
| const openingBracketTokenIndex = tokens.indexOf( | |
| openingBracketToken, | |
| ) | |
| const openingBracketPreToken = | |
| tokens[openingBracketTokenIndex - 1] |
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
|
|
||
| if (closingPosition - indexingOpeningBracketIndex <= 2) { |
There was a problem hiding this comment.
The magic number '2' should be replaced with a named constant or explained with a comment to clarify why this specific threshold is used.
Copilot uses AI. Check for mistakes.
Deploying dalbit-yaksok with
|
| Latest commit: |
333f574
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b1bc4d1a.yaksok-ts.pages.dev |
| Branch Preview URL: | https://brackte-first-parsing-in-ind.yaksok-ts.pages.dev |
…timistically method
This pull request introduces a new parsing function for index-based list access, updates the version across multiple components, and adds a test case to validate the new functionality. Below are the key changes grouped by theme:
Parsing Enhancements:
parseIndexFetchfunction incore/prepare/parse/parse-index-fetch.tsto handle parsing of index-based list access, including recursive parsing of bracketed expressions.parseIndexFetchinto the main parsing pipeline incore/prepare/parse/index.ts. This includes importing the new function and modifying theparsefunction to process nodes with index-based access. [1] [2]Version Updates:
2.2.0to2.2.1in multipledeno.jsonfiles across the project, includingcore/deno.json,monaco-language-provider/deno.json,quickjs/deno.json, anddeno.json. [1] [2] [3] [4]Testing:
test/codes/list.yakto validate index-based list access parsing.test/codes/list.yak.outto reflect the new test case, confirming correct parsing and execution.