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

bug(semantic): AstNode has incorrect ScopeId #4436

Closed
DonIsaac opened this issue Jul 23, 2024 · 2 comments
Closed

bug(semantic): AstNode has incorrect ScopeId #4436

DonIsaac opened this issue Jul 23, 2024 · 2 comments
Assignees
Labels
A-semantic Area - Semantic C-bug Category - Bug

Comments

@DonIsaac
Copy link
Contributor

playground link

function foo(cb) { cb = function() { function something(a) { cb(1 + a); } register(something); }(); } foo();
//                                                           ^^^^^^^^^ 

To reproduce:

  1. get The first read reference for cb (it has a node whose Id is for the above CallExpression)
  2. Get the ScopeId pointed to by that Reference
  3. Get the node associated with that ScopeId
  4. Notice that it's the anonymous function wrapping something, when it should be the something function
@DonIsaac DonIsaac added C-bug Category - Bug A-semantic Area - Semantic labels Jul 23, 2024
@overlookmotel
Copy link
Contributor

Excellent bug-hunting!

I suspect this may have been caused by #4347 (but that's just a guess).

@Dunqing Dunqing self-assigned this Jul 24, 2024
@Dunqing
Copy link
Member

Dunqing commented Jul 24, 2024

  1. Get the ScopeId pointed to by that Reference

The reference doesn't store ScopeId. If you mean the ScopeId of the symbol pointed to by this reference, then the node looks correct.

The following scope snapshot was generated by your provided playground's code

---
source: crates/oxc_semantic/tests/main.rs
assertion_line: 112
input_file: crates/oxc_semantic/tests/fixtures/oxc/index.ts
---
[
  {
    "children": [
      {
        "children": [
          {
            "children": [
              {
                "children": [],
                "flag": "ScopeFlags(StrictMode | Function)",
                "id": 3,
                "node": "Function(something)",
                "symbols": [
                  {
                    "flag": "SymbolFlags(FunctionScopedVariable)",
                    "id": 3,
                    "name": "a",
                    "node": "FormalParameter",
                    "references": [
                      {
                        "flag": "ReferenceFlag(Read)",
                        "id": 0,
                        "name": "a",
                        "node_id": 27
                      }
                    ]
                  }
                ]
              }
            ],
            "flag": "ScopeFlags(StrictMode | Function)",
            "id": 2,
            "node": "Function(<anonymous>)",
            "symbols": [
              {
                "flag": "SymbolFlags(FunctionScopedVariable)",
                "id": 2,
                "name": "something",
                "node": "Function(something)",
                "references": [
                  {
                    "flag": "ReferenceFlag(Read)",
                    "id": 0,
                    "name": "something",
                    "node_id": 32
                  }
                ]
              }
            ]
          }
        ],
        "flag": "ScopeFlags(StrictMode | Function)",
        "id": 1,
        "node": "Function(foo)",
        "symbols": [
          {
            "flag": "SymbolFlags(FunctionScopedVariable)",
            "id": 1,
            "name": "cb",
            "node": "FormalParameter",
            "references": [
              {
                "flag": "ReferenceFlag(Write)",
                "id": 0,
                "name": "cb",
                "node_id": 11
              },
              {
                "flag": "ReferenceFlag(Read)",
                "id": 1,
                "name": "cb",
                "node_id": 28
              }
            ]
          }
        ]
      }
    ],
    "flag": "ScopeFlags(StrictMode | Top)",
    "id": 0,
    "node": "Program",
    "symbols": [
      {
        "flag": "SymbolFlags(BlockScopedVariable | Function)",
        "id": 0,
        "name": "foo",
        "node": "Function(foo)",
        "references": [
          {
            "flag": "ReferenceFlag(Read)",
            "id": 0,
            "name": "foo",
            "node_id": 36
          }
        ]
      }
    ]
  }
]

@DonIsaac DonIsaac closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2024
Dunqing pushed a commit that referenced this issue Jul 31, 2024
> Re-creation of #4427 due to rebasing issues. Original attempt: #642
-----

Third time's the charm?

Each time I attempt this rule, I find a bunch of bugs in `Semantic`, and I expect this attempt to be no different. Expect sidecar issues+PRs stemming from this PR here.

## Not Supported
These are cases supported in the original eslint rule, but that I'm intentionally deciding not to support
- export comments in scripts
  ```js
  /* exported a */ var a;
  ```
- global comments
  ```js
  /* global a */ var a;
   ```

## Behavior Changes
These are intentional deviations from the original rule's behavior:
- logical re-assignments are not considered usages
  ```js
  // passes in eslint/no-unused-vars, fails in this implementation
  let a = 0; a ||= 1;
  let b = 0; b &&= 2;
  let c = undefined; c ??= []
  ```

## Known Limitations
- Lint rules do not have babel or tsconfig information, meaning we can't determine if `React` imports are being used or not. The relevant tsconfig settings here are `jsx`, `jsxPragma`, and `jsxFragmentName`. To accommodate this, all imports to symbols named `React` or `h` are ignored in JSX files.
- References to symbols used in JSDoc `{@link}` tags are not created, so symbols that are only used in doc comments will be reported as unused. See: #4443
- `.vue` files are skipped completely, since variables can be used in templates in ways we cannot detect
  > note: `.d.ts` files are skipped as well.

## Todo
- [x] Skip unused TS enum members on used enums
- [x] Skip unused parameters followed by used variables in object/array spreads
- [x] Re-assignments to array/object spreads do not respect `destructuredArrayIgnorePattern` (related to: #4435)
- [x] #4493
- [x] References inside a nested scope are not considered usages (#4447)
- [x] Port over typescript-eslint test cases _(wip, they've been copied and I'm slowly enabling them)_
- [x] Handle constructor properties
  ```ts
  class Foo {
    constructor(public a) {} // `a` should be allowed
  }
  ```
- [x] Read references in sequence expressions (that are not in the last position) should not count as a usage
  ```js
  let a = 0; let b = (a++, 0); console.log(b)
  ```
  > Honestly, is anyone even writing code like this?
- [x] function overload signatures should not be reported
- [x] Named functions returned from other functions get incorrectly reported as unused (found by @camc314)
  ```js
  function foo() {
    return function bar() { }
  }
  Foo()()
  ```
- [x] false positive for TS modules within ambient modules
  ```ts
  declare global {
    // incorrectly marked as unused
    namespace jest { }
  }
  ```

## Blockers
- #4436
- #4437
- #4446
- #4447
- #4494
- #4495

## Non-Blocking Issues
- #4443
- #4475 (prevents checks on exported symbols from namespaces)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

3 participants