imported function call without leading semicolon

[aleclarson]

Repro link

import {fun} from './foo'
let arr = []
fun()

..becomes..

"use strict";var _foo = require('./foo');
let arr = []
(0, _foo.fun)()

..which evaluates to TypeError: [] is not a function

[alangpierce]

Thanks. JS is the worst...

This seems hard, but there may be a nice fix with enough creativity. Generally the straightforward fix is to put a semicolon either after the [] or before the (0, but detecting the situations where we need to do that is possible but not trivial. The parser knows when we start a new statement, so it could leave a marker whenever that happens and then the transformer could insert a ; when necessary.

Interestingly, this only happens because of the (0, _foo.fun) trick so that _foo isn't passed as the this. Maybe there's an alternate trick that doesn't start with an open-paren, or something like that. One idea is to generate an identity function _id at the top of the file (with a guaranteed-unique name) and write it as _id(_foo.fun)(). That way it still syntactically starts with an identifier.

I probably won't have time to get to this really soon, but will take a closer look when I get a chance. Also happy to accept a PR for the _id suggestion above.

[aleclarson]

I bet you could just run Prettier on the output. lol

[alangpierce]

Prettier's too slow. 😛

[alangpierce]

I was curious on details, so just to add some actual numbers to my off-the-cuff statement, looks like Prettier is about as slow as Babel, at least in simple cases:

Simulating 100 compilations of expression.ts:
Sucrase: 400.329ms
Prettier: 6578.802ms
ESLint: 14548.790ms
TypeScript: 2802.569ms
Babel: 5491.316ms

[aleclarson]

Which modules are related to this specific case?

edit: I found this:

sucrase/src/transformers/CJSImportTransformer.ts

Lines 180 to 196 in 9fdabab

	// We need to change to (0, f) if this is a function call, so that it won't be interpreted as a
	// method access. This can also happen in situations like (f)(), so a following close-paren
	// should trigger this behavior as well if it eventually has an open-paren. In some cases, like
	// export assignees, we must NOT turn the identifier into a normal expression, so we need to
	// just to the regular replacement.
	let possibleOpenParenIndex = this.tokens.currentIndex() + 1;
	while (
	possibleOpenParenIndex < this.tokens.tokens.length &&
	this.tokens.tokens[possibleOpenParenIndex].type === tt.parenR
	) {
	possibleOpenParenIndex++;
	}
	if (this.tokens.tokens[possibleOpenParenIndex].type === tt.parenL) {
	this.tokens.replaceToken(`(0, ${replacement})`);
	} else {
	this.tokens.replaceToken(replacement);
	}

Are there more, or is that it?

[aleclarson]

We should transpile to _foo.fun.call(void 0, ...). WDYT?

[alangpierce]

@aleclarson Yep, that's the main place where the replacement happens. There's also one similar case in ReactDisplayNameTransformer when doing custom processing on createReactClass calls.

Can we just call _foo.fun.call(void 0, ...)?

Good point, in general I think that should work, but here's an example case that makes me worry:

https://sucrase.io/#code=import%20a%20from%20'a'%3B%0Aimport%20b%20from%20'b'%3B%0A(a%20%2B%20b)()

Right now it's a benign false positive in Sucrase since (0, foo) is a valid replacement for foo even if foo isn't a function. Inserting .call( actually would still work in that case (at least preserve behavior), but it's an example of where you can have an identifier followed by some close-parens followed by an open-paren and the identifier wasn't a function. But it might still be the case that the open-paren will always be a function call in that situation, and that .call is valid.

For the _id approach I described above above, any transformer can add to getPrefixCode to generate a function declaration at the top of the file, and you can do this.nameManager.claimFreeName("_id") to generate a guaranteed-unique name. CJSImportProcessor already does that with _interopRequireWildcard, so that's an example you could use. You may actually just want to put most of the new code as a method in CJSImportProcessor since that's also accessible to ReactDisplayNameTransformer. This would also generate some new boilerplate at the top of a bunch of files, so tests would need to be updated accordingly, something like how IMPORT_WILDCARD_PREFIX is used.

The CI test suite runs on a bunch of open source projects (yarn run-examples locally), so I'm fine with either approach as long as that passes.

Sounds like you're interested in taking this on? Happy to chat through it more either here or on Gitter. I was also still hoping to get to this at some point, and I have some free time coming up, so if you get stuck, I can take over.

[alangpierce]

To clarify the call thing:

Right now, the algorithm for replacing f with (0, f) starts by looking at all identifiers in the code. For each one, it checks if all of the following are true:

• It has been marked as IdentifierRole.Access by the parse step, meaning it's a plain expression rather than a parameter name, object key, class name, etc.
• The identifier references a name in the global scope rather than e.g. being a local variable that happens to shadow a global name. (This is determined by the parse step and some preprocessing.)
• There exists a result for getIdentifierReplacement, meaning that this is a name defined by an import statement and thus needs a corresponding replacement like _myImportedModule.myFunction.
• The identifier is followed by some number of ) characters and then a ( character. This is a rough attempt at determining if this is a function call.

If all of those are true, then we do the (0, f) thing, using the getIdentifierReplacement result as well.

The thing I'm pondering is whether that ( from the last bullet point is guaranteed to be a function call. If it's not a function call, then replacing ( with .call(void 0, would be wrong. It's a little riskier than the (0, f) thing since the (0, f) thing still works even if we had a false positive and it wasn't a function call.

Some example cases to think about: f(x), (f)(x), ((f))(x), f(g)(x), (f + g)(x). In all of these cases, the open-paren after the identifier is a function call, so all of these are safe to transform to .call(void 0, . Note that f(g)(x) and (f + g)(x) are a bit interesting since g does not need to be transformed, but it's still safe to transform anyway.

[aleclarson]

If we detect a right paren(s) before the left paren, then using (0, f) is safe. The issue I've reported is always with a preceding statement; never when calling an imported function inside an expression (which a right paren indicates we're in).

If no right parens are detected, then using f.call(0) is safe, I think.

WDYT?

[alangpierce]

Good point. I think I'd probably still prefer to keep the same strategy everywhere, so I think I'd prefer to do rootTransformer.processToken on the close-parens and then replace the open-paren with f.call(void 0, , and if it passes tests, then seems fine to ship. If that adds implementation complexity or turns out to be wrong for some subtle reason, certainly seems reasonable to do either (0, f) or f.call(void 0, based on context.

[aleclarson]

What does this comment mean?

In some cases, like export assignees, we must NOT turn the identifier into a normal expression, so we need to just to the regular replacement.

[alangpierce]

(Oops, typo, it should be "just do the regular replacement".)

I think it's referring to cases like this:

export let x = 1;
x = 2;

Without being careful, we might transform the second line to (0, exports.x) = 2;, which isn't a valid assignment. It's from #180 , where I switched the export approach to use variable replacement and changed the replacement strategy from "do the (0, f) thing everywhere for simplicity" to a more targeted approach.

If you're reworking the code, not super-important to keep that line, I think it made more sense in the original context.