flow-remove-types implemented in Rust

[mischnic]

Summary

flow-remove-types uses the flow parser generated from OCaml (so in that sense, similar to hermes-parser) to strip flow types (with a visitor and codegen in JS). I tried to use hermes to achieve the same thing but without the additional roundtrips to JS.

A benchmark (react-dom.development.js) of stripping a 898kb Javascript file:

• flow-remove-types: 1.56s
• With this (release build): 53ms (Not a totally fair comparison because this doesn't involve JS at all, where the final usecase would still be having an npm package that passes a string to a WASM module, executes the above operation, returns the string and sourcemap - these two steps are missing from this benchmark)

This PR is definitely not ready as a drop-in replacement for flow-remove-types, but I think this might be useful:

• ~I'm not sure if the unsupported Juno project is here to stay? This might be an interesting usecase for the already existing parser/ast/codegen Rust infrastructure because I imagine that the actual logic to optimize JS will take quite some time to be developed.~
• ~I haven't been able yet to create a WASM build because I haven't figured out how to create a WASM build of Rust with linked C++.~
• ~Sourcemaps should be emitted alongside the stripped Javascript code~
• It currently behaves like flow-remove-types --ignore-uninitialized-fields.
• ~The only incompatibility I found between Hermes' parser and flow-remove-types is "this params" which result in a parser error:~

  declare function y(this: string): void

• ~The newly-announced~ Flow Enums are ~not supported (probably also neither in the parser)~ not transformed yet

[tmikov]

This is quite an unexpected PR, thank you for submitting it! For now Juno is highly experimental and completely unsupported and we literally cannot offer any guarantees or promises for future support, maintenance or even existence (although we can certainly hope). It is also very early with much of the design (like the AST itself) still in a flux, so we don't want to add a lot of code that we might need to refactor.

With that said, this looks like a simple transformation in line with things that we want Juno to be able to do. We will review the changes and discuss whether it makes sense to keep it as an evolving test/example of the AST and transformation APIs (including the mutable visitor).

Half of the team is on vacation currently, so we will comment here some time next week.

Again, I want to emphasize that we are very pleasantly surprised by this contribution and grateful for it.

[facebook-github-bot]

@tmikov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[tmikov]

Update: we would like to integrate this transform and keep it as a running example and test of the internal Juno API.

Right now we are performing a pretty big refactoring of the AST (replacing node pointers with integer indexes, things of that nature) and creating a separate crate for transforms like this, so it doesn't really make sense to merge it right away.

But once these changes are committed, which should be by the end of the week, and if you don't mind rebasing and refactoring on top of them, we can merge the PR.

[tmikov]

@mischnic FYI, we just discovered a major design flaw in the committed AST representation, we are working on fixing it.

[tmikov]

@mischnic we are finally done, after more than a month :-) The AST is in what we expect to be a more or less stable form. The major change is that AST nodes are now immutable and garbage collected. We have also added source map generation and the basics of semantic resolution. We addressed the problem of locating Hermes, so it should work in IDEs (it works for me in CLion).

There are some example transformations under https://github.com/facebook/hermes/tree/main/unsupported/juno/crates/pass/src/passes.

If you like to re-implement the transformation on top of the new infrastructure, we would be happy to review it and accept it.

[facebook-github-bot]

@mischnic has updated the pull request. You must reimport the pull request before landing.

[mischnic]

I've rebased it but ran into two problems:

The code generator is currently broken for type casts (so this only occurs without stripping the types):

async function test(){
  return await (x: any);
}

becomes this (unbalanced parenthesis around the return value):

