1473202 - [BinAST] Refactor code generation in order to generate streaming compiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

I'm about to use js/src/frontend/binsource/src/main.rs for generating streaming compiler code,
and it requires some refactoring and new feature.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Skip overwriting BinAST auto-generated file if the content is same.

there are 13 patches.

this patch is to avoid touching the file when there's no change,
in order to reduce the number of files to rebuild, after running js/src/frontend/binsource/build.sh

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Make the result type customizable.

In streaming compiler, the default return type is JS::Result<Ok>, instead of JS::Result<ParseNode*>.
so added the config to yaml.

also, removed the `default` parameter from get_type_ok/get_method_signature, given that it's known in most case.
the exception is enum case, which can be detected by checking if the name is enum name.
CPPExporter.enum_names holds the set of enum names.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Make default value for the optional field customizable.

now that the result type is customizable,
the default value is also customizable.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Make class name customizable.

also moved the class name to yaml file.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Make list append customizable.

Now the code doesn't expect ParseNode, list's append should be moved to yaml file.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 6.1: Add get_method_call function and use it when parsing field.

4 patches for refactoring method call,
in order to centralize method call code generation, and handle the "Ok" special case.

this patch adds get_method_call, which returns the code to call method.
MethodCallKind is to specify whether to use BINJS_MOZ_TRY_DECL or MOZ_TRY_VAR.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 6.2: Use get_method_call function when parsing list item.

Simply replaced list item method call.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 6.3: Use get_method_call function when parsing interface.

Added prefix/args parameters to get_method_call, similar to other get_method_* functions,
and applied to interface calls.

AlwaysDecl/AlwaysVar is added to suppress "Ok" special case,
to generate the same code as now.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 6.4: Use get_method_call function when parsing sum.

Applied to parseSum* code.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Move replace rule out of block.

currently "replace" for field is inside "block", but I think it's fine to move it outside, to reduce the indentation of the code in yaml file.
given that other "block" fields are not used.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Add extra parameter to methods.

(this is not used by current parser)

in streaming compiler, I need to pass extra parameters to methods, to specify the behavior, or receive 2+ data.
This adds definition for extra parameters for method declaration/definition and extra arguments for method call.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Add before/after for optional field, and replace for none case.

(also not used by current parser)

in streaming compiler, I need to add some code when parsing optional field,
both for "some" case and "none" case.

for now, "some" case supports "before"/"after".
"none" case supports "replace".

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Fix error message when inherit target is not found.

and fixed error message.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Add extra parameter to methods.

Fixed the parameter for field.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Wrap method declaration.

One more patch, to wrap method declaration in auto-generated header.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

I probably won't have time to review this until I'm back from PTO.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989626 [details] [diff] [review]
Part 1: Skip overwriting BinAST auto-generated file if the content is same.

Review of attachment 8989626 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +1403,5 @@
> +        if !Path::new(path).is_file() {
> +            return None;
> +        }
> +
> +        let mut f = File::open(path).expect("File not found");

Nit: `.expect` on the next line, here and below.

