-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Track inclusion of entities returned by literal methods #2263
Conversation
This also provides another noticeable performance boost (e.g. bundling the TypeScript compiler is now >20% faster) |
Also note the negative diff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great simplification to see.
import { ExpressionNode, NodeBase } from './shared/Node'; | ||
import SpreadElement from './SpreadElement'; | ||
|
||
export default class ArrayExpression extends NodeBase { | ||
type: NodeType.tArrayExpression; | ||
elements: (ExpressionNode | SpreadElement | null)[]; | ||
|
||
getReturnExpressionWhenCalledAtPath(path: ObjectPath) { | ||
if (path.length !== 1) return UNKNOWN_EXPRESSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we return something like THROW_EXPRESSION
here if path.length === 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super obscure edge case I know...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure we could. What is your intention here:
- documentation, or
- improved tree-shaking in certain situations?
I am a little hesitant to do this just in a few places as there are quite a lot of situations where we deoptimize because we would do something illegal and having it done in some places only could leave people falsely relying on this.
If you see some potential tree-shaking benefit, it might be worth doing it in all relevant places in a separate PR.
src/ast/nodes/CallExpression.ts
Outdated
return ( | ||
this.callee.hasEffects(options) || | ||
this.callee.hasEffectsWhenCalledAtPath( | ||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not EMPTY_PATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed this in some other places as well.
properties.length === 0 | ||
) | ||
return getMemberReturnExpressionWhenCalled(objectMembers, key); | ||
if (!hasCertainHit || properties.length > 1) return UNKNOWN_EXPRESSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look forward to PossibleExpressionSet
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this could well be coming with SSA. PossibleExpressionSet
is a good name for Phi-functions 😉
): LiteralValueOrUnknown { | ||
): ExpressionEntity { | ||
if (this.kind === 'set') { | ||
return UNKNOWN_EXPRESSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again could be nice to have THROWS_EXPRESSION
or similar.
d080293
to
e3f8bb3
Compare
5d0ff17
to
58678fd
Compare
This Pull Request updates dependency [rollup](https://github.com/rollup/rollup) from `v0.60.4` to `v0.60.6` <details> <summary>Release Notes</summary> ### [`v0.60.6`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0606) [Compare Source](rollup/rollup@v0.60.5...v0.60.6) *2018-06-14* * Track mutations of included virtual arrays ([#​2263](`rollup/rollup#2263)) * Update readme ([#​2266](`rollup/rollup#2266)) --- ### [`v0.60.5`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0605) [Compare Source](rollup/rollup@v0.60.4...v0.60.5) *2018-06-14* * Track deep reassignments of global and exported variables and improve performance ([#​2254](`rollup/rollup#2254)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
This resolves #2253 by internally using instances that can track their own inclusion status for return values of literal methods. E.g. in this example:
the line
virtual.push(3)
will be included asvirtual
itself is included due to theconsole.log
statement. The third line, on the other hand, is still removed as it is no side-effect.To be able to do that, a larger refactoring was necessary. The main issue was that in order to track the inclusion status of an entity that is not represented by an AST node, another AST node needed to take up that responsibility. In this case, the respective
CallExpression
seemed the natural choice (each call toArray.prototype.slice
creates an independent new object).Thus I needed to change the fundamental logic that tracks side-effects around return expressions. Instead of e.g. the call expression "asking" a function declaration if calling its return value has a side-effect, the call expression initially asks the function declaration for its return expression, stores this value and then directly performs queries on this return expression.
The only (tested) situation where this performs less optimal than before is when we call an expression that can resolve to more than one possible callable entity. In this case, we just treat the return value as "unknown".