fix receiver on calls of imported and exported functions #35877
Conversation
|
I see a lot of fixture changes but none of the test cases from #35420 added - specifically, a |
|
@ljharb the vast majority of the tests only verifies the correct transpilation result by comparing with the baselines. You can see from the changed baselines that imported functions are now called indirectly, which fixes the issue with the receiver. |
|
It's certainly not up to me, but the important thing is the correct runtime semantics of the code, not the specifics of what gets output, so it seems like those tests would be the important thing :-) |
|
@rbuckton This is now scheduled for 4.0. Can you review this fix during the beta? |
| if (!isIdentifier(node.expression)) { | ||
| return node; | ||
| } | ||
| const newExpression = substituteExpressionIdentifier(node.expression); |
There was a problem hiding this comment.
The substitution for the identifier should be invoked by the printer, you shouldn't have to call it directly.
|
Ping again - i know this needs a rebase, but it’s a pretty critical bug fix. |
rbuckton
force-pushed
the
import-implied-receiver
branch
from
ec2ef0b
to
0bf051b
Compare
|
I've rebased this PR and made a few small updates. We may also want to consider doing the same thing for namespace exports: namespace X {
export function foo(this: unknown) {
console.log(this === X);
}
}
namespace X {
foo(); // compiles to `X.foo()`, so would print `true`
}@DanielRosenwasser we should discuss this emit change at the next design meeting. |
|
I've updated this to support namespace exports as well. as its essentially a bug there too and I'd like to see how this change effects compiler performance (because we call a lot of functions on the |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at b788c09. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..35877
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Per discussion with @RyanCavanaugh today, I'm going to revert the change for namespaces for the time being. |
rbuckton
force-pushed
the
import-implied-receiver
branch
from
b788c09
to
80ac673
Compare
# Conflicts: # tests/baselines/reference/inlineJsxFactoryDeclarationsLocalTypes.js
|
Ah, that's a typo in the externalModules.ts test. |
|
It looks like we will need to revert this change until TS 4.4 while we investigate the significant performance regression this causes during emit. In the worst case, we may need to land this behind a flag to opt-in to the change, though ideally I can find a mitigation for the performance regression so that we can land this without a flag. |
|
Reverting this seems like it will restore the active breakage on a number of my shims like Promise.allSettled, where the module.exports function explicitly relies on having an undefined receiver. Why does a performance regression matter when compared to runtime breakage? |
|
@rbuckton ping, TS is severely broken without this fix; any update on restoring it? |
|
We re-re-discussed this at #44555. We've always tried to balance reasonable/canonical-looking emit and general UX with exact semantics of the spec. We're not ready to sacrifice that and also impose an emit overhead on everyone, especially when performance is something we're really working to improve. If we can find some optimizations that make this "free-ish", we'd feel better about this feature; otherwise, we'll need to put it behind a flag (like
The bigger risk here is actually imposing runtime breakage on existing users who accidentally came to rely on the existing behavior. Given that, the safer thing might actually be to put it behind a flag. |
|
@DanielRosenwasser so you’re worried about users who expect that, defying the way it works everywhere else in the JS exosystem, an exported function’s receiver will be the module namespace object when called as a bare function? I’d love to understand what code is relying on this behavior. |
Yes. Improved spec compliance has caused problems before, e.g. when we made exports immutable/nonenumerable (jasmine/jasmine#1817, #38568). |
|
Alright - can it at least be landed behind a flag, so i can tell users how to unbreak their applications? |
|
We're definitely going to figure something out, just trying to work out what exactly that is |
|
We were able to re-land this in #44624 in a way that does not significantly impact emit performance. |
|
That's amazing, great to hear! |
fixes: #35420
@rbuckton for review