Skip to content
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

New AST types #8263

Merged
merged 62 commits into from
Dec 5, 2023
Merged

New AST types #8263

merged 62 commits into from
Dec 5, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Nov 8, 2023

Pull Request Description

Introduce new AST type; use it to replace current uses of AstExtended; for now edits and synchronization are implemented on the old mechanisms (text edits / RelativeRange id map).

Important Notes

  • Edit-related code is commented out until the next PR because it is incompatible with the transitional IdMap-based synchronization.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw added the -gui label Nov 8, 2023
@kazcw kazcw self-assigned this Nov 8, 2023
@kazcw kazcw mentioned this pull request Nov 8, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 8, 2023
app/gui2/src/util/ast/abstract.ts Outdated Show resolved Hide resolved
* represent the structure are materialized (e.g. parentheses are inserted as needed.) During this printing process,
* the `Tree`s corresponding to preexisting AST nodes are consulted, in order to ensure that the concrete syntax of
* unmodified parts of the output matches the input. This is the only use of `Tree` in the frontend outside of the
* process of translating from `Tree` to frontend AST.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this will necessarily be the only use. The concrete AST might still be more suited for things like alias analysis, unless you think that it can be easily implemented on top of this abstract AST as well.

app/gui2/src/util/ast/abstract.ts Outdated Show resolved Hide resolved
@kazcw
Copy link
Contributor Author

kazcw commented Nov 20, 2023

abstract.ts is ready for review--I don't expect any further changes, except to the editing functionality which is currently commented. WIP changes to the application logic are included to demonstrate usage of the new APIs.

The rest of the API changes, except for using the new AST for editing, will be ready soon (hopefully today). Editing will be a follow-up PR.

@kazcw kazcw requested review from Frizi and farmaazon November 20, 2023 15:02
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked if my comments are resolved, but we agreed to merge it to unblock work. I will re-review the AST-related code once it will be more or less done (with editing).

@kazcw kazcw marked this pull request as ready for review November 29, 2023 15:28
@Frizi Frizi added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Dec 5, 2023
@mergify mergify bot merged commit 98988e8 into develop Dec 5, 2023
31 of 33 checks passed
@mergify mergify bot deleted the wip/kw/ast branch December 5, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants