[Merged by Bors] - Implement AST Visitor pattern (attempt #3) #2392

addisoncrump · 2022-10-29T11:31:14Z

This Pull Request closes no specific issue, but allows for analysis and post-processing passes by both internal and external developers.

It changes the following:

Adds a Visitor trait, to be implemented by visitors of a particular node type.
Adds TypeVisitor traits which offer access to private members of a node.
Adds an example which demonstrates the use of Visitor traits by walking over an AST and printing its contents.

At this time, the PR is more of a demonstration of intent rather than a full PR. Once it's in a satisfactory state, I'll mark it as not a draft.

addisoncrump · 2022-10-29T11:31:53Z

cc: @jedel1043

codecov · 2022-10-29T11:42:50Z

Codecov Report

Merging #2392 (468ecb9) into main (c72e4c2) will decrease coverage by 1.25%.
The diff coverage is 0.00%.

❗ Current head 468ecb9 differs from pull request most recent head 58aa397. Consider uploading reports for the commit 58aa397 to get more accurate results

@@            Coverage Diff             @@
##             main    #2392      +/-   ##
==========================================
- Coverage   39.61%   38.36%   -1.26%     
==========================================
  Files         307      310       +3     
  Lines       23507    24273     +766     
==========================================
  Hits         9313     9313              
- Misses      14194    14960     +766

Impacted Files	Coverage Δ
boa_engine/src/syntax/ast/declaration/mod.rs	`55.31% <0.00%> (-23.47%)`	⬇️
boa_engine/src/syntax/ast/declaration/variable.rs	`27.27% <0.00%> (-12.73%)`	⬇️
boa_engine/src/syntax/ast/expression/access.rs	`17.64% <0.00%> (-5.43%)`	⬇️
boa_engine/src/syntax/ast/expression/await.rs	`7.14% <0.00%> (-1.20%)`	⬇️
boa_engine/src/syntax/ast/expression/call.rs	`21.95% <0.00%> (-11.39%)`	⬇️
boa_engine/src/syntax/ast/expression/identifier.rs	`14.28% <0.00%> (-1.51%)`	⬇️
..._engine/src/syntax/ast/expression/literal/array.rs	`25.80% <0.00%> (-6.20%)`	⬇️
...oa_engine/src/syntax/ast/expression/literal/mod.rs	`23.33% <0.00%> (-5.84%)`	⬇️
...ne/src/syntax/ast/expression/literal/object/mod.rs	`46.93% <0.00%> (-6.55%)`	⬇️
...gine/src/syntax/ast/expression/literal/template.rs	`8.10% <0.00%> (-3.90%)`	⬇️
... and 43 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

addisoncrump · 2022-10-29T11:43:18Z

boa_engine/src/value/mod.rs

-pub use equality::*;
-pub use hash::*;
+// pub use equality::*;
+// pub use hash::*;


This was causing a build failure for me locally...

error: unreachable `pub` item --> boa_engine/src/value/mod.rs:43:9 | 43 | pub use equality::*; | --- ^^^^^^^^ | | | help: consider restricting its visibility: `pub(crate)` | = help: or consider exporting it for use by other crates note: the lint level is defined here --> boa_engine/src/lib.rs:41:5 | 41 | unreachable_pub, | ^^^^^^^^^^^^^^^

Didn't mean to commit it, but easy enough to revert.

If this didn't break any APIs, I think you can remove it

boa_engine/src/syntax/ast/statement_list/mod.rs

boa_engine/src/syntax/ast/visitor.rs

addisoncrump · 2022-11-01T13:55:41Z

Okay -- I have updated the style of visitor. Unsure how licensing here needs to work, but seeing as it's only a few lines that are exact or near-exact matches (given that you can't do them in literally any other way), I expect this is not a major concern.

boa_engine/src/syntax/ast/visitor.rs

Razican

I'm happy with the design :) It just needs some documentation on macros and PrinterVisitor, and the implementation of the implementers, but I'm happy about its look. I'd like to know the thoughts of others too.

jedel1043