async function test(){
  return await ((x: (any));
}

I tried to write tests, but the parser fails when parsing a string containing only whitespaces:

thread 'run_test' panicked at 'called `Option::unwrap()` on a `None` value', crates/juno/src/hparser/convert.rs:134:80
stack backtrace:
   0: rust_begin_unwind
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:101:14
   2: core::panicking::panic
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:50:5
   3: core::option::Option<T>::unwrap
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/option.rs:735:21
   4: juno::hparser::convert::Converter::cvt_smloc
             at hermes/unsupported/juno/crates/juno/src/hparser/convert.rs:134:30
   5: juno::hparser::generated_cvt::cvt_node_ptr
             at hermes/unsupported/juno/crates/juno/src/hparser/generated_cvt.rs:22:16
   6: juno::hparser::convert_ast
             at hermes/unsupported/juno/crates/juno/src/hparser/mod.rs:82:14
   7: juno::hparser::ParsedJS::to_ast
             at hermes/unsupported/juno/crates/juno/src/hparser/mod.rs:72:32
   8: strip_flow::parse
             at ./tests/strip_flow.rs:59:32
   9: strip_flow::assert_strip
             at ./tests/strip_flow.rs:80:44
  10: strip_flow::run_test
             at ./tests/strip_flow.rs:27:5
  11: strip_flow::run_test::{{closure}}
             at ./tests/strip_flow.rs:19:1
  12: core::ops::function::FnOnce::call_once
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5

which would be needed for

    assert_strip(
        r#"import type { SomeOtherType } from 'some-module';"#,
        "",
    );

You can reproduce that by running the test in crates/pass/tests/strip_flow.rs

[tmikov]

The empty input file assert is a known bug, sorry about that (it is actually quite interesting) - can you work around it for now by adding an extra ";"?

We will look into the parenthesis bug.

[tmikov]

We are also looking into the two other problems reported, but we can't quite figure out what you meant:

• declare function y(this: string): void parses fine.
• enum Status { Active, Paused, Off } also parses fine.

[facebook-github-bot]

@tmikov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@mischnic has updated the pull request. You must reimport the pull request before landing.

[mischnic]

Yes, that function seems to indeed work now, not sure why it didn't when I wrote that. But this still fails

type T = (this : string) : void

with

thread 'strip_flow::this_params' panicked at
'Some((SourceLoc { line: 3, col: 34 }, "'=>' expected in function type annotation"))',
crates/pass/tests/strip_flow.rs:358:5

---

You're right, enums parse correctly. I'll add support to transform them like the babel transformer does: https://github.com/facebook/flow/blob/main/packages/babel-plugin-transform-flow-enums/__tests__/babel-plugin-transform-flow-enums-test.js

---

It currently behaves like flow-remove-types --ignore-uninitialized-fields (which is also what @babel/preset-flow does by default):

     --ignore-uninitialized-fields
                    Removes uninitialized class fields (`foo;`, `foo: string;`)
                    completely rather than only removing the type. THIS IS NOT
                    SPEC COMPLIANT! Instead, use `declare foo: string;` for
                    type-only fields.

The reason I did this is that I got a broken React Native bundle without that flag because this section here only works correctly if the lines are removed completely (the default behaviour is to keep the LHS: onclose; ...).

https://github.com/facebook/react-native/blob/b7b59aee84de9b1ae9eaa4a419efbd62760321cf/Libraries/WebSocket/WebSocket.js#L82-L85

🤷

---

I've finished the tests now.

[avp]

@mischnic I tried

type T = (this : string) : void

in Flow 0.163.0 (most recent version) and it doesn't parse ("Unexpected token :, expected the token =>"). I don't think it has anything to do with the this constraint, because

type T = (foo : string) : void

fails with the same error. I think function types in Flow must use the => token.

[mischnic]

True, but strange given that I copied that line from Flow in the first place:

https://github.com/facebook/flow/blob/1a80a257b569456bb6e494fe508779fc91971484/packages/flow-remove-types/test/source.js#L224

[facebook-github-bot]

@tmikov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[avp]

@mischnic The flow parser is recovering from the error and makes a TypeAlias which flow-remove-types strips entirely, so the error goes away. It's possible that flow-remove-types should simply not run if there's parse errors, but it doesn't currently check that. The Hermes parser doesn't have the level of error recovery logic that the Flow parser does, so we just error and that's it.

[facebook-github-bot]

@mischnic has updated the pull request. You must reimport the pull request before landing.

[mischnic]

I see, thanks!

I've now implemented transforming enums and also fixed up the this-params tests and implementation. So this should be ready

[facebook-github-bot]

@tmikov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@tmikov merged this pull request in facebook/hermes@9f9aea0a68cd99bedbfdd3d98d4e60a433f4beb7.