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.

[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/[email protected]".;

[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())