The API looks perfect! Missing implementations aside, I think it would be good to have an example on how to implement Visitor in the module documentation (e.g. https://docs.rs/serde/latest/serde/de/trait.Visitor.html).

boa_examples/Cargo.toml

jedel1043 · 2022-11-01T20:58:12Z

Changes look good! It's just missing some clippy fixes.

jedel1043

Just remains addressing the commented out uses from the value module. Everything else looks really good. Very nice job!

jedel1043 · 2022-11-01T21:39:25Z

Ah no, I meant deleting the uses altogether, since those modules don't export any new types.

Razican

Everything looks good to me. The only thing here is that we might want to feature-gate this, and not compile the full visitor implementation for all cases, since we are not using it in the engine itself, if I'm not mistaken, at least for now.

Style-wise, we usually prefer use statements to be grouped, and in levels, but we have a bit of a mess, and this can be solved by the nightly cargo fmt, so that could probably be a separate thing.

In any case, I'm OK to merge this as-is and then add the feature before the 0.17 in a separate PR, for example.

addisoncrump · 2022-11-02T15:09:13Z

I would offer to maybe replace some internal mechanisms. For example: https://github.com/boa-dev/boa/blob/main/boa_engine/src/syntax/ast/statement/mod.rs#L140

jedel1043 · 2022-11-02T15:17:39Z

Yeah, I was planning on replacing contains and contains_arguments with a visitor

addisoncrump · 2022-11-02T15:19:50Z

For sure. Unless I'm mistaken, the code I linked becomes:

    pub(crate) fn var_declared_names(&self, vars: &mut FxHashSet<Identifier>) {
        struct IdentVisitor<'a> {
            vars: &'a mut FxHashSet<Identifier>,
        }

        impl<'a, 'ast> Visitor<'ast> for IdentVisitor<'a> {
            type BreakTy = Infallible;

            fn visit_identifier(&mut self, node: &'ast Identifier) -> ControlFlow<Self::BreakTy> {
                self.vars.insert(*node);
                ControlFlow::Continue(())
            }
        }

        let mut visitor = IdentVisitor { vars };
        visitor.visit_statement(self);
    }

addisoncrump · 2022-11-02T15:32:26Z

Hm, actually, that causes a bunch of errors. I am not sure why.

jedel1043 · 2022-11-02T15:34:42Z

Hm, actually, that causes a bunch of errors. I am not sure why.

Not the correct definition. That includes vars inside functions, which is incorrect since those are part of the var environment of the function. Here's the correct algorithm for var_declared_names:

https://tc39.es/ecma262/#sec-static-semantics-vardeclarednames

EDIT: It also includes all other identifiers, including let, const and undeclared variables, which should be excluded.

jedel1043 · 2022-11-02T15:45:00Z

Gonna merge this as it is, and I'll create some PRs to take advantage of the new visitor.

bors r+

This Pull Request closes no specific issue, but allows for analysis and post-processing passes by both internal and external developers. It changes the following: - Adds a Visitor trait, to be implemented by visitors of a particular node type. - Adds `Type`Visitor traits which offer access to private members of a node. - Adds an example which demonstrates the use of Visitor traits by walking over an AST and printing its contents. At this time, the PR is more of a demonstration of intent rather than a full PR. Once it's in a satisfactory state, I'll mark it as not a draft. Co-authored-by: Addison Crump <[email protected]>

bors · 2022-11-02T16:59:33Z

Pull request successfully merged into main.

Build succeeded:

addisoncrump commented Oct 29, 2022

View reviewed changes

Razican requested review from jedel1043, Razican, HalidOdat, jasonwilliams, RageKnify and raskad October 29, 2022 14:47

jedel1043 reviewed Oct 29, 2022

View reviewed changes

boa_engine/src/syntax/ast/statement_list/mod.rs Outdated Show resolved Hide resolved

boa_engine/src/syntax/ast/visitor.rs Outdated Show resolved Hide resolved

use chalk/swc patterns

ad14efe

addisoncrump force-pushed the ast-walking-3 branch from 58fa8c2 to ad14efe Compare November 1, 2022 13:54

addisoncrump added 4 commits November 1, 2022 14:56

make fmt happy

9b8089a

whoops, missed a spot

548a44b

more precise missing_docs

3905165

again

481c433

