Add inline variable refactor

[MariaSolOs]

Fixes #18459

[typescript-bot]

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

[typescript-bot]

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

[typescript-bot]

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

[mjbvz]

Excited to test this out!

[andrewbranch]

I think there are a couple other invalid inlinings to consider.

1. Type queries:

   const Bar = class Bar {}
   type BarConstructor = typeof Bar;

   typeof can only take an identifier reference.
2. Merged type/value meanings:

   const a = "hello";
   type a = any;
   type T = a;

   There’s one symbol for a with two declarations, so Find All References might return a in type T = a as a reference, even though you wouldn’t want to inline "hello" there. The real case for merging meanings is usually more like

   // Simulate type/value duality of real class declarations
   const SomeClass = mixin(OtherClass);
   type SomeClass = typeof SomeClass;

   and in those cases it seems undesirable to inline anyway. I think you can implement a simple rule that says if a symbol has more than one declaration, it can’t be inlined.

[andrewbranch]

This is looking really good! The implementation is impressively small and readable and I like that each test is very focused.

Have you tried to beat this up on some more complicated real-world code? Do that if you haven’t already, and then I would also suggest having one or two kitchen-sink tests that exercise more complex cases with several references and complex expressions (multi-line function expression?)

[MariaSolOs]

@andrewbranch I've added a couple more tests with more complex scenarios like call expressions. LMK what you think :)

[andrewbranch]

I still think inlining a multi-line expression would be good to test:

function Component() {
  const onClick /*inline me*/ = () => {
    console.log("clicked");
  };
  return (
    <button onClick={onClick}>
      Do useful thing
    </button>
  );
}

[DanielRosenwasser]

@typescript-bot pack this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100

[andrewbranch]

Looks good to me, nice job 👍

[DanielRosenwasser]

I SAID

@typescript-bot pack this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 2f97c5c. You can monitor the build here.

Update: The results are in!

[typescript-bot]

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

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 2f97c5c. You can monitor the build here.

Update: The results are in!

[DanielRosenwasser]

That's what I thought.

[typescript-bot]

Hey @DanielRosenwasser, 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/155242/artifacts?artifactName=tgz&fileId=20071FB31E0A3D7DF07B6D5E41FC11EFD71181FE4C4C85CE4B27E7183150EAC902&fileName=/typescript-5.2.0-insiders.20230531.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54281-11".;

[typescript-bot]

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54281/merge:

Everything looks good!

[DanielRosenwasser]

Here's a fun one I just tried out:

const /**/yadda = () => yadda;

Error: Debug Failure. False expression: Changes overlap
Verbose Debug Information: {"pos":4,"end":29} and {"pos":24,"end":29}
    at c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:166694:15
    at mapDefined (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:2644:22)
    at Object.getTextChangesFromChanges (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:166690:12)
    at ChangeTracker.getChanges (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:166587:35)
    at ChangeTracker.with (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:166071:20)
    at Object.getEditsForAction (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:136427:60)
    at Object.getEditsForRefactor (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:135605:31)
    at Object.getEditsForRefactor2 [as getEditsForRefactor] (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:142982:32)
    at IpcIOSession.getEditsForRefactor (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:182282:49)
    at getEditsForRefactor (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:180538:43)
    at c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:182666:69
    at IpcIOSession.executeWithRequestId (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:182658:14)
    at IpcIOSession.executeCommand (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:182666:29)
    at IpcIOSession.onMessage (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:182708:51)
    at process.<anonymous> (c:/Users/Daniel/workspace/node_modules/typescript/lib/tsserver.js:184288:14)
    at process.emit (node:events:513:28)
    at emit (node:internal/child_process:958:14)
    at process.processTicksAndRejections (node:internal/process/task_queues:84:21)

[MariaSolOs]

Also, I noticed that if I request an inline at the following position

let/**/ asdasd = 42;

let qwerty = asdasd;

the refactoring is made available, but not elsewhere on the let (e.g. l/**/et asdasd = ...).

I feel like it should either work solely on:

1. The variable name
2. The variable name or the variable statement keyword
3. The entire variable statement

@DanielRosenwasser I believe we should stick with option 1, since there would be an ambiguity about what to inline in cases like let x = 0, y = 1 with the other 2 options.

However, if I change this bit to:

if (isIdentifier(token) && isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) {

The refactor is still available in let/**/ foo = 0;, since I believe that's leading trivia that belongs to the identifier node. I think this should be fine...?

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

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

[typescript-bot]

Hey @DanielRosenwasser, 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/155454/artifacts?artifactName=tgz&fileId=6087B18552E765F2B38E8662F37137B2540138A75BDE31954CDB44014CC8C60C02&fileName=/typescript-5.2.0-insiders.20230612.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54281-25".;

[awerlogus]

Inlining variable data changes runtime behaviour. Is it bug or design limitation?

const data = new Map()

const getter = () => data.get('foo')

data.set('foo', 'bar')

console.log(getter())