Rescript-compiler: Make a trade off between module alias and linking behavior

[mlms13]

Minimal demo repo: https://github.com/mlms13/bsb-520-include

Per this comment in #3839, I've been trying to clean up impure modules to avoid the unnecessary explosion of imports in the compiled JS. I noticed that the following OCaml:

include SemiringExtras.Make(IntSemiring)

let v = increment 3

Produces the following JS:

var include = SemiringExtras.Make(IntSemiring);

var increment = include.increment;

var v = Curry._1(increment, 3);

...

/* include Not a pure module */

You can see more context in the sample repo, but in this case IntSemiring is pure and SemiringExtras is pure, so I'm not sure what's causing the include to make the module impure. At first I thought this was a regression with 5.2.0, but I've been able to reproduce it with 5.0.4 as well.

[anmonteiro]

SemiringExtras doesn't look pure.

let increment = S.add S.one

When applied S.one to produce the result, S.add could perform a side effect, causing it not to be pure.

[mlms13]

OK, how do I convince Bucklescript that it _is_ in fact pure? Because the net effect is that this "impurity" is adding dozens of unused imports to my bundled JS...

S.add could perform a side effect

I guess maybe I'm not understanding how purity is checked. By this reasoning, how is any function application known to be pure at compile time?

[anmonteiro]

let add first = 
  Js.log "Foo";
  fun second ->
    first + second + 1
;;

^ This is the concrete example of what I'm talking about -- the signature is still val add: int -> int -> int, but add 1 is not pure, nor is add 1 2.

I don't know what kinds of purity checks BuckleScript has, so I can't speak to that.

[mlms13]

I don't know much about the internals, but shouldn't that be caught where add is defined? It's true that SemiringExtras doesn't know if the module passed to it is pure, but it only does pure things with that module, so it seems strange to assume that the whole tree of modules is impure at that point.

[bobzhang]

Hi @mlms13 our purity analysis is primitive. Essentially function application (except few cases which is inlined) is classified as impure. Here functor application is exactly the same as function application underlying. Functor application is a runtime behavior not compile time thing.

There is another use case which can delay functor application, would you have a look at how Belt.Map is implemented

In the short term, there is no plan to refine the purity analysis, so I am going to close this issue

[mlms13]

I understand the reasoning for closing both this and #3839, but in practice, the combination of the two issues means that every project that uses Relude (or any project that depends on relude, such as bs-decode) ends up importing every single module in a rather large standard library in the bundled JS.

It would be nice if there was something Bucklescript could do to help us out here, since this wasn't an issue prior to 5.2.0.

[andywhite37]

@bobzhang - can we have a little more discussion about this? If modules are being more aggressively assumped to be impure, and this results in more things being auto-imported into the .js files, it's going to wreak havoc with download sizes. What changed between 5.0.4 and 5.2.0 (or whenever this changed) that caused this behavior to start?

[mlms13]

In this case, I think the main change in 5.2.0 was #3839. The "impure" modules already existed (I checked as far back as 5.0.4), but their impurity didn't really matter.

I tested bundling es6 modules using Rollup (which is supposed to be good at tree-shaking as far as I know), and the one-line example repo (Js.log (Relude.List.toArray [])) has an extra 50kb with 5.2.0 compared with 5.0.6.

[bobzhang]

@andywhite37 I believe it is that we removed -no-alias-deps by default, which caused a subtle build system bug.
I think it is okay to have a file with purely module alias to use the old behavior

(* all are module aliases *)
module A = X_A
module B = X_B 

But I am unsure it is sufficient to restore the old bundle size

[bobzhang]

I can add a dirty patch to see if it is what you want

[mlms13]

I'm not 100% sure, but I'd guess that special-casing modules that only have module aliases in them would fix the increased bundle size we're seeing with Relude.

[andywhite37]

I'm just now learning about -no-alias-deps, and I'm not qualified to really ask questions about this or understand the answers or reasons, but why was -no-alias-deps removed by default? It seems desirable to have -no-alias-deps turned on by default, to avoid these problems. Are you saying that you will restore -no-alias-deps to being set by default? Does -no-alias-deps only affect modules that only contain module aliases? Or are you saying -no-alias-deps will stay removed, and another solution is being proposed?

[TheSpyder]

I was too late to the discord discussion to mention this - modules as objects shouldn't be causing the addition of extra dependencies, because even compiled to arrays they wouldn't be tree shaken out. I think -no-alias-deps is the cause of this.

I believe-no-alias-deps was removed because of #3796, with it turned on the aliased module isn't listed as a dependency; this clearly aids in tree shaking but breaks incremental builds.

@bobzhang perhaps it's finally time for a "dev mode" for bsb? By default -no-alias-deps is enabled but "dev mode" would disable it. This would also be a good place to toggle -bs-g as in #3716.

[andywhite37]

Having a dev mode and a prod mode makes sense to me if that's a good way to address this. Big bundles in dev shouldn't be a problem for most people if you have the option to do a more optimized prod build.

[TheSpyder]

During the discussion in https://github.com/reazen/relude/issues/168 it highlighted to me that perhaps the stale build issue was more recent than I thought - in which case changing -no-alias-deps to fix it is a wider fix than was necessary 🤔

Keeping it this way and in a dev mode is my preferred option, but re-evaluating the stale fix might also work

[bobzhang]

Hi, this is a thing we would like to fix (maybe by allowing user to customize the behavior for some special module).
But this is not a regression, _it is a bug fix and the current semantics is correct_ - when you alias a module to a potential effectful module, it will be linked in. For our own built-in namespace support, it behaves correctly.

@TheSpyder this issue is not related to dev mode or prod mode, since in both modes, the semantics should be the same

[bobzhang]

My current proposal is that for a module with all module aliases, we still record the deps in the build system, but consider this module itself pure

[TheSpyder]

OK, so the behaviour before was more of an accident. Got it.

Technically the module can't be proven pure, even if it's just aliases, but to be honest this is the same shortcut used by my company with rollup. We have entry point modules that just import other modules, and this chunk of code inlines those to aid in tree shaking.

I'm 100% in favour of considering all-alias modules pure 👍

[andywhite37]

@bobzhang - that seems reasonable if you think it can be done.

Since module purity is kind of at the root of this problem - what is it that makes the compiler think that Belt.Map is pure? I see that you are not using module functors, but how does it know that all of your function applications are pure?

Is there a list of things you must conform to in order to pass the purity check?

E.g.

For your module to be pure:

1. You must not use any module functors (is this true?)
2. You must only apply pure functions (how is this determined?)
3. etc?

If there is any material or code on this, I'd be happy to do some of my own reading, but I'm not sure where to start looking - I'm pretty new to the whole ocaml ecosystem.

[bobzhang]

@andywhite37 Belt.Map used a pattern to delay the functor application to the user side

[bobzhang]

@mlms13 can you confirm it is fixed in master? thanks

[mlms13]

I'll take a look in just a bit. Thanks for your help on this one!

[andywhite37]

I installed 5.2.1, and our Relude.re module, which contains only module aliases, now says /* No side effect */ at the bottom of the Relude.bs.js file, so I think that means the fix is working, right? A module containing only module aliases is now considered pure.