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

linter: false positive for FunctionExpressions in unicorn/consistent-function-scoping #5365

Open
DonIsaac opened this issue Aug 31, 2024 · 5 comments
Assignees
Labels
A-linter Area - Linter C-bug Category - Bug

Comments

@DonIsaac
Copy link
Collaborator

This currently passes, but it should not:

function outer() {
  const inner = function inner() {}
}

See this thread for details.

@DonIsaac DonIsaac added C-bug Category - Bug A-linter Area - Linter labels Aug 31, 2024
@camc314
Copy link
Collaborator

camc314 commented Aug 31, 2024

wait i'm confused 😅

to be clear, the correct behavour here is that the following code:

function outer() {
  const inner = function inner() {}
}

should report an error (inner can be moved to the outer scope)

but with oxlint's current behaviour, it does not report an error?


if so, isn't this fixed by #5312, with the new test case here ? https://github.com/oxc-project/oxc/pull/5312/files#diff-8becea68b83c5a44bd998989f16fb08f3fc78f8ccfbbd95a30cc3056a6cf0edc

@DonIsaac
Copy link
Collaborator Author

image

@camc314
Copy link
Collaborator

camc314 commented Aug 31, 2024

ahhh i see - thanks

In summary:

we do not report an error for the following code:

let inner;

function outer() {
  inner = function inner() {}
}

But we should as there's no reason why the assignment couldn't be removed to the outer scope.

NOTE: when/if we implement this, we should add the following as a PASS test case:

let inner;

function foo1() {
    inner = function {}
}
function foo2() {
    inner = function {}
}

I can't think of why someone would want to do this (maybe a different logger in dev vs prod or something?), but i don't think we should report this case

@Arian94
Copy link
Contributor

Arian94 commented Sep 1, 2024

@camc314 would work on this.

@Arian94
Copy link
Contributor

Arian94 commented Sep 1, 2024

@camc314 would work on this.

@camc314 I mean myself 🍡. Assign it to me if you'd want.

DonIsaac added a commit that referenced this issue Sep 16, 2024
…orn/consistent-function-scoping``` (#5675)

This is to fix the cases mentioned in the comment section of #5365.

In short, it will report these as PASS test cases:

```js
let inner;

function foo1() {
  inner = function() {}
}
function foo2() {
  inner = function() {}
}
```

```js
let inner;

function outer() {
  inner = function() {}
}
```

And report these below as FAIL test cases:

```js
let inner;

function foo1() {
  inner = function smth() {}
}
function foo2() {
  inner = function bar() {}
}
```

```js
let inner;

function outer() {
  inner = function inner() {}
}
```

The case below was already done in #5312 but mentioned in #5365. It is a
FAIL case as well:
```js
function outer() {
  const inner = function inner() {}
}
```

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Don Isaac <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

3 participants