Add unsafe optimization to inline undefined for missing objlit props

[shicks]

Fixes #584

This is safe unless the code is relying on non-standard properties being defined on Object.prototype, which is why the optimization is protected behind the "unsafe" option.

Enabling this allows the following general pattern:

const ENABLE_ASSERTS = process.env.ENABLE_ASSERTS ?? process.env.NODE_ENV === 'development';

If compiled with --define process.env={} as the last define, then it allows individually toggling (in this case) asserts, but if nothing is specified then it will smoothly fall back on NODE_ENV. In addition to debugging features this can be useful for differential compilation (e.g. a "master" switch for "assume browsers released after 20XX" that provides reasonable defaults for avoiding feature detection in most cases, but also providing fine-grained overrides for individual features that may be more important to actually detect for one application over another.

[fabiosantoscode]

This is a pretty good patch! However I believe it might not be the best solution for your specific use case.

Maybe instead we can improve the existing --define so that we can specify that the properties in objects it... defines, are the only properties in that object.

I need to give more thought to this, but interesting ways to do this would be to use a label: --define "complete: foo={ my: 'props', are: 'final' }" or Object.seal: --define "foo=Object.seal({ my: 'props', are: 'final' })". Both are pretty obscure but they scratch my inner JS nerd. And the Object.seal has the advantage of enabling you to specify subsets of the object whose properties are final, and leave the rest of the object alone.

[shicks]

Using Object.seal is clever, but it actually doesn't address the underlying unsafety - not only because it assumed Object.seal is unpatched, but also because sealing doesn't actually protect against prototype mutation. Another potential concern is scalability/composability - requiring all properties be defined in a single argument makes it harder to combine definitions from several different sources (that said, my approach introduces an unfortunate order dependence, so I'm definitely not sold on that, either).

Please give it a think.

[fabiosantoscode]

The Object.seal is just for marking up the option, it doesn't literally mean Object.seal in the final output. If you were to, say, do console.log(foo) and foo is undefined, the output would read console.log({ my: 'props', are: 'final' }) without Object.seal.

Another interesting markup would be:

--define "process.env={NODE_ENV: 'production', ...nothingElse}" where nothingElse is a special variable that means nothing else is defined when added as an object spread. Or a comment could be used as well: --define "process.env=/*#__ONLY__*/{NODE_ENV: 'production'}"

edit: the advantage of the comment is that it can easily be used in regular source code.

[shicks]

I'm wondering if you've had a chance to think a bit more on this. Of the options I've seen so far, I'm still partial to mine - it seems the simplest and least surprising/contrived, and is reasonably generalizable outside of the direct defines case as well (i.e. transitive defines, or some other inlining/defaulting that results in {foo: 42}.bar would still get the benefit).

[fabiosantoscode]

In the grand scheme of things, I think it's unrealistic that someone would --define process.env={}, alter the Object prototype, and expect that process.env.propInPrototype would be there. Even if it wasn't process.env.

So I think the correct course of action, is to create a new flag for the AST_Node#flag bitfield, which flags the objects created by --define, allowing them to be understood as "complete". This makes it so people don't have to opt into unsafe or some obscure unsafe_* option.

I think this will greatly improve how people use --define especially for process.env.

[shicks]

But does --define really "create" objects, per se? I understand it to be more of a slightly-more-syntactically-aware text substitution? I don't have a good sense of the variety of real-world usage, but it wouldn't surprise me if folks use it to poke in properties of objects that they really do want to stick around at runtime, and it's not clear how to distinguish between the two cases.

Or are you referring to the explicit --define process.env={} as what would trigger this? What do you think about the case of indirect inlining enabling further optimization? Here's a slightly more concrete (though admittedly still contrived) example:

/*@__INLINE__*/
function log(obj) {
  if (obj.foo) console.log(`Had a foo: ${obj.foo}`);
  if (obj.bar) console.log(`Had a bar: ${obj.bar}`);
  // ...
}
log({foo: 42});

There's no --defines anywhere here, but I would still expect this to optimize to just

console.log(`Had a foo: ${42}`);

But without the more general treatment, the extra ifs can't be stripped.

[fabiosantoscode]

@shicks I see what you mean!

In that case let me suggest changes in the PR. For starters, we shouldn't use unsafe. It can break code, so users are wary of it. However, unsafe_proto already exists, and its use case is similar, so we can use it again here.

[fabiosantoscode]

Some changes that need to happen, so this transformation doesn't remove any side effects which might be left behind in the object.

Also, please change the option name to be unsafe_proto :)

[fabiosantoscode]

This will get rid of side effects in other properties of the object. Picture this (too contrived) use case:

const x = {
  iHaveSideEffects: sideEffects(),
  foo
}.bar

If we turned this object into undefined outright here, the call with side effects disappears. There's a method which can fix this, AST_Node#drop_side_effect_free which when called on the object X is being initialised to, would return simply sideEffects(). Be careful because when nothing has side effects this method just returns null.

But! We want the result of the expression to really be undefined.

So we can call make_sequence with an array of two which results in (sideEffects(), undefined). The sequence expression is terrible in terms of readability, but it's used extensively in Terser for being an expression instead of a statement.

There is, however, some calling code of flatten_object which expects the return to be either null or a AST_Sub with a number. I'll leave that one up to you but maybe returning a pair would work ;)

[fabiosantoscode]

I don't think there's a need for this condition here. If any of the properties is an accessor, it can still be doomed to oblivion :)