"implement interface" action produces bloated result when argument type has recursive type parameters

[mjwach]

Hi, I'm trying to tell Atom to add skeletal methods to my class that is supposed to (but doesn't yet) implement an interface. It is producing an unusably lengthy result.

I tried to find where this feature is implemented in the atom-typescript code. It looked like it was coming from a different library from this one, maybe? I didn't understand very well what I was looking at. Anyway, if this isn't something you guys can fix, can you point me to the GitHub project (or whatever other place) where I can more appropriately report this?

As for the actual error - the specific problem can be demonstrated with this test case:

A.ts

import type Target from "./Aa";

interface Interfo { enact(target: Target): void; }

export default class Implem implements Interfo
{
}

Aa.ts

export default class Target<T0 extends TNode = TNode, T1 extends TNode = TNode>
{
}

type TNode = Target | null;

Implem doesn't implement enact() so its name gets an error marking on it. If I move my cursor to that marking and press alt-enter, then after two or three seconds of excessive thought, Atom offers to add an implementing method. If I tell it to do so, it inserts an insanely long result with lots of unnecessary "Target<Target<Target<Target<Target<Target<Target<Target<Target" stuff in it, instead of the simple result that I expect.

Putting the Target and TNode definitions into A.ts removes the bloat. (It may be relevant that I'm using the isolatedModules option.)

[lierdakil]

Hi. This feature is provided by the TypeScript server which is developed in the main TypeScript repo. If you want to report this as an issue, you probably want to report it there: https://github.com/microsoft/TypeScript/issues.

However, I'm not convinced this is an issue actually. By default, TypeScript works in "lax" mode, which, amongst other things, ignores null and undefined when doing type checking and inference. As a consequence, your type TNode = Target | null is actually interpreted as type TNode = Target in lax mode, and so mad recursion is pretty much guaranteed at this point (good thing there's a limit on recursion depth). To make TypeScript care about nulls, you need to enable strictNullChecks in tsconfig.json.  With strictNullChecks enabled, everything works as expected.

[mjwach]

I already had the "strict" option enabled; I have now also tried with "strictNullChecks" on instead, and with both flags on. None of that fixed this for me. I think I've got the latest versions of the relevant libraries. But anyway I will abandon this issue and take my problem to Microsoft, unless you have more to add. Thanks!

[mjwach]

Postscript: I couldn't figure out how to tell Microsoft how to reproduce this problem, so I gave up and started using a silly workaround in which I export from Aa.ts both the generic class/type Target and an additional type called DefaultTarget that is equal to Target. The latter has no type parameters (it captures the default values: T0=TNode, T1=TNode), so using it outside Aa.ts will not prompt any unnecessary specifications, recursive or otherwise, of type parameters.

[lierdakil]

Huh. I was too hasty in saying "With strictNullChecks enabled, everything works as expected." In my previous testing I neglected to recreate the file structure (which was done on the go via the typescript playground, so that was a limitation of the tooling), and was able to recreate the issue with strictNullChecks disabled. Then I jumped to conclusions. I apologize. The issue is reproducible with the instructions in OP and appropriate tsconfig.json (i.e. with "strict": true). Furthermore, it is reproducible in VSCode as well, so it's definitely not Atom-specific.

That said, after recreating the structure and playing around with that, I believe I know what's going on there. The type TNode isn't in context in A.ts and it can't be since it's not exported. Exporting TNode from Aa.ts seems to "fix" the issue: https://gist.github.com/lierdakil/5f4601be0e55b0183d5561e5561e1eb1

Furthermore, if TNode is marked as an export in Aa.ts then running "implement interface" codefix automatically imports TNode if it's not already imported.

[lierdakil]

Here's a quick demo

[mjwach]

Ah you're right, that works for me too. So now I have two workarounds, thanks.

What I really want is for the automatic implementation to just leave out the type parameters altogether and say only "target: Target" like the interface did - the compiler does not seem to mind if I do it that way manually, so I guess it should be willing to do it automatically. But this is good enough.

[github-actions[bot]]

This issue has been marked as stale because it did not have any activity for the last 90 days or more. Remove the stale label or comment or this will be closed in 14 days