Remove preprinter, add parenthesizer callback to emit #43652
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
rbuckton
force-pushed
the
substitutionParenthesizer
branch
from
d24112d to
8d60631
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 8d60631. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..43652
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
requested review from
andrewbranch,
elibarzilay,
jessetrinity,
orta,
weswigham and
ahejlsberg
|
@rbuckton can you point out what code is new, as opposed to a reversion? |
|
In emitter.ts, most of the changes are a reversion. I left in some of the changes in comment/sourcemap emit as it improves the number of cases where we can use our binary expression trampoline to avoid exhausting the call stack. The only real addition is the ability to pass a Anything in nodeFactory.ts, parenthesizerRules.ts, and types.ts is new and was added to support passing a |
|
This one wasn't as simple as just rolling back the change, as the work to create a reusable binary expression trampoline is something we want to keep so that we're consistent across the binder, checker, and emitter. As a result, we also kept the changes to comment and source map emit so that we could reuse the logic inside the binary expression trampoline. |
| getParenthesizeLeftSideOfBinaryForOperator(binaryOperator: SyntaxKind): (leftSide: Expression) => Expression; | ||
| getParenthesizeRightSideOfBinaryForOperator(binaryOperator: SyntaxKind): (rightSide: Expression) => Expression; |
There was a problem hiding this comment.
These are new, and exist to support passing in a (node: Node) => Node parenthesizer rule for binary expressions. The implementation simply wraps parenthesizeLeftSideOfBinary or parenthesizeRightSideOfBinary and caches the result for reuse.
| @@ -562,31 +562,31 @@ namespace ts { | |||
| } | |||
|
|
|||
| export const nullTransformationContext: TransformationContext = { | |||
| get factory() { return factory; }, | |||
There was a problem hiding this comment.
This is simply a reordering of existing properties to match the order in transformNodes so they (hopefully) share the same "map" in V8.
| @@ -161,4 +161,6 @@ var _brokenTree = (0, renderer_1.dom)(component_1.MySFC, { x: 1, y: 2 }, | |||
| (0, renderer_1.dom)(component_1.MyClass, { x: 3, y: 4 }), | |||
| (0, renderer_1.dom)(component_1.MyClass, { x: 5, y: 6 })); | |||
| // Should fail, nondom isn't allowed as children of dom | |||
| var _brokenTree2 = (0, renderer_1.dom)(DOMSFC, { x: 1, y: 2 }, component_1.tree, component_1.tree); | |||
| var _brokenTree2 = (0, renderer_1.dom)(DOMSFC, { x: 1, y: 2 }, | |||
There was a problem hiding this comment.
This indentation matches the emit in release-4.2.
| if (currentParenthesizerRule) { | ||
| lastSubstitution = currentParenthesizerRule(lastSubstitution); |
There was a problem hiding this comment.
This (and the code path that sets currentParenthesizerRule) is the only "actual" change from what we previously were doing in 4.2. The point here is to only invoke the parenthesizer rule when substitution results in a different node (which might have a different precedence).
|
While not perfect (as there are some other changes in emitter.ts unrelated to this), you can get a gist of what changed and was reverted by doing the following on the commandline: git difftool d3b8d5e9c423d2565e9c5122326fbf4a2a2e5d6a^..substitutionParenthesizer -- src/compiler/emitter.ts |
rbuckton
force-pushed
the
substitutionParenthesizer
branch
from
8d60631 to
b48a51a
Compare
|
I've pushed up a change that reorders some of the functions so that they line up with where they were prior to #43676 to make the command-line diff easier to read through. |
|
I don't think this should have broken anything, but just to be on the safe side... @typescript-bot run dt |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
RWC tests failed, but its unclear why. I'm running them locally. |
|
RWC failures are mostly unrelated to this change, the few that are are changes in indentation that were likely present before the preprinter transform was added (similar to what I mentioned here), so this is just going back to existing behavior. |
|
The |
|
@sandersn did a bit ago, iirc? |
|
I cannot repro the OOM locally. @typescript-bot user test this |
|
Ok. It didn't crash that time. Must have been a glitch in the matrix. |
|
The mithril/cytoscape crash already has a PR up with a fix~ |
|
The other crash has the same origin; the electron-* are known failures that shouldn't show up in the final output. They should, I hope, be gone after the additional |
This reverts the "preprinter" transform from #42676 that was primarily used to ensure correct parenthesization using the node factory and amends the emitter to support providing a parenthesizer rule callback when emitting certain nodes.
Related #43486