Skip to content

Bug: for (using foo in {}); should be invalid #7555

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.

Sign up for GitHub

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

Jump to bottom
Closed
4 tasks done
fisker opened this issue Aug 29, 2023 · 4 comments · Fixed by #7649
Closed
4 tasks done

Bug: for (using foo in {}); should be invalid #7555

fisker opened this issue Aug 29, 2023 · 4 comments · Fixed by #7649
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@fisker
Copy link
Contributor

fisker commented Aug 29, 2023 •
edited
Loading

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Relevant Package

parser

Playground Link

https://typescript-eslint.io/play/#ts=5.2.0-beta&showAST=es&fileType=.tsx&code=GYewTgBAFArgzgSwHYHMKhBZEDeBfASgG4g&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

for (using foo in {});

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/<rule-name>": ["error", ...<options>],
  },
};

tsconfig

{
  "compilerOptions": {
    // ...
  }
}

Expected Result

Error thrown.

Actual Result

Successfuly parsed.

Additional Info

No response

Versions

package version
@typescript-eslint/parser 5.6.0
@fisker fisker added bug Something isn't working triage Waiting for team members to take a look labels Aug 29, 2023
@fisker fisker mentioned this issue Aug 29, 2023
Merged
4 tasks
@bradzacher bradzacher added package: typescript-estree Issues related to @typescript-eslint/typescript-estree AST PRs and Issues about the AST structure accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Aug 29, 2023
@Solo-steven
Copy link
Contributor

HI @bradzacher I am interested in solving this bug, I find the result of this bug is because some default setting prevent parseAndGenerateServices return a program instance.

first, for(using foo in {}); is a semantic error, not a syntax error, so we need a program instance to emit it.

typescript-eslint/packages/typescript-estree/src/parser.ts

Line 270 in d2973cc

if (program && parseSettings.errorOnTypeScriptSyntacticAndSemanticIssues) {

but this ternary operation does not return a program instance, because hasFullTypeInformation is false, which seems to be the default setting.

typescript-eslint/packages/typescript-estree/src/parser.ts

Line 244 in d2973cc

const { ast, program } =

This bug can be solved by always returning a program instance. but I think there must be some reason that makes hasFullTypeInformation's default false. so I would like to hear why hasFullTypeInformation is defaulted to false, thanks ~

@fisker
Copy link
Contributor Author

fisker commented Sep 5, 2023 •
edited
Loading

The check can be added here.

typescript-eslint/packages/typescript-estree/src/convert.ts

Lines 920 to 926 in d2973cc

case SyntaxKind.ForInStatement:
return this.createNode<TSESTree.ForInStatement>(node, {
type: AST_NODE_TYPES.ForInStatement,
left: this.convertPattern(node.initializer),
right: this.convertChild(node.expression),
body: this.convertChild(node.statement),
});

@Solo-steven
Copy link
Contributor

HI @fisker thanks for your reply, but is the Typescript Program instance already doing the semantic diagnosis for us, why not just use but manual check in the coverter?

@bradzacher
Copy link
Member

bradzacher commented Sep 5, 2023 •
edited
Loading

It costs around 50ms per file to calculate diagnostics. This adds up quickly across any non-trivial codebase and so we purposely avoid it - 100 files = 5s!! Just to calculate diagnostics that likely aren't even there most of the time!

Instead we implement our own restrictions within our AST conversion as we can do it very quickly during the traversal - we have to inspect and disect the AST any way to convert it so it's easy and natural for us to enforce certain invariants during this process.

@Solo-steven Solo-steven mentioned this issue Sep 15, 2023
Merged
3 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Milestone
No milestone
3 participants
@fisker @bradzacher @Solo-steven