Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor collectVariableUsage #38

Open
ajafff opened this issue Mar 21, 2018 · 3 comments · May be fixed by #84
Open

Refactor collectVariableUsage #38

ajafff opened this issue Mar 21, 2018 · 3 comments · May be fixed by #84
Assignees

Comments

@ajafff
Copy link
Owner

ajafff commented Mar 21, 2018

The current API is somewhat difficult to use and does too much by analyzing the whole source file ahead of time.

The new API should return an object that can be used to:

  • get all declarations of an identifier (also with an identifier of a usage)
  • get all uses of a declaration (allow filtering by domain)
  • check if identifier is in scope
  • get a list of all declarations in the file

optionally:

  • get all identifiers in scope (optionally filtered by kind)
  • get all locations where the identifier is in scope (excluding TDZ)
  • lazily analyze only the scopes necessary to answer requests

Needs a good name:

  • ScopeAnalyzer
  • UsageTracker
  • ReferenceFinder
@ajafff ajafff added this to the v3.0.0 milestone Mar 21, 2018
@ajafff
Copy link
Owner Author

ajafff commented Jul 29, 2018

This doesn't need to be shipped in a new major version. It can simply be published once it's ready while still keeping the old API around

@ajafff ajafff removed this from the v3.0.0 milestone Nov 1, 2018
@ajafff ajafff self-assigned this Nov 1, 2018
@ajafff
Copy link
Owner Author

ajafff commented Nov 3, 2018

It turns out that this analysis in not possible without the type checker. Namespace merging and imports in namespaces can introduce identifiers whose domain is not known. That's already an issue in the current implementation, which has yet to be reported.

// foo.ts
namespace foo {
    export class C {}
}

// bar.ts
class C {}
namespace foo {
    new C(); // references `foo.C`
}
namespace bar {
    import C = foo.C;
    new C(); // references `foo.C`
}

Everything that doesn't involve namespaces during name resolution could still work as expected.

TBD: how to proceed from here?

  • keep the current, broken implementation
  • require TypeChecker
    • always
    • only if namespaces are involved (might be very surprising if this never occurs in tests and only crashes on user code)

@ajafff
Copy link
Owner Author

ajafff commented Nov 10, 2018

It might be the best to simply return undefined if the result of a request is not fully known. That is: if no TypeChecker was provided and there is a namespace involved (or an import)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant