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

feat(typescript-lint): implement consistent indexed object style #3126

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

todor-a
Copy link
Contributor

@todor-a todor-a commented Apr 29, 2024

@github-actions github-actions bot added the A-linter Area - Linter label Apr 29, 2024
@todor-a todor-a marked this pull request as ready for review April 29, 2024 18:40
Copy link

codspeed-hq bot commented Apr 29, 2024

CodSpeed Performance Report

Merging #3126 will not alter performance

Comparing todor-a:feat-consistent-indexed-object-style (f799919) with main (2064ae9)

Summary

✅ 27 untouched benchmarks

@Boshen Boshen requested a review from mysteryven April 30, 2024 03:24
Comment on lines 107 to 112
TSType::TSUnknownKeyword(_) | TSType::TSAnyKeyword(_) => {
ctx.diagnostic(ConsistentIndexedObjectStyleDiagnostic(
"record",
"index signature",
idx.span,
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to diagnostic here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the TSAnyKeyword is for this test:

interface Foo {
  [key: string]: any;
}

you might ask, why not just error if there is an index signature, and the answer is that in some cases, even though the setting is Record, an index signature is allowed. For example this:

interface Foo<T> {
  [key: string]: Foo<T> | string;
}

Copy link
Member

@mysteryven mysteryven May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interface Foo {
[key: string]: Foo | string;
}

This is a case of circle.

you might ask, why not just error if there is an index signature

Yeah, I am curious about this. If we set it to use "index-signature", it has a false positive report to interface, which is not expected:

 typescript-eslint(consistent-indexed-object-style):A "record" is preferred over an "index signature".
   ╭─[consistent_indexed_object_style.tsx:2:13]
 1 │ interface Foo {
 2 │             [key: string]: any
   ·             ─────────┬────────
   ·                      ╰── A record is preferred over an index signature.
 3 │         }
   ╰────

the TSAnyKeyword is for this test:

Look like the test case failed because the default option "record".

So, to sum up, I think this part can be removed 🤔

TSType::TSUnknownKeyword(_) | TSType::TSAnyKeyword(_) => {
      ctx.diagnostic(ConsistentIndexedObjectStyleDiagnostic(
          "record",
          "index signature",
          idx.span,
      ));
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around wit this but could not find a way to remove the TSUnknownKeyword & TSAnyKeyword checks and still have the tests pass 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to add early bailout if we find a circle?

interface Foo {
    [key: string]: Foo | string;
}

The above case is a circle because Foo is used in its body, the minimal code of find a circle may like:

fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
    match node.kind() {
        AstKind::TSInterfaceDeclaration(decl) => {
           // --snip--
            if let Some(symbol_id) =  &decl.id.symbol_id.get() {
                for reference in ctx.symbols().get_resolved_references(*symbol_id) {
                    let node_id = reference.node_id();
                    let node = ctx.nodes().get_node(node_id);
                    if let AstKind::TSTypeReference(type_reference) = node.kind() {
                        // if the range of `type_reference`'s span is included in decl's body, 
                       // we think it's a circle.
                    }
            }
    }
}

if decl.body.body.len() == 1 {
for signature in &decl.body.body {
if let TSSignature::TSIndexSignature(idx) = signature {
match &idx.type_annotation.type_annotation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check idx.type_annotation.type_annotation, looks like original codes only check parameter's type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the following case:

interface Foo {
   [key: string]: Foo;
}

even with a setting of Record, the above is still valid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a circle case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I got what you meant here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For case

interface Foo {
   [key: string]: Foo;
}

It is valid because it has been treated as a circle, Foo in the second line refers to the interface name Foo, hope I understand right from here :)

@todor-a todor-a requested a review from mysteryven April 30, 2024 18:53
if let Statement::FunctionDeclaration(func) = body {
if let Some(return_type) = &func.return_type {
if !self.is_index_signature {
if let TSType::TSTypeLiteral(lit) = &return_type.type_annotation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we don't need to revisit TSType::TSTypeLiteral, since all TSType::TSTypeLiteral has been visited.

// if we want to visit all `TSTypeLiteral` in our rule 
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
      match node.kind() {
          AstKind::TSTypeLiteral(lit) => { } // here will visit all TSTypeLiteral in our file
      }
}

@todor-a todor-a force-pushed the feat-consistent-indexed-object-style branch from 1834331 to 8efa6aa Compare May 19, 2024 10:27
@github-actions github-actions bot added A-parser Area - Parser A-semantic Area - Semantic A-minifier Area - Minifier A-ast Area - AST A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation A-prettier Area - Prettier labels May 19, 2024
@todor-a todor-a force-pushed the feat-consistent-indexed-object-style branch from 8efa6aa to 5a07354 Compare May 19, 2024 10:28
@mysteryven
Copy link
Member

Thanks to your effort on this rule, take your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-codegen Area - Code Generation A-linter Area - Linter A-minifier Area - Minifier A-parser Area - Parser A-prettier Area - Prettier A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants