Open mysteryven opened 3 weeks ago
I'm tempted to add TSIdentifierReference
:-)
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`
}
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:
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.
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) {}
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?
Thanks for putting this together @DonIsaac. A couple of responses:
- 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.
- 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 commit5d70bf894efe9014d24d4ba439807311e5b33fb8
- createNameResolver, which resolves a symbol from a name by walking up a scope tree;
onFailedToResolveSymbol
inchecker.ts
, line 3130;onSuccessfullyResolvedSymbol
inchecker.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.
The PR Don is referring to in his comment above is #3863.
React has side effect when import: https://github.com/typescript-eslint/typescript-eslint/issues/2455#issuecomment-685015542
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>;
};
We treat function parameter
foo
's type refers to itself.