Incorrect reference resolve in function parameter's type

[mysteryven]

import * as foo from 'foo';

function bar(foo: foo) {}

We treat function parameter foo's type refers to itself.

1. our playground
2. typescript playground

[Boshen]

I'm tempted to add TSIdentifierReference :-)

[overlookmotel]

This is tricky. As the example above demonstrates, there are two parallel scope chains - variables and types. Other examples which make this even clearer:

// 2 bindings with same name in same scope
type Foo = number;
const Foo: Foo = 123;
function f(foo: Foo) { // `Foo` here refers to `type Foo`
    return Foo; // `Foo` here refers to `const Foo`
}

let Bar = 456;
function f2<Bar>(foo: Bar) { // `Bar` here refers to `<Bar>`
    return Bar; // `Bar` here refers to `let Bar`
}

TS playground

To make it worse, these 2 scope chains are not entirely independent. There are places where a binding can be "dual use" - can be used as either a variable or a type:

import X from 'blah';
function f2(x: X) { return X; }

class Y {}
function f(y: Y) { return Y; }

Are there any other "dual use" cases apart from import and class?

I think we have 2 choices:

1. Adapt our scope tree to include both variable and type bindings.
2. Don't create bindings for type definitions, and don't resolve TS identifier references at all (so foo in import * as foo in original example would have no references).

Option 1 is ideal from a completeness perspective, and could be useful for tools which want to do any form of type-checking/type analysis e.g. linter. But it'd require major changes to design of scope tree - need to support 2 bindings with same name in a scope.

Option 2 solves the problem by going the other way - completely opting out of resolving type identifier references.

[overlookmotel]

If we go with option 2, and don't resolve type references, I'm not sure we can accurately determine which imports to strip out in TS->JS transform. e.g.:

import Foo from 'foo';
function f(foo: Foo) {}

import Bar from 'bar';
function f2<Bar>(bar: Bar) {}

TS Playground

tsc strips out both import statements in the Playground, but is that right, and would it be the same if doing a full-application build where it can examine the contents of 'bar' module? What if 'bar' has side effects?

[DonIsaac]

Thanks for putting this together @DonIsaac. A couple of responses:

1. This PR aims to resolve reference identifiers correctly depending on whether they are types or variables. I'm not sure if this is what we want to do or not. As mentioned in Incorrect reference resolve in function parameter's type #3799 (comment), maybe the best solution is not to resolve type identifier references at all. i.e. The scope tree becomes purely for variable bindings and type bindings/references are ignored.
2. If we do want to include type bindings in the scope tree, and resolve references to them, then this PR is a good way to go about it, but it's not complete. In particular, we need to figure out how to support 2 bindings with same name in the same scope (1 variable binding + 1 type binding) - example below. Also, I don't believe semantic at present visits all type identifier references, and therefore doesn't resolve them all.

type Foo = number;
const Foo = 123;
function f(foo: Foo) { // `Foo` here refers to `type Foo`
    return Foo; // `Foo` here refers to `const Foo`
}

I think before going further, we should figure out what our goal is.

Meaning-based symbol resolution is a requirement if we want to include symbols in the symbol table. This means that code such as

type T = number
const x = T

also does not work right now. I am strongly against removing types from our symbol table, since it will hinder our ability to effectively lint and process typescript code. My end goal is to support type-aware linting and minification, and removing type symbols will be a step backwards in this regard.

Some useful references on meaning-based symbol resolution are below:

checker.ts is too large to support github permalinks. Line references to this file are from commit 5d70bf894efe9014d24d4ba439807311e5b33fb8

• createNameResolver, which resolves a symbol from a name by walking up a scope tree;
• onFailedToResolveSymbol in checker.ts, line 3130;
• onSuccessfullyResolvedSymbol in checker.ts, line 3193

I agree that this PR is not complete until we support type read references; I see a lack of this resolution as a bug. I'm not certain if this PR should contain a fix for it or if we should make another, separate PR. Unfortunately I cannot make stacked PRs since I'm not part of the Graphite organisation.

[overlookmotel]

The PR Don is referring to in his comment above is #3863.

[mysteryven]

React has side effect when import: typescript-eslint/typescript-eslint#2455 (comment)

The below cases also need a value reference, even it doesn't have:

import React from 'react';

export const ComponentFoo: React.FC = () => {
	return <div>Foo Foo</div>;
};

https://github.com/typescript-eslint/typescript-eslint/pull/2498/files#diff-509f6837d9267e9df4d390a9325e9c44911c5d0cd42a0d3e16658ee9ab7924dd