-
Notifications
You must be signed in to change notification settings - Fork 2
feat: support bracket-first parsing & parse variables with exported rules #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 significantly enhances the parser's robustness, particularly in handling list literals and module imports. It introduces a new bracket-first parsing strategy to correctly interpret complex array structures and enables the automatic export of variables as callable rules, making modules more flexible and interoperable. These changes aim to improve the overall reliability and expressiveness of the language.
Highlights
- Enhanced Parsing for Lists and Variables: Implemented 'bracket-first' parsing logic to robustly handle list literals, especially when used inline as parameters in function calls or mentions. This resolves issues where the parser previously struggled with list literals and indexing.
- Exporting Variables as Rules: Added the capability to automatically export variables defined within a module as callable rules. This improves the module's ability to expose its internal state or definitions for external use.
- New Feature Flag for Parsing Control: Introduced a
DISABLE_BRACKET_FIRST_PARSING
feature flag, allowing developers to disable the new bracket-first parsing behavior if it causes unforeseen issues, providing a fallback mechanism. - Streamlined Rule Definitions: Refactored the core parsing rules by removing redundant
Mention
andMentionScope
related definitions, simplifying the parser's rule set.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces bracket-first parsing to better handle list literals and enables exporting variables as callable rules from modules. The changes are logical and include relevant tests for the new functionality. My review focuses on the newly added parse-bracket.ts
file, where I've identified a few areas for improvement regarding performance, error handling, and robustness against stack overflows. Addressing these points will make the new parsing logic more resilient.
core/prepare/parse/parse-bracket.ts
Outdated
throw new Error( | ||
'Unmatched opening bracket at position ' + | ||
listOpeningBracketIndex, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the rest of the codebase and to ensure proper error handling, it's better to throw a YaksokError
instead of a generic Error
. This ensures that errors are caught and reported through the established mechanisms.
You will need to import YaksokError
from ../../error/common.ts
at the top of the file.
throw new Error( | |
'Unmatched opening bracket at position ' + | |
listOpeningBracketIndex, | |
) | |
throw new YaksokError({ | |
message: '대괄호의 짝이 맞지 않습니다.', | |
tokens: nodes[listOpeningBracketIndex].tokens, | |
}) |
const lastOpeningBracketTokenIndex = tokens.indexOf( | ||
lastOpeningBracketToken, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling tokens.indexOf()
inside this loop can lead to poor performance on large files, as it has a time complexity of O(M) where M is the number of tokens. Since this is inside a loop that iterates up to N times (number of nodes), the total complexity for finding brackets could approach O(N*M).
To optimize this, consider one of the following:
- Create a
Map<Token, number>
before the loop for O(1) token index lookups. - Modify the
Node
class to store the index of its starting token during an earlier processing stage. This would avoid the need for lookups here entirely.
...nodes.slice(closingPosition + 1), | ||
] | ||
|
||
return parseBracket(newNodes, tokens, dynamicRules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function calls itself recursively. If the input code contains a large number of bracket expressions (e.g., [1], [2], [3], ...
), this could lead to a stack overflow error. It would be more robust to convert this recursion into an iterative loop.
Here is an example of how you could structure it iteratively:
export function parseBracket(
nodes: Node[],
tokens: Token[],
dynamicRules: [Rule[][], Rule[][]],
) {
let currentNodes = nodes;
while (true) {
// ... logic to find one bracket pair ...
// (your current for-loop to find listOpeningBracketIndex)
if (listOpeningBracketIndex < 0) {
return currentNodes; // No more brackets to parse
}
// ... logic to find closing bracket and parse contents ...
const newNodes = [
...currentNodes.slice(0, listOpeningBracketIndex),
...childNodes,
...currentNodes.slice(closingPosition + 1),
];
currentNodes = newNodes;
}
}
Deploying dalbit-yaksok with
|
Latest commit: |
c72c49f
|
Status: | ✅ Deploy successful! |
Preview URL: | https://67d3a0a7.yaksok-ts.pages.dev |
Branch Preview URL: | https://fix-invalid-module-call.yaksok-ts.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the parser to handle bracket-first parsing of inline list literals, exports variable definitions as dynamic parsing rules, and centralizes error wrapping in FFI execution.
- Introduces
parseBracket
and a feature flag to disable bracket-first parsing - Adds
extractExportedVariables
to turnSetVariable
nodes into exported parser rules - Updates
YaksokSession
to catch and rethrow unexpected FFI errors in a unified exception type
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
core/prepare/parse/parse-bracket.ts | New function to perform bracket-prioritized parsing |
core/prepare/parse/index.ts | Integrates parseBracket , adds variable-export rules |
core/constant/feature-flags.ts | Defines DISABLE_BRACKET_FIRST_PARSING feature flag |
core/session/session.ts | Wraps FFI execution in try/catch, uses ErrorInFFIExecution |
test/* | Adjusts tests to assert on result objects over thrown exceptions |
deno.json, quickjs/deno.json, monaco-language-provider/deno.json | Version bumps |
@@ -15,6 +15,7 @@ import { | |||
type SessionConfig, | |||
} from './session-config.ts' | |||
|
|||
import { ErrorInFFIExecution } from '@dalbit-yaksok/core' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session error handling uses ErrorInFFIExecution
but tests expect ErrorOccurredWhileRunningFFIExecution
. Update the import and thrown exception class to match the API consumers' expectations.
import { ErrorInFFIExecution } from '@dalbit-yaksok/core' | |
import { ErrorOccurredWhileRunningFFIExecution } from '@dalbit-yaksok/core' |
Copilot uses AI. Check for mistakes.
const childNodes = callParseRecursively(indentedNodes, dynamicRules) | ||
const bracketParsedNodes = codeFile.session?.flags[ | ||
'disable-bracket-first-parsing' | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Access feature flags via the FEATURE_FLAG.DISABLE_BRACKET_FIRST_PARSING
constant instead of a raw string to prevent typos and improve maintainability.
Copilot uses AI. Check for mistakes.
위와 같은 복잡한 코드도 실행할 수 있습니다.
정리: 이제 배열과 불러오기에서 더욱 Robust합니다