AST and IIFE refinements

[rhendric]

Description of the change

Hello the working group!

After reviewing the ES modules PR I had two nagging concerns that I wanted to work on; this PR addresses them.

I wasn't crazy about how the number of constructors in the CoreImp AST was growing. Adding complexity there makes writing CoreImp optimizations more difficult. Also, the new constructors were just sort of an awkward fit in my opinion; import and export statements can't mix with the rest of a module's AST, and the Pure constructor was just a comment with some special printing behavior.

To address this, the first commit in this PR merges the Comment and Pure constructors (and also removes a never-used SourceSpan parameter from both). The second commit replaces the [AST] type in the return value of moduleToJs with a new CoreImp.Module type that holds separate fields for header comments, imports, the module body, and exports, allowing the Import and Export constructors to be removed from the main CoreImp AST type.

My other concern was the fairly liberal use of IIFEs at the top level to enable dead code elimination in external bundlers, particularly for object and array literals. Especially given that class instances take the form of top-level object literals, I wanted to protect them from any clutter that isn't absolutely necessary. The third commit here is a reimplementation of the pure-annotating function that tries a little harder not to create IIFEs. I ran the bundler benchmark from purescript-halogen-template on this new PR, and the revised implementation shaves another 2 KB off of the minified output of each bundler, representing more than half of the IIFEs introduced by the original implementation. (Note that in a few cases, this new implementation leads to more pure-annotation comments than the old one; for example:

var isJust = /* #__PURE__ */ maybe(false)(/* #__PURE__ */ Data_Function["const"](true));

But those disappear during minification, unlike IIFEs, or can be removed with simple global search-and-replace tools, also unlike IIFEs. Also, yes, one of the improvements here is that an IIFE isn't necessary for Data_Function["const"], because Data_Function will be an ES module object, and the bundlers tested don't expect those to have effectful getters.)

Full results from bundle.sh

(generous-iifes is the original PR; stingy-iifes is this PR.)

$ tree -s bundles
bundles
├── [         66]  esbuild
│   ├── [        112]  generous-iifes
│   │   ├── [     203649]  index.js
│   │   ├── [      30486]  index.js.gz
│   │   ├── [      84539]  index.minified.js
│   │   └── [      18677]  index.minified.js.gz
│   ├── [        112]  stingy-iifes
│   │   ├── [     197813]  index.js
│   │   ├── [      30318]  index.js.gz
│   │   ├── [      82242]  index.minified.js
│   │   └── [      18494]  index.minified.js.gz
│   └── [        112]  v0.14.5
│       ├── [     259386]  index.js
│       ├── [      35835]  index.js.gz
│       ├── [     112911]  index.minified.js
│       └── [      24801]  index.minified.js.gz
├── [         52]  parcel
│   ├── [        168]  generous-iifes
│   │   ├── [    2116771]  index.13db5cae.js
│   │   ├── [     232545]  index.13db5cae.js.gz
│   │   ├── [      80107]  index.8524bd67.js
│   │   ├── [      16962]  index.8524bd67.js.gz
│   │   └── [        166]  index.html
│   └── [        168]  stingy-iifes
│       ├── [    2010525]  index.5a8573a1.js
│       ├── [     230689]  index.5a8573a1.js.gz
│       ├── [      77706]  index.cfeb9ae0.js
│       ├── [      16703]  index.cfeb9ae0.js.gz
│       └── [        166]  index.html
├── [         66]  purs
│   ├── [        172]  index.cjs.html
│   ├── [     280474]  index.js
│   └── [      39881]  index.js.gz
└── [         52]  webpack
    ├── [        112]  generous-iifes
    │   ├── [    2997585]  index.js
    │   ├── [     257231]  index.js.gz
    │   ├── [      78385]  index.minified.js
    │   └── [      16675]  index.minified.js.gz
    └── [        112]  stingy-iifes
        ├── [    2879227]  index.js
        ├── [     255871]  index.js.gz
        ├── [      75664]  index.minified.js
        └── [      16394]  index.minified.js.gz

---

Checklist:

• Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
• Added myself to CONTRIBUTORS.md (if this is my first contribution)
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[JordanMartinez]

also removes a never-used SourceSpan parameter from both

Should that be done? Or asked differently, would it ever be used in the future?

[JordanMartinez]

Just FYI. We discussed this in today's working group call and everyone approved it.

[rhendric]

also removes a never-used SourceSpan parameter from both

Should that be done? Or asked differently, would it ever be used in the future?

It's academic now, but my response is twofold:

• Nothing of value is being destroyed here. In the event this is desired in the future, what to do is self-evident, and the effort to do it is minimal. And in exchange, we are being clearer about what does and doesn't carry source information in the present.
• I think it's somewhat unlikely this will ever be desired, because the source information is just there to make source maps work, and source maps are there just to make debugging tools work, and comments aren't executable code.

[JordanMartinez]

Thanks for clarifying!