Convert destruction

[Kingwl]

Fixes #29917
Fixes #25946
Fixes #30996

Well, the PR comes after 1 year.

This PR add a refactor that allow us convert between access expression and destruction.

The behavior follow below rules:

• Not work with javascript
• Edits on current file only
• Work on variable like declaration but not Enum member.
• Trigger if selected node is the left part of access expression: [|a|].b // apply at a, [|a.b|].c // apply at a.b
• Work with property access and constant element access: a["xxx"] // ok, a[foo] // not work
• Not work if type of selection narrowed into another type unless the referenced type is not changed with the original type
• Ignore if references has some method call (missing this) [|a|].b()
• Ignore if reference in assignment target
• Ignore if the argumentExpression of element access is not a valid identifier, eg: a["w-t-f"]
• Ignore if reference is before declaration in the code (hoisting).
• Ignore if reference is inside a function but not in body.
• Generate new name if not conflict with others
• If all access is numeric index access and they all less than 15, Call it Dense
• Generate index_i as array destruction binding for Dense array destruction
• Generate i: index_i as object destruction binding for array destruction
• Numeric index signature will convert into object like destruction

Todo:

• Basic find references and apply changes
• Smart selection
• Unique name resolve
• Method call detection
• Reference assignment detection
• Object literal like destruction
• array like destruction
• Convert back into property access
• Copy comments
• Tests

Known Issues

• First Reference should at top level
• Element access not work
• Element name collapse
• Element access with invalid identifier

[Kingwl]

@typescript-bot pack this.

[typescript-bot]

Heya @Kingwl, I've started to run the tarball bundle task on this PR at bf481c1. You can monitor the build here.

[typescript-bot]

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/81132/artifacts?artifactName=tgz&fileId=7C28FAB521A6AE9DC7B55FC8B8CA6DB961FA39DDE0F862BAB1E4D75BE97BA6F802&fileName=/typescript-4.0.0-insiders.20200731.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[Kingwl]

@typescript-bot pack this.

[typescript-bot]

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 39d2ccb. You can monitor the build here.

[typescript-bot]

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/81294/artifacts?artifactName=tgz&fileId=D09A0AD99616727A37D2D412E06446B64951A11D5FD8CDE501A8D85B750F59E902&fileName=/typescript-4.0.0-insiders.20200803.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[Kingwl]

Emmmmmmmm... Up

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #29917. If you can get it accepted, this PR will have a better chance of being reviewed.

[Kingwl]

Bad news..

 interface I { a: string, b: number }
 function foo (item: I) {
    call(item./*a*/a/*b*/, item.b)
 }

I'm stuck on common parent node lookup....... Seems it's very expansive if we lookup on every references.

[sandersn]

@Kingwl is there some way we can help on this? Maybe if we had somebody look at the PR and try to think of a fast way to find common parents? (or a way to avoid needing to find them)

[Kingwl]

I guess we can simplify the code by anders's control flow changes.

[Kingwl]

I will pick this up soon.

[sandersn]

@Kingwl do you still have time to work on this? Otherwise let's close it.

[Kingwl]

@Kingwl do you still have time to work on this? Otherwise let's close it.

Emmmmm. I guess we can ignore about the type narrow related logic now. So It's might be more simple

[sandersn]

Without narrowing, it's nearly done, right?

[Kingwl]

Without narrowing, it's nearly done, right?

In my memory, yes. I'll try to pick this up today or tomorrow(UTC+8)😜

[Kingwl]

Shit. I guess I cannot pick this up...

[Kingwl]

Bad news..

 interface I { a: string, b: number }
 function foo (item: I) {
    call(item./*a*/a/*b*/, item.b)
 }

I'm stuck on common parent node lookup....... Seems it's very expansive if we lookup on every references.

Hey @sandersn ! Would you mind give some suggestions about this?

The blocker is I have no idea to find a position to insert the destruction declaration.

[sandersn]

You'll have to remind me: how does the current code fail to find the right position? Is it that find-all-refs for a returns a: string, incorrectly setting the position to insert the destructuring to be the first line of your example?

Maybe if you find-all-refs on item you can use "the first line after item is declared".

[Kingwl]

@sandersn Well, It's not a good case. Below is a better one:

 interface I { a: string, b: number }
 interface II { i: I }
 function foo (item: II) {
    call(item.i./*a*/a/*b*/, item.i.b)
 }

It's suppose to be convert as:

 interface I { a: string, b: number }
 interface II { i: I }
 function foo (item: II) {
    const { a, b } = item.i
    call(a, b)
 }

But how could I find the position to insert the destruction?
valueDeclaration of item.i is a propertySignature.
Is that means I should find the left most symbol (in this case: item)?

[sandersn]

Well, it's a good start anyway. That's the symbol that's actually in scope at the point where the refactoring is invoked.