jedel1043 reviewed Nov 1, 2022

View reviewed changes

boa_engine/src/syntax/ast/visitor.rs Outdated Show resolved Hide resolved

Razican reviewed Nov 1, 2022

View reviewed changes

addisoncrump added 7 commits November 1, 2022 16:42

Create documentation for Visitor{,Mut} dynamically

5250407

expand for declaration, statement; move V parameter to function

0bbe562

expand for Function

803ed95

expand other function types

350656d

expand class

b0b7433

expand variable

fe4bbd5

expand block

0b57f6b

jedel1043 reviewed Nov 1, 2022

View reviewed changes

addisoncrump added 3 commits November 1, 2022 17:55

expand vardeclaration

b2e730e

expand expression

4743d45

loops

49e9d15

addisoncrump added 2 commits November 1, 2022 20:43

finish visitors

0e06f05

simplify visitor example

61ca13d

jedel1043 reviewed Nov 1, 2022

View reviewed changes

boa_examples/Cargo.toml Outdated Show resolved Hide resolved

addisoncrump added 3 commits November 1, 2022 21:20

Add VisitorMut example

dc9aa74

simplify Cargo.toml

1dc7657

cleanup examples

0953cfc

addisoncrump marked this pull request as ready for review November 1, 2022 20:26

addisoncrump added 3 commits November 1, 2022 22:10

apply clippy fixes

c1ce837

fixup fmt

88c6f01

{std=>core}::ops::ControlFlow

468ecb9

jedel1043 approved these changes Nov 1, 2022

View reviewed changes

undo comment

04b2482

poof

58aa397

jedel1043 added API ast Issue surrounding the abstract syntax tree enhancement New feature or request labels Nov 2, 2022

jedel1043 added this to the v0.17.0 milestone Nov 2, 2022

Razican approved these changes Nov 2, 2022

View reviewed changes

bors bot changed the title ~~Implement AST Visitor pattern (attempt #3)~~ [Merged by Bors] - Implement AST Visitor pattern (attempt #3) Nov 2, 2022

bors bot closed this Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Implement AST Visitor pattern (attempt #3) #2392

[Merged by Bors] - Implement AST Visitor pattern (attempt #3) #2392

addisoncrump commented Oct 29, 2022

addisoncrump commented Oct 29, 2022

codecov bot commented Oct 29, 2022 •

edited

Loading

addisoncrump Oct 29, 2022

jedel1043 Nov 1, 2022 •

edited

Loading

addisoncrump commented Nov 1, 2022

Razican left a comment

jedel1043 left a comment

jedel1043 commented Nov 1, 2022

jedel1043 left a comment

jedel1043 commented Nov 1, 2022

Razican left a comment

addisoncrump commented Nov 2, 2022

jedel1043 commented Nov 2, 2022

addisoncrump commented Nov 2, 2022

addisoncrump commented Nov 2, 2022

jedel1043 commented Nov 2, 2022 •

edited

Loading

jedel1043 commented Nov 2, 2022

bors bot commented Nov 2, 2022

[Merged by Bors] - Implement AST Visitor pattern (attempt #3) #2392

[Merged by Bors] - Implement AST Visitor pattern (attempt #3) #2392

Conversation

addisoncrump commented Oct 29, 2022

addisoncrump commented Oct 29, 2022

codecov bot commented Oct 29, 2022 • edited Loading

Codecov Report

addisoncrump Oct 29, 2022

Choose a reason for hiding this comment

jedel1043 Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

addisoncrump commented Nov 1, 2022

Razican left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 commented Nov 1, 2022

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 commented Nov 1, 2022

Razican left a comment

Choose a reason for hiding this comment

addisoncrump commented Nov 2, 2022

jedel1043 commented Nov 2, 2022

addisoncrump commented Nov 2, 2022

addisoncrump commented Nov 2, 2022

jedel1043 commented Nov 2, 2022 • edited Loading

jedel1043 commented Nov 2, 2022

bors bot commented Nov 2, 2022

codecov bot commented Oct 29, 2022 •

edited

Loading

jedel1043 Nov 1, 2022 •

edited

Loading

jedel1043 commented Nov 2, 2022 •

edited

Loading