-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
fix: references of Object.assign in prefer-object-spread
#18148
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
var foo = Object.assign; | ||
var bar = foo({}, baz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be reported because foo
has never been reassigned.
Incorrect
/*eslint prefer-object-spread: "error" */
let doSomething;
doSomething = Object.assign;
var bar = doSomething({}, a, b); // Error
Correct
/*eslint prefer-object-spread: "error" */
let doSomething;
if (foo) {
doSomething = Object.assign;
} else {
doSomething = somethingElse
}
var bar = doSomething({}, a, b); // OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like i have understood the issue wrong but what about autofixes is it also correct
/*eslint prefer-object-spread: "error" */
let doSomething;
doSomething = Object.assign;
var bar = { ...a, ...b};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe the auto fix is also correct, doSomething
will be reported by the no-unused-vars
rule.
@mdjermanovic Can you once confirm the expected behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that ReferenceTracker
returns both certain references (e.g., const x = Object.assign; x();
) and potential references (those that are references only in some code paths, e.g., const x = foo ? Object.assign : bar; x();
), which is okay for some rules (e.g., for no-obj-calls
, because it's a bug if JSON()
is called in any code paths) but not for this one.
We currently don't have a utility that would help find only certain references. The solution could be:
- Add an option to
ReferenceTracker
to not return potential references. - Make another utility.
- Limit this rule to check only explicit
Object.assign
calls. Maybe also<global object>.Object.assign
(likeno-eval
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, changes in this PR falls in 3rd point so what should we do now, move forward or look for another approaches (point 1st and 2nd)?
the reason i chose 3rd one is i noticed in this rule's previous version is that references was used so that it allows the Object.assign
if Object
is importing from a module, and i didn't find any tests which doesn't call Object.assign
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still doubtful that this fix is correct and doesn't lead to bugs.
/*eslint prefer-object-spread: "error" */
let doSomething;
doSomething = Object.assign;
var bar = { ...a, ...b};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could first ask at https://github.com/eslint-community/eslint-utils if option 1 would be possible. If not, then I think option 3 is fine.
…#16196) * feat: [no-restricted-imports] added option `excludeImportNames` * Fix: Removed question marks from checks in `no-restricted-imports` * Fix: [no-restricted-imports] `excludeImportNames` * docs: Update no-restricted-imports.md with new option Documentation added for option `excludeImportNames`. * docs: Update no-restricted-imports option exludeImportNames to allowImportNames * fix: Renamed `no-restricted-imports` option `excludeImportNames` to `allowImportNames` * fix: `no-restricted-imports` comments for options `allowImportNames` to pass test * fix: `no-restricted-imports` option `allowImportNames` for importing `*` * fix: `no-restricted-imports` rules added for option `allowImportNames` * fix: `no-restricted-imports` tests added for option `allowImportNames` * Lint * Lint Fix * Check Rules * fix: `no-restricted-imports` * fix: `no-restricted-imports`: Add tests for allowImportNamePattern * docs: `no-restricted-imports` updated * feat: refactor no-restricted-imports and add tests * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * fix: `no-restricted-imports`: requested changes * docs: `no-restricted-imports`: added `allowImportNames` to `patterns` * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * docs: `no-restricted-imports` message variation * feat: validate schema * docs: `no-restricted-imports`: disallow using both `importNames` and `allowImportNames` * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * fix: `no-restricted-imports`: Review suggestions * feat: add more validations to schema * docs: add validate options name * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * fix: `no-restricted-imports`: tests updated accordingly * feat: add return * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update lib/rules/no-restricted-imports.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * feat: `no-restricted-imports`: added custom message to tests * fix: `no-restricted-imports`: remove test case * fix: `no-restricted-imports` * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> * Update docs/src/rules/no-restricted-imports.md Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> --------- Co-authored-by: Tanuj Kanti <tanujkanti4441@gmail.com> Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com> Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
docs: add inline cases condition
…slint#18152) Both, optional chaining expressions and default values (in function parameters or descructuring expressions) increase the branching of the code and thus the complexity is increased. Fixes: eslint#18060
* fix: improve error message for invalid rule config * refactor: update indentation & use more strict checks
…#18157) * feat!: disallow multiple configuration comments for same rule Fixes eslint#18132 * update lint error message Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * update tests Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
eslint#18082) * feat: report granular errors on arbitrary literals * use npm dependency * test with unescaped CRLF * inline `createReportLocationGenerator` * unit test for templates with expressions * restore old name `getNodeReportLocations` * update JSDoc * `charInfos` → `codeUnits` * extract char-source to a utility module * add `read` method to `SourceReader` * add `advance` method and JSDoc * fix logic * `SourceReader` → `TextReader` * handle `RegExp` calls with regex patterns * fix for browser test * fix for Node.js 18 * limit applicability of `getStaticValue` for Node.js 18 compatibility * fix for `RegExp()` without arguments * update JSDoc for `getStaticValueOrRegex`
* docs: Update release documentation * Fix lint error
* docs: show red underlines in examples in rules docs * fix: add postinstall to install eslint dependencies * fix: after-tokenize hook * fix: after-tokenize script * fix: after-tokenize hook * fix: after-tokenize hook * fix: comment * fix: change to verify using the custom container parserOptions * fix: refactor it so it doesn't affect tests * fix bug for no-multiple-empty-lines * fix: bug for eol-last * feat: use wavy line * feat: refactor * fix: refactor * feat: change to add message to title attribute * chore: revert scripts and change docs-ci * fix: prism-eslint-hook.js * fix: for unicode-bom * fix: disabled mark process if no linter is available * Update docs/tools/prism-eslint-hook.js Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * chore: add fileoverview * chore: add netlify.toml * fix: for unicode-bom with always * feat: improved styles for zero width markers. * chore: fix format * Update syntax-highlighter.scss * fix: issue with consecutive line breaks * fix: hides marker on newline following the marker --------- Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
…eslint#18170) * feat: add option to ignore SIB-classes * add docs and tests * add test * update docs
* docs: Explain Node.js version support fixes eslint#11022 * Update README.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* docs: Explain limitations of RuleTester fix testing fixes eslint#18007 * Update docs/src/integrate/nodejs-api.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update docs/src/integrate/nodejs-api.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* chore: add Knip + config * chore: use named exports consistently (only shorthands) * chore: update Knip & reduce config (by improved 11ty plugin) * chore: rename `lint:knip` → `lint:imports` Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * chore: upgrade Knip to v4.3.0 * chore: fix createCoreRuleConfigs import in test * chore: annotate default export for runtime-info (to satisfy both proxyquire and knip) * chore: update knip.jsonc Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * chore: add comment to export/import hint * chore: upgrade Knip to v5.0.1 (no breaking changes) * chore: remove unused files, dependencies & exports * chore: fix whitespace in ci.yml * chore: add Knip to CI job * Remove tools/update-rule-types.js from knip.jsonc Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Add corrected shared/types Rule import to lazy-loading-rule-map.js * Added back eslint-plugin-* devDependencies * Rename to lint:unused * Fix introduced prism-eslint-hook complaints --------- Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* fix: use `@eslint/config-inspector@latest` like @eslint/create-config (eslint#18369), it may use an outdated version, this commit forces to use the latest version. fixes eslint#18481 * Update docs/src/use/command-line-interface.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* docs: clarify func-style Fixes eslint#18474 * Update docs/src/rules/func-style.md Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> --------- Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* docs: refactor prefer-destructuring rule * correct options description * update option description * update wording
* feat: ignore IIFE's in the `no-loop-func` rule * refactor: remove false negatives * docs: add note about IIFE * fix: correct hasUnsafeRefsInNonIIFE logic * fix: handle more cases * chore: fix lint issues * fix: report only unsafe ref names in IIFEs * fix: handle more cases * chore: update comments * refactor: remove unwanted code * refactor: remove unwanted code * refactor: update code * fix: avoid duplicate reports * chore: refactor code to avoid memory leak * chore: apply suggestions from code review Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * test: add location --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* docs: update components files * update * update
* chore: switch to `@eslint/config-array` * remove redundant check
…8510) docs: update theme when when prefers-color-scheme changes
* ci: pin @wdio/browser-runner v8.36.0 the browser testing was failing: https://github.com/eslint/eslint/actions/runs/9324019940/job/25668389482. the commit aims to fix it for now. * fix: alias node modules
ci failing has been fixed in the main branch, can you please rebase it, thanks! |
…slint into limit-reference
|
Sorry! i just tried something and this happened, i'll open a new one. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What rule do you want to change?
prefer-object-spread
What change do you want to make (place an "X" next to just one item)?
[ ] Generate more warnings
[x] Generate fewer warnings
[ ] Implement autofix
[ ] Implement suggestions
How will the change be implemented (place an "X" next to just one item)?
[ ] A new option
[ ] A new default behavior
[x] Other
Please provide some example code that this change will affect:
What does the rule currently do for this code?
Rule reports and fixes the
doSomething({}, a, b)
as it's a reference of Object.assignWhat will the rule do after it's changed?
Rule will not report
doSomething({}, a, b)
and only report theObject.assign({}, a)
andGlobalThis.Object.assign({}, a)
Is there anything you'd like reviewers to focus on?
Fixes #12826