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

enhancement(linter) expand only-used-in-recursion to support jsx #5530

Closed
camc314 opened this issue Sep 6, 2024 · 5 comments · Fixed by #7120
Closed

enhancement(linter) expand only-used-in-recursion to support jsx #5530

camc314 opened this issue Sep 6, 2024 · 5 comments · Fixed by #7120
Assignees
Labels
A-linter Area - Linter C-bug Category - Bug good first issue Experience Level - Good for newcomers

Comments

@camc314
Copy link
Contributor

camc314 commented Sep 6, 2024

currently, only-used-in-recursion only supports cases such as:

function foo(arg0) {
    foo(arg0)
}

it could be expanded to support cases such as:

function Listitem({ depth }) {
    return <Listitem depth={depth + 1} />
}

cc @no-yan

@camc314 camc314 added the C-bug Category - Bug label Sep 6, 2024
@camc314 camc314 self-assigned this Sep 6, 2024
@DonIsaac DonIsaac added the A-linter Area - Linter label Sep 12, 2024
@DonIsaac DonIsaac added the good first issue Experience Level - Good for newcomers label Sep 22, 2024
Boshen pushed a commit that referenced this issue Oct 14, 2024
… creation (#6513)

### Overview
This PR refactors `only-used-in-recursion` codebase to make the
implementation of #5530 easier.

The diff isn't displaying cleanly, so it might be better to review it
commit by commit when looking at the changes.

### Key changes
1. Extracted diagnostic logic into `craete_diagnostic` function:
3bf0015
2. Removed redundant check in `is_function_maybe_reassigned`:
a133ec6
3. Simplified `is_argument_only_used_in_recursion` by removing nesting:
6e6bd04
@camc314 camc314 removed their assignment Oct 14, 2024
@camc314
Copy link
Contributor Author

camc314 commented Oct 14, 2024

i can't seem to assign this. But @no-yan is going to work on it!

@camc314 camc314 assigned camc314 and unassigned camc314 Oct 14, 2024
@Boshen
Copy link
Member

Boshen commented Oct 14, 2024

i can't seem to assign this. But @no-yan is going to work on it!

You can only assign an outside contributor when they make a comment here.

@no-yan
Copy link
Contributor

no-yan commented Oct 14, 2024

It would have been better to comment here. Thanks for the reminder!

@camc314

  1. Why does the only-used-in-recursion rule only apply fixes when the argument is at the end of the function? Is it to avoid issues with residual arguments?
  2. The only edge case I can think of is when using the spread operator in JSX attributes. Do you have any other potential edge cases in mind?

For example, in this case, the spread operator might alter the program’s behavior, which is why no fix is applied:

function Test(props) { // could be {key: 0}
    return (
        <Test key={2} {...props} />
    );
}

But in most cases, like the one below, removing an argument seems safe in JSX:

- <Item innerItem={item} width={width} />
+ <Item width={width} />

@camc314
Copy link
Contributor Author

camc314 commented Oct 14, 2024

Why does the only-used-in-recursion rule only apply fixes when the argument is at the end of the function? Is it to avoid issues with residual arguments?
i think i did it this way because if you remove args that are not the last one, you've got to also find everywhere the fn is used and remove those args also to retain the same behaviour. Feel to skip a fixer for this one
The only edge case I can think of is when using the spread operator in JSX attributes. Do you have any other potential edge cases in mind?

i think reporting the spread should be fine. i can't think of any other edges.

function Test(props) { // could be {key: 0}
    return (
        <Test key={2} {...props} />
    );
}

reporting here should be fine right? i don't think key: 0 can ever be in the props

From the react docs

Note that your components won’t receive key as a prop. It’s only used as a hint by React itself. If your component needs an ID, you have to pass it as a separate prop: .

@no-yan
Copy link
Contributor

no-yan commented Oct 16, 2024

Thanks for the clarification!

reporting here should be fine right? i don't think key: 0 can ever be in the props

(This might be off-topic)
You're right that it doesn't end up in the props, but it does end up in the key.
The jsx implementation prioritizes element.props.key (e.g. <Component {...{key:0}}) over element.key (e.g., key={2}).
So, if we remove this props, it could unintentionally cause the key to change.

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 good first issue Experience Level - Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants