Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(parser): Draft of scope analysis with types#233

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
armano2 wants to merge13 commits intotypescript-eslint:masterfromarmano2:scope2
Closed

Conversation

armano2
Copy link
Collaborator

@armano2armano2 commentedFeb 8, 2019
edited
Loading

This is early stage of updated scope-analysis with types included into scope, there is still a lot to do.
Goal of this PR is to solve#18,#19,#21,#60,#207#122,#249

@armano2armano2 added enhancementNew feature or request DO NOT MERGEPRs which should not be merged yet package: eslint-pluginIssues related to @typescript-eslint/eslint-plugin package: parserIssues related to @typescript-eslint/parser breaking changeThis change will require a new major version to be released and removed package: eslint-pluginIssues related to @typescript-eslint/eslint-plugin labelsFeb 8, 2019
@armano2armano2 self-assigned thisFeb 9, 2019
@armano2

This comment has been minimized.

@armano2
Copy link
CollaboratorAuthor

i decided to split this PR to few smaller one (fixes for non types), and after they are going to be merged i will continue working on types

#244#240#237#234

@armano2
Copy link
CollaboratorAuthor

looks like its working, we have to change logic inno-undef from eslint scope i have no access to global types defined by user or typescript, and there are false positives in this rule because of that:

const links = document.querySelectorAll( selector ) as NodeListOf<HTMLElement>

even if you specify globals for browser and es6 it will report thatNodeListOf is not defined.

@typescript-eslint/core-team@mysticatea i will appreciate some feedback (and maybe some suggestions for tests)

@mysticatea
Copy link
Member

Amazing!

Hm. I have thought that we should use twoScopeManager instance for variables and types because variables and types are different concepts.

In TypeScript, a declaration creates entities in at least one of three groups: namespace, type, or value. Namespace-creating declarations create a namespace, which contains names that are accessed using a dotted notation. Type-creating declarations do just that: they create a type that is visible with the declared shape and bound to the given name. Lastly, value-creating declarations create values that are visible in the output JavaScript.
https://www.typescriptlang.org/docs/handbook/declaration-merging.html

Varaible references must not resolve to types and type references must not resolve to variables. Some entities such as classes and enums belong to both the variable scope and the type scope.

(soclass A {} interface A {} is redeclaration.)

Therefore, my thought is aScopeManager manages only variables and variable references for core rules, and anotherScopeManager manages only types and type references inparserServices. (but I'm not sure ifScopeManager for types is needed because we can useTypeChecker of TS compiler API.)

If a singleScopeManager manages both, I think that scopes will havetypes andtypeReferences, and the resolving logic connectsvariablesreferences andtypestypeReferences.

Though, we needTypeChecker to know imported bindings belong to which area. :(


About variable rules in the plugin, I believe that we should deprecate those to avoid doing duplication. But If we decide to maintain those, I think that we should implement those withTypeChecker API rather than modifying core rules.

@armano2
Copy link
CollaboratorAuthor

armano2 commentedFeb 15, 2019
edited
Loading

hmm, this is good idea to split it, but i think it should be one ScopeManager with separated refferences and variables that we can share only some declarations between between types and variables

i will change this that scope has 2 types of references and core rules are going to work only for variables

i'm going to have to fix only no-unused-vars to check for types and other rules will have to be re-implemented for types.


no-unused-variables and other rules/checks witch can be done by tsc, can be migrated but only after we solve performance issue with creating programs...


even if we decide to leverage tsc as replacement for some of rules we still need scope for types to implement rules not supported by it.

no-shadow is good example, typescript is not warning user about this and this is great rule to catch some potential issues

class foo<T> {  test<T>() {}}

with this new implementation (splitting references) we should consider makingno-shadow-types for this

thanks@mysticatea for this idea 👍

mysticatea reacted with thumbs up emoji

@JamesHenry
Copy link
Member

Please can we reopen this using the new WIP PR feature on GitHub?

kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull requestAug 27, 2019
* Update eslint-docs* Upgrade eslint-docs (take 2)
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsApr 21, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@bradzacherbradzacherbradzacher left review comments

Assignees

@armano2armano2

Labels
breaking changeThis change will require a new major version to be releasedDO NOT MERGEPRs which should not be merged yetenhancementNew feature or requestpackage: parserIssues related to @typescript-eslint/parser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@armano2@mysticatea@JamesHenry@bradzacher

[8]ページ先頭

©2009-2025 Movatter.jp