1454744 - [BinAST] Create proper bindings for positional formal parameters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Augment BinSource parser generator to allow code in sum arms

This is modeled after (read: cargo culted) from the fields stuff. I tried commoning up the logic with some callbacks, but the helper got more complicated than the code duplication, so this is where I landed.

I admit that in theory this isn't a great place to put parsing code, since it's just the structural induction step, but it's pretty convenient here.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: add bindings for formal positional parameters

Comment 3

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

Comment on attachment 8968636 [details] [diff] [review]
Part 1: Augment BinSource parser generator to allow code in sum arms

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

lgtm

::: js/src/frontend/binsource/src/main.rs
@@ +73,4 @@
>      /// Custom per-field treatment. Used only for interfaces.
>      by_field: HashMap<FieldName, FieldRules>,
>  
> +    by_sum: HashMap<NodeName, SumRules>,

Nit: Could you document this?

@@ +775,4 @@
>              (rules_for_this_node.build_result.is_some(), "build:"),
>              (rules_for_this_node.append.is_some(), "append:"),
>              (rules_for_this_node.by_field.len() > 0, "fields:"),
> +            (rules_for_this_node.by_sum.len() > 0, "sum-arms:"),

Could you add this same verification to `fn generate_implement_interface`? Not urgent, that can wait for a followup bug.

Comment 4

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

Comment on attachment 8968637 [details] [diff] [review]
Part 2: add bindings for formal positional parameters

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

lgtm.

However, sums are footgunish as the deanonymizer needs to transform `Foo = A|Bar, Bar = C|D` into `Foo = A|C|D, Bar = C|D`, so D ends up appearing in several places. So, can you double-check in the generated that you have applied it in all the places you meant to apply it?

I have code in master to mitigate this problem, but I have not attempted to use it for binsource/src/main.rs. This might be needed for this use case.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42c4d4acfba5
Part 1: Allow BinSource generator to place code after parsing interface sum branches. (r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e7d188fd56
Part 2: Add bindings for formal parameter names. (r=Yoric)

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/42c4d4acfba5
https://hg.mozilla.org/mozilla-central/rev/02e7d188fd56