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

Enhancements to AST traversal #2944

Merged
merged 20 commits into from
Apr 22, 2024
Merged

Enhancements to AST traversal #2944

merged 20 commits into from
Apr 22, 2024

Conversation

katsmil
Copy link
Contributor

@katsmil katsmil commented Apr 9, 2024

This PR is part of my earlier PR on providing pipeline suggestions through the Gleam LSP.
I identified some potential enhancements to the Gleam AST traversal algorithm, and as a result, I made the following changes:

  • Pruning the AST tree at the definition level.
  • Pruning the AST tree at the statement level.
  • Implementing early exit logic to handle cases where elements in expressions cannot correspond to the searched index (as described in the comment above the function).
  • Introducing coverage marks to ensure certain code paths are executed.

I believe these enhancements will contribute to the overall effectiveness and efficiency of the pipeline suggestion feature.
I welcome any feedback or suggestions for improvement. 😃

katsmil and others added 5 commits February 9, 2024 20:35
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Hello! Thank you.

Could you add comments to explain the additional logic, and could you remove the cov_mark tool please. We intentionally don't test implementation details as it makes maintaining harder rather than easier.

@katsmil
Copy link
Contributor Author

katsmil commented Apr 12, 2024

I provided some additional comments to the code and removed the cov_mark tool.
Curious if the comments are descriptive enough.

katsmil and others added 4 commits April 16, 2024 10:18
* pruning on definition nodes
* pruning on statement nodes
* early exit list, block and tuple expressions
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you

compiler-core/src/ast.rs Show resolved Hide resolved
#[test]
fn find_node_sequence_early_exit() {
let block = compile_expression(r#"{ 1 2 3 }"#);
assert!(block.find_node(1).is_none());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return the block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test, but it should not return the block, as the test find_node_sequence() shows.

compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast/tests.rs Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

@lpil
Copy link
Member

lpil commented Apr 19, 2024

Could you rebase on main to remove the merge conflict please 🙏

@katsmil
Copy link
Contributor Author

katsmil commented Apr 19, 2024

I am not really sure how to fix this failing check, sorry.
Does anyone have a suggestion?

@Acepie
Copy link
Contributor

Acepie commented Apr 19, 2024

Oh yeah there was a vulnerability that apparently was surfaced today. you can fix by running cargo update rustls and commiting the lock change

@Acepie
Copy link
Contributor

Acepie commented Apr 19, 2024

34864ed i had to make the same change here to get things working

@lpil lpil enabled auto-merge (squash) April 22, 2024 16:43
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

@lpil lpil merged commit 92ae5b2 into gleam-lang:main Apr 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants