Workaround for Import/Export Declarations can break chained transformers

[dko-slapdash]

Hi.

The library uses createLiteral() (which is deprecated in the last TS BTW) which creates a token with parent=undefined. And other transformers (like typescript-is) may not like this: they expect parent to always be defined.

Steps to repro:

a.ts:

import b from "b"; // 
import { assertType } from "typescript-is";

interface A {
  a: number;
}

console.log(b);
console.log(assertType<A>({ a: 10 }));

package.json:

{
  "name": "typescript-transform-paths-bug",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "ttypescript": "*",
    "typescript": "*",
    "typescript-is": "*",
    "typescript-transform-paths": "*"
  }
}

tsconfig.json:

{
  "compilerOptions": {
    "skipLibCheck": true,
    "jsx": "react",
    "moduleResolution": "node",
    "experimentalDecorators": true,
    "pretty": true,
    "resolveJsonModule": true,
    "target": "esnext",
    "esModuleInterop": true,
    "noImplicitReturns": true,
    "strict": true,
    "paths": { "*": ["*"] },
    "plugins": [
      { "transform": "typescript-transform-paths" },
      { "transform": "typescript-is/lib/transform-inline/transformer" },
    ],
    "rootDir": "src",
    "outDir": "dist",
    "baseUrl": "src"
  }
}

It crashes TS with the following error:

Caused By: TypeError: Cannot read property 'kind' of undefined
    at Object.isImportDeclaration (node_modules/typescript/lib/typescript.js:26795:21)
- or -
Caused By: Error: start < 0
    at createTextSpan

I printed what's in the original node.moduleSpecifier and what's in p here: https://github.com/LeDDGroup/typescript-transform-paths/blob/master/src/index.ts#L228 - and here is the result:

If I manually set p.parent = node.moduleSpecifier.parent prior to calling Object.assign(), the crash disappears (but only for some cases).

Potential solution

I think that createLiteral() creates a "too raw" literal; the screenshot above shows that the properties of the "original" moduleSpecifier is very different from the new one (e.g. parent, pos, flags etc.). I tried to use factory.cloneNode(), but it's not for these purposes and didn't work, there must be something else to replace string literals in the code inplace.

P.S.
I think this topic is related to typescript-transform-paths and not to e.g. typescript-is, because it's typescript-transform-paths who generates a "slightly non native" AST. Thus, other transformers which reasonably expect to receive a "native" AST will break.

[nonara]

@dko-slapdash Thanks for the report. There were several issues due to that. It was an attempt to workaround an existing bug in TS. The latest version has an updated method which creates a mutable clone of the existing node.

createLiteral is okay to use. The issue is that that particular replacement was done in a way that circumvented the internal logic to fixup transformed nodes during transformation, which, among other things, caused the parent not to be set.

Please try out the latest version and let me know if you have any issues. If the issue is related to no parent, the latest should work for you.

PS. I appreciate the detail in your issue! Thanks for the added diligence.

[dko-slapdash]

Thanks! I've just tried the version from master, got another error:

Caused By: TypeError: Cannot read property 'text' of undefined
    at getErrorSpanForNode (node_modules/typescript/lib/typescript.js:14332:40)
    at createDiagnosticForNodeInSourceFile

Same steps to reproduce as above.

[dko-slapdash]

I think cloneNode() doesn't do what is expected from its name. Here is the result of such cloning:

We can see that pos/end/parent are still different.

[nonara]

@dko-slapdash Thanks. I'll look into this tomorrow.

I think cloneNode() doesn't do what is expected from its name

cloneNode is the TS4 replacement for getMutableClone. It works as expected / documented. pos and end being -1 is actually the expected value. Any factory.create/update... function outputs nodes with the same values for position. The compiler fixes up the position detail and parent for new nodes after transformation.

This is part of a sort of ongoing battle to find a workaround for an issue in the TS compiler: microsoft/TypeScript#40603

The short version is, I can't replace either an ImportDeclaration or ExportDeclaration or the literal node for its moduleSpecifier property using the common factory methods without it breaking type elision in emit. Right now there's another issue with watch mode that's related to this as well. The latest patch seems to have better coverage, but it seems there are edge cases relating to a diagnostic being attached to the previous node.

I have some ideas, so I'll have a look tomorrow and see if I can get it working for these edge cases.

[dko-slapdash]

Interesting.

What about watch mode BTW? We sometime notice that tsc stops responding to some particular (random) *.ts files changes in watch mode, and also that for such files, typescript-transform-paths v1 also stops outputting the right paths. Not possible to reproduce unfortunately. Is it somehow related to what you mentioned about watch mode?

[nonara]

What about watch mode BTW?

Watch mode can be tricky. We've seen quite a few issues related to watch mode in issues for ttypescript / ts-patch. It's been awhile since I've been in that part of the TS codebase, and I haven't dug too deep in, so I can't speak with total authority, however, if memory serves, watch mode minimizes redundancy as much as possible. I believe it rebuilds the Program instance using the former and recomputes SourceFile as needed. This can cause issues if something breaks in the original nodes during transformation.

If you're doing everything by the book, ie. using the prescribed factory functions to transform and create nodes during transform, things usually are able to track nicely. It can begin to break with any colouring outside the lines. Historically, doing so has been necessary to compensate for limitations with the compiler functions, or in other cases it's done simply because the TS compiler API is obscure and transformer authors understandably don't know the best practices.

In our case, we are having to go outside of that for the time being. I believe there's a path to doing what we need with the mutable clone route. When TS fixes the bug upstream, we'll convert over to the proper method.

I do notice that these errors seem to crop up around diagnostics being attached to a node which was improperly replaced. I don't think v1 of this library did anything like that, but I don't completely recall, as I wasn't involved at that point. It's also possible some things break down over time with continual rebuilding from some issue happening within TS where the AST / type info gets corrupted. I noticed that happening with the tsserverlibrary consumed by webstorm in the past, so I'd need to restart the TS server. It usually seems diagnostic related when it does.

If you do encounter anything like that in the new version once we've got this issue sorted, please let me know. Most of the issues that get filed related to watch mode for ttypescript / tspatch don't have any repro, so I can't do anything. If there is an issue in TS, it'd be good to track that out.

[nonara]

Ok. I'm looking over this now. Couple things:

1. As of TS 4, the factory functions were migrated from the ts namespace to a factory instance.

   During that migration, some methods were renamed for clarity. I don't believe that createStringLiteral existed originally. I believe there was only createLiteral which was later split. In either case, however, they end up producing the same AST. Using createLiteral doesn't create "non-native" AST. Again, parent, pos, end, flags and other data is set via the compiler after processing the new node.

2. Did you mean to not include a b.ts ?

[dko-slapdash]

b.ts is arbitrary - it can just be

export const b = 42;

P.S.
We’re currently using v1 with ts 4.1 successfully (at least for one day). I hope it will continue working that way. And we can’t use v2 due to the above conflict with typescript-is library.

[nonara]

Ok, here's my update on this.

typescript-is appears to be trying to resolving the module name via the typechecker and failing. It calls getResolvedSignature which eventually fails at resolveExternalModule. That diagnostic gets swallowed due to no parent set for the node, as it tries to walk up the chain to get the SourceFile.

The diagnostic looks like this:

After some exploration, it looks like although it's functionally viable to replace the moduleSpecifier with a new StringLiteral and set the originalNode property on that StringLiteral, it appears that some parts of TS, like the resolution method mentioned above, pulls the moduleSpecifier from the parent's original property, which is likely why we're getting the error shown above. In a normal situation, it would resolve using the original TS node and not produce a diagnostic error.

That unfortunately takes us back to our original problem, which is that if we set an original on the parent declaration, type elision breaks.

To sum it up, I was hoping I could find a reasonable way around it to replicate the internal behaviour, but I'm at a point where I don't think it would be prudent to investigate trying to circumvent the setup any further, as it'd be too hacky. I think the best bet is to wait on TS for this.

I will leave this issue open, and will notify when we have it sorted.

[nonara]

Just a heads up - this is fixed in v2.1.0. We implemented proper type elision. No more workarounds.

If interested, more detail can be found in #81