@@ +1418,5 @@
> +
> +        if let Some(old_data) = get_file_content(dest_path) {
> +            if old_data == *data {
> +                println!("skip");
> +                return;

Could you just add one quick comment to explain why we want to skip?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989627 [details] [diff] [review]
Part 2: Make the result type customizable.

Review of attachment 8989627 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +101,5 @@
>  /// Extracted from the yaml file.
>  #[derive(Default)]
>  struct GlobalRules {
> +    /// The return value of each method.
> +    parser_type_ok: String,

Since we clone it a lot, could you make it a `Rc<String>`?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989630 [details] [diff] [review]
Part 3: Make default value for the optional field customizable.

Review of attachment 8989630 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +107,5 @@
>      /// The return value of each method.
>      parser_type_ok: String,
>  
> +    /// Default value for the optional field.
> +    parser_default_value: String,

Here, too, Rc<String>.

@@ +342,5 @@
>          if let Some(ref parent) = rules.inherits {
>              let NodeRules {
>                  inherits: _,
>                  type_ok,
> +                default_value,

I'm confused. Isn't `default_value` a field of `GlobalRules`?

Can you double-check?

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989631 [details] [diff] [review]
Part 4: Make class name customizable.

Review of attachment 8989631 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +104,5 @@
>  /// Extracted from the yaml file.
>  #[derive(Default)]
>  struct GlobalRules {
> +    /// C++ class name of the parser class.
> +    parser_class_name: String,

As usual, Rc<String>. I'm going to stop repeating it :)

Comment 21

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to David Teller [:Yoric] (away until end of July - please use "needinfo") from comment #19)
> @@ +342,5 @@
> >          if let Some(ref parent) = rules.inherits {
> >              let NodeRules {
> >                  inherits: _,
> >                  type_ok,
> > +                default_value,
> 
> I'm confused. Isn't `default_value` a field of `GlobalRules`?
> 
> Can you double-check?

default-value is both in parser and node rule, since node rule can override type-ok, and there can be rule for Optional*.

Comment 22

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (away until end of July - please use "needinfo") from comment #18)
> Comment on attachment 8989627 [details] [diff] [review]
> Part 2: Make the result type customizable.
> 
> Review of attachment 8989627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/binsource/src/main.rs
> @@ +101,5 @@
> >  /// Extracted from the yaml file.
> >  #[derive(Default)]
> >  struct GlobalRules {
> > +    /// The return value of each method.
> > +    parser_type_ok: String,
> 
> Since we clone it a lot, could you make it a `Rc<String>`?

Does it mean that the return type of `get_type_ok` should also be changed to Rc<String> ?
(otherwise the copy will happen when leaving the function anyway?)
in that case, what should I do about other cases like NodeRules.type_ok ?

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 2.1: WIP: Use Rc<String>.

something like this?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989920 [details] [diff] [review]
Part 2.1: WIP: Use Rc<String>.

Review of attachment 8989920 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989633 [details] [diff] [review]
Part 6.1: Add get_method_call function and use it when parsing field.

Review of attachment 8989633 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +447,5 @@
>      // A set of enum class names.
>      enum_names: HashSet<NodeName>,
>  }
>  
> +enum MethodCallKind {

Could you document this? Without context, it's not clear that `Decl` is `BINJS_MOZ_TRY_DECL` and `Var` is `MOZ_TRY_VAR`.

@@ +585,5 @@
> +        let type_ok = self.get_type_ok(name);
> +        let call = format!("parse{name}()",
> +                           name = name.to_class_cases());
> +
> +        if type_ok == "Ok" {

Note for later: we could turn `type_ok` into an `enum`.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989635 [details] [diff] [review]
Part 6.3: Use get_method_call function when parsing interface.

Review of attachment 8989635 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +449,5 @@
>  }
>  
>  enum MethodCallKind {
>      Decl,
> +    AlwaysDecl,

(as in the previous patch, could you document this?)

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989639 [details] [diff] [review]
Part 9: Add before/after for optional field, and replace for none case.

Review of attachment 8989639 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +1183,5 @@
> +                        .unwrap_or("".to_string())
> +                        .reindent("        ")
> +                        .newline_if_not_empty(),
> +                    none_block = rules_for_this_node.none_replace
> +                        .unwrap_or(format!("result = {default_value};",

Could you rather use `unwrap_or_else` to avoid recomputing everything even when it's not needed?

Also, below.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989646 [details] [diff] [review]
Part 8: Add extra parameter to methods.

Review of attachment 8989646 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +592,5 @@
>          let type_ok = self.get_type_ok(name);
>          let kind = name.to_class_cases();
> +        let extra = match extra_params {
> +            Some(s) => {
> +                format!("{}\n{}", if args.len() > 0 { "," } else { "" }, s.reindent("        "))

Could you expand the (Rust) source to several lines?

@@ +613,5 @@
>          let type_ok = self.get_type_ok(name);
>          let kind = name.to_class_cases();
> +        let extra = match extra_params {
> +            Some(s) => {
> +                format!("{}\n{}", if args.len() > 0 { ", " } else { "" }, s.reindent("        "))

Same here.

@@ +644,5 @@
>              }
>          };
> +        let extra = match extra_params {
> +            Some(s) => {
> +                format!("{}\n{}", if args.len() > 0 { ", " } else { "" }, s.reindent("    "))

Also here.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989650 [details] [diff] [review]
Part 11: Wrap method declaration.

Review of attachment 8989650 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +820,5 @@
>              let rules_for_this_sum = self.rules.get(name);
>              let extra_params = rules_for_this_sum.extra_params;
>              let rendered = self.get_method_signature(name, "", "",
>                                                       &extra_params);
> +            buffer.push_str(&rendered.reindent("").newline_if_not_empty());

Nit: can you add the call to `newline_if_not_empty()` on the next line? Here and below.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8989637 [details] [diff] [review]
Part 7: Move replace rule out of block.

Review of attachment 8989637 [details] [diff] [review]:
-----------------------------------------------------------------

The idea is that `replace` was for replacing a block. I'm not entirely convinced of the necessity of this change.

Comment 31

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (away until end of July - please use "needinfo") from comment #30)
> Comment on attachment 8989637 [details] [diff] [review]
> Part 7: Move replace rule out of block.
> 
> Review of attachment 8989637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The idea is that `replace` was for replacing a block. I'm not entirely
> convinced of the necessity of this change.

currently the "block" behaves like "put a block statement with these properties",
since before/after fields outside the block also has effect, and decl is also outside the block.

>                         format!("{before_field}{decl_var}    {{
> {block_before_field}{parse_var}{block_after_field}
>     }}
> {after_field}",

then, "replace" replaces whole those things, including before/after outside the block,
so I feel it strange to put "replace" inside "block".
(also, moving "replace" outside the "block" reduces the indentation :)

Comment 32

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #31)
> (In reply to David Teller [:Yoric] (away until end of July - please use
> "needinfo") from comment #30)
> > Comment on attachment 8989637 [details] [diff] [review]
> > Part 7: Move replace rule out of block.
> > 
> > Review of attachment 8989637 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The idea is that `replace` was for replacing a block. I'm not entirely
> > convinced of the necessity of this change.
> 
> currently the "block" behaves like "put a block statement with these
> properties",
> since before/after fields outside the block also has effect, and decl is
> also outside the block.
> 
> >                         format!("{before_field}{decl_var}    {{
> > {block_before_field}{parse_var}{block_after_field}
> >     }}
> > {after_field}",
> 
> then, "replace" replaces whole those things, including before/after outside
> the block,
> so I feel it strange to put "replace" inside "block".
> (also, moving "replace" outside the "block" reduces the indentation :)

anyway, I'll land other patches for now.

Comment 33

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b03792874ae6649f45499652c37021abe1e98c1d
Bug 1473202 - Part 1: Skip overwriting BinAST auto-generated file if the content is same. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ae808f48b763ae225c7a2ff6cdf32e1f8f1534
Bug 1473202 - Part 2: Make the result type customizable. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/2afb27cb58bce8f97246a12204d10db83dfcc4fa
Bug 1473202 - Part 3: Make default value for the optional field customizable. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4d43b7c3a59639e67c76e6b89f11d70a869550
Bug 1473202 - Part 4: Make class name customizable. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/88c863e541ed1daaa606ac499271b7f36b6a441f
Bug 1473202 - Part 5: Make list append customizable. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/e212b6f200d369e0b9c53f5e929fba9aba7ac5a2
Bug 1473202 - Part 6.1: Add get_method_call function and use it when parsing field. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c6a0a2a63e9c1ed34c8d043b0a9819cc84781c
Bug 1473202 - Part 6.2: Use get_method_call function when parsing list item. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/e675fb44e70a8739af0ab1a1d7a8ec5af8961088
Bug 1473202 - Part 6.3: Use get_method_call function when parsing interface. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed588704ee3a29236da5e414c7a1d9c079d41119
Bug 1473202 - Part 6.4: Use get_method_call function when parsing sum. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0d0e6b9464756231bfbdf8791173847980e5b1
Bug 1473202 - Part 7: Add extra parameter to methods. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3e02c6c4c27b2c5f600de8f0c109f69c77e2eb
Bug 1473202 - Part 8: Add before/after for optional field, and replace for none case. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e403f08c079429d0d7b4b727f861361c25432aa
Bug 1473202 - Part 9: Fix error message when inherit target is not found. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3600583e49a97a7be6d8088dba37ebff1cd30fe
Bug 1473202 - Part 10: Wrap method declaration. r=Yoric

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b03792874ae6
https://hg.mozilla.org/mozilla-central/rev/13ae808f48b7
https://hg.mozilla.org/mozilla-central/rev/2afb27cb58bc
https://hg.mozilla.org/mozilla-central/rev/1b4d43b7c3a5
https://hg.mozilla.org/mozilla-central/rev/88c863e541ed
https://hg.mozilla.org/mozilla-central/rev/e212b6f200d3
https://hg.mozilla.org/mozilla-central/rev/33c6a0a2a63e
https://hg.mozilla.org/mozilla-central/rev/e675fb44e70a
https://hg.mozilla.org/mozilla-central/rev/ed588704ee3a
https://hg.mozilla.org/mozilla-central/rev/ef0d0e6b9464
https://hg.mozilla.org/mozilla-central/rev/ce3e02c6c4c2
https://hg.mozilla.org/mozilla-central/rev/6e403f08c079
https://hg.mozilla.org/mozilla-central/rev/e3600583e49a