"error.stack;" expression removed by tree-shaker, breaks bindings

[mildsunrise]

• Rollup Version: 2.39.1
• Operating System (or Browser): Node
• Node Version (if applicable):
• Link to reproduction

Expected Behavior

The dummy.stack; line is not removed.

Actual Behavior

The dummy.stack; line is removed.

Accessing stack is necessary for the prepareStackTrace function to be called. Removing it breaks the bindings package. (I'm not sure that the tree-shaker can be smart enough to detect the side-effect here, but the documentation asked to file an issue, and I didn't find an existing one, so I did)

[kzc]

Odd API. Who knew?

• https://v8.dev/docs/stack-trace-api#customizing-stack-traces
• https://nodejs.org/api/errors.html#errors_error_capturestacktrace_targetobject_constructoropt

I guess all stack property reads have to be registered as having side effects.

[kzc]

This is notable:

https://v8.dev/docs/stack-trace-api#compatibility

Compatibility

The API described here is specific to V8 and is not supported by any other JavaScript implementations. Most implementations do provide an error.stack property but the format of the stack trace is likely to be different from the format described here.

[mildsunrise]

yes, everything regarding stack traces is probably implementation dependent... if you think this is out of scope feel free to close, I just wanted to let you know

[mildsunrise]

for context: even if implementation dependent, this API seems to be important in the ecosystem; so much that we explicitly preserved its behavior when source maps are present (nodejs/node#31143)

[kzc]

It's not my call, but I'm not against supporting this if it's widely used - even though property reads with side effects are an anti-pattern. It would be relatively easy to add a side effect for all stack property reads. It would be more difficult to isolate error related stack property reads in all situations.

[kzc]

This patch retains all stack property accesses:

--- a/src/ast/nodes/MemberExpression.ts
+++ b/src/ast/nodes/MemberExpression.ts
@@ -71,4 +71,15 @@ function getStringFromPath(path: PathWithPositions): string {
 }
 
+// properties with read side effects - just one entry for now
+const propertyEffects = {
+       __proto__: null,
+       stack: true
+};
+
+function propertyReadHasEffects(property: any) {
+       return (property.type == 'Literal' && property.value in propertyEffects) ||
+               (property.type == 'Identifier' && property.name in propertyEffects);
+}
+
 export default class MemberExpression extends NodeBase implements DeoptimizableEntity {
        computed!: boolean;
@@ -174,4 +185,5 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
        hasEffects(context: HasEffectsContext): boolean {
                return (
+                       propertyReadHasEffects(this.property) ||
                        this.property.hasEffects(context) ||
                        this.object.hasEffects(context) ||

With patch:

$ echo 'var o={}; o.foo; o["bar"]; o.stack;' | rollup --silent
var o={}; o.stack;

$ echo 'var o={}; o.foo; o["bar"]; o["stack"];' | rollup --silent
var o={}; o["stack"];

[kzc]

--treeshake.propertyReadSideEffects=always can now be used as a workaround for tree shaking with getters with side effects.

[mildsunrise]

sorry about not replying, and thank you for patching this 😊