From fcbdcbae2327f0335e75c9f756ff67d96c712f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20W=C3=B3dkiewicz?= Date: Tue, 6 Aug 2024 09:08:37 +0200 Subject: [PATCH] [Fix] `no-named-as-default`: Allow using an identifier if the export is both a named and a default export - add tests for #1594 --- CHANGELOG.md | 3 + src/rules/no-named-as-default.js | 63 ++++++++-- tests/files/no-named-as-default/exports.js | 4 + .../misleading-re-exports.js | 2 + .../no-named-as-default/no-default-export.js | 1 + tests/files/no-named-as-default/re-exports.js | 2 + tests/files/no-named-as-default/something.js | 1 + tests/src/rules/no-named-as-default.js | 110 +++++++++++++++--- 8 files changed, 162 insertions(+), 24 deletions(-) create mode 100644 tests/files/no-named-as-default/exports.js create mode 100644 tests/files/no-named-as-default/misleading-re-exports.js create mode 100644 tests/files/no-named-as-default/no-default-export.js create mode 100644 tests/files/no-named-as-default/re-exports.js create mode 100644 tests/files/no-named-as-default/something.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 89c752b4e..4e7ef808e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange ### Fixed - `ExportMap` / flat config: include `languageOptions` in context ([#3052], thanks [@michaelfaith]) +- [`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export ([#3032], thanks [@akwodkiewicz]) ## [2.30.0] - 2024-09-02 @@ -1139,6 +1140,7 @@ for info on changes for earlier releases. [#3043]: https://github.com/import-js/eslint-plugin-import/pull/3043 [#3036]: https://github.com/import-js/eslint-plugin-import/pull/3036 [#3033]: https://github.com/import-js/eslint-plugin-import/pull/3033 +[#3032]: https://github.com/import-js/eslint-plugin-import/pull/3032 [#3018]: https://github.com/import-js/eslint-plugin-import/pull/3018 [#3012]: https://github.com/import-js/eslint-plugin-import/pull/3012 [#3011]: https://github.com/import-js/eslint-plugin-import/pull/3011 @@ -1726,6 +1728,7 @@ for info on changes for earlier releases. [@AdriAt360]: https://github.com/AdriAt360 [@ai]: https://github.com/ai [@aks-]: https://github.com/aks- +[@akwodkiewicz]: https://github.com/akwodkiewicz [@aladdin-add]: https://github.com/aladdin-add [@alex-page]: https://github.com/alex-page [@alexgorbatchev]: https://github.com/alexgorbatchev diff --git a/src/rules/no-named-as-default.js b/src/rules/no-named-as-default.js index c5adc7afe..dacd294f4 100644 --- a/src/rules/no-named-as-default.js +++ b/src/rules/no-named-as-default.js @@ -15,28 +15,71 @@ module.exports = { create(context) { function checkDefault(nameKey, defaultSpecifier) { + /** + * For ImportDefaultSpecifier we're interested in the "local" name (`foo` for `import {bar as foo} ...`) + * For ExportDefaultSpecifier we're interested in the "exported" name (`foo` for `export {bar as foo} ...`) + */ + const analyzedName = defaultSpecifier[nameKey].name; + // #566: default is a valid specifier - if (defaultSpecifier[nameKey].name === 'default') { return; } + if (analyzedName === 'default') { return; } const declaration = importDeclaration(context, defaultSpecifier); + /** @type {import('../exportMap').default | null} */ + const importedModule = ExportMapBuilder.get(declaration.source.value, context); + if (importedModule == null) { return; } - const imports = ExportMapBuilder.get(declaration.source.value, context); - if (imports == null) { return; } + if (importedModule.errors.length > 0) { + importedModule.reportErrors(context, declaration); + return; + } - if (imports.errors.length) { - imports.reportErrors(context, declaration); + if (!importedModule.hasDefault) { + // The rule is triggered for default imports/exports, so if the imported module has no default + // this means we're dealing with incorrect source code anyway return; } - if (imports.has('default') && imports.has(defaultSpecifier[nameKey].name)) { + if (!importedModule.has(analyzedName)) { + // The name used locally for the default import was not even used in the imported module. + return; + } - context.report( - defaultSpecifier, - `Using exported name '${defaultSpecifier[nameKey].name}' as identifier for default export.`, - ); + /** + * FIXME: We can verify if a default and a named export are pointing to the same symbol only + * if they are both `reexports`. In case one of the symbols is not a re-export, but defined + * in the file, the ExportMap structure has no info about what actually is being exported -- + * the value in the `namespace` Map is an empty object. + * + * To solve this, it would require not relying on the ExportMap, but on some other way of + * accessing the imported module and its exported values. + * + * Additionally, although `ExportMap.get` is a unified way to get info from both `reexports` + * and `namespace` maps, it does not return valid output we need here, and I think this is + * related to the "cycle safeguards" in the `get` function. + */ + if (importedModule.reexports.has(analyzedName) && importedModule.reexports.has('default')) { + const thingImportedWithNamedImport = importedModule.reexports.get(analyzedName).getImport(); + const thingImportedWithDefaultImport = importedModule.reexports.get('default').getImport(); + + // Case: both imports point to the same file and they both refer to the same symbol in this file. + if ( + thingImportedWithNamedImport.path === thingImportedWithDefaultImport.path + && thingImportedWithNamedImport.local === thingImportedWithDefaultImport.local + ) { + // #1594: the imported module exports the same thing via a default export and a named export + return; + } } + + context.report( + defaultSpecifier, + `Using exported name '${defaultSpecifier[nameKey].name}' as identifier for default ${nameKey === 'local' ? `import` : `export`}.`, + ); + } + return { ImportDefaultSpecifier: checkDefault.bind(null, 'local'), ExportDefaultSpecifier: checkDefault.bind(null, 'exported'), diff --git a/tests/files/no-named-as-default/exports.js b/tests/files/no-named-as-default/exports.js new file mode 100644 index 000000000..62402634f --- /dev/null +++ b/tests/files/no-named-as-default/exports.js @@ -0,0 +1,4 @@ +const variable = 1; + +export { variable }; +export default variable; diff --git a/tests/files/no-named-as-default/misleading-re-exports.js b/tests/files/no-named-as-default/misleading-re-exports.js new file mode 100644 index 000000000..8d36a0866 --- /dev/null +++ b/tests/files/no-named-as-default/misleading-re-exports.js @@ -0,0 +1,2 @@ +export { variable as something } from './exports'; +export { something as default } from './something'; diff --git a/tests/files/no-named-as-default/no-default-export.js b/tests/files/no-named-as-default/no-default-export.js new file mode 100644 index 000000000..db3074797 --- /dev/null +++ b/tests/files/no-named-as-default/no-default-export.js @@ -0,0 +1 @@ +export const foobar = 4; diff --git a/tests/files/no-named-as-default/re-exports.js b/tests/files/no-named-as-default/re-exports.js new file mode 100644 index 000000000..20306c182 --- /dev/null +++ b/tests/files/no-named-as-default/re-exports.js @@ -0,0 +1,2 @@ +export { something as default } from "./something"; +export { something } from "./something"; diff --git a/tests/files/no-named-as-default/something.js b/tests/files/no-named-as-default/something.js new file mode 100644 index 000000000..d8fd6851b --- /dev/null +++ b/tests/files/no-named-as-default/something.js @@ -0,0 +1 @@ +export const something = 42; diff --git a/tests/src/rules/no-named-as-default.js b/tests/src/rules/no-named-as-default.js index c6646a4f0..7fcd6e4f7 100644 --- a/tests/src/rules/no-named-as-default.js +++ b/tests/src/rules/no-named-as-default.js @@ -12,14 +12,20 @@ ruleTester.run('no-named-as-default', rule, { test({ code: 'import bar, { foo } from "./empty-folder";' }), // es7 - test({ code: 'export bar, { foo } from "./bar";', - parser: parsers.BABEL_OLD }), - test({ code: 'export bar from "./bar";', - parser: parsers.BABEL_OLD }), + test({ + code: 'export bar, { foo } from "./bar";', + parser: parsers.BABEL_OLD, + }), + test({ + code: 'export bar from "./bar";', + parser: parsers.BABEL_OLD, + }), // #566: don't false-positive on `default` itself - test({ code: 'export default from "./bar";', - parser: parsers.BABEL_OLD }), + test({ + code: 'export default from "./bar";', + parser: parsers.BABEL_OLD, + }), // es2022: Arbitrary module namespae identifier names testVersion('>= 8.7', () => ({ @@ -27,6 +33,48 @@ ruleTester.run('no-named-as-default', rule, { parserOptions: { ecmaVersion: 2022 }, })), + // #1594: Allow importing as default if object is exported both as default and named + test({ code: 'import something from "./no-named-as-default/re-exports.js";' }), + test({ + code: 'import { something } from "./no-named-as-default/re-exports.js";', + }), + test({ + code: 'import myOwnNameForVariable from "./no-named-as-default/exports.js";', + }), + test({ + code: 'import { variable } from "./no-named-as-default/exports.js";', + }), + test({ + code: 'import variable from "./no-named-as-default/misleading-re-exports.js";', + }), + test({ + // incorrect import + code: 'import foobar from "./no-named-as-default/no-default-export.js";', + }), + // same tests, but for exports + test({ + code: 'export something from "./no-named-as-default/re-exports.js";', + parser: parsers.BABEL_OLD, + }), + test({ + code: 'export { something } from "./no-named-as-default/re-exports.js";', + }), + test({ + code: 'export myOwnNameForVariable from "./no-named-as-default/exports.js";', + parser: parsers.BABEL_OLD, + }), + test({ + code: 'export { variable } from "./no-named-as-default/exports.js";', + }), + test({ + code: 'export variable from "./no-named-as-default/misleading-re-exports.js";', + parser: parsers.BABEL_OLD, + }), + test({ + code: 'export foobar from "./no-named-as-default/no-default-export.js";', + parser: parsers.BABEL_OLD, + }), + ...SYNTAX_CASES, ), @@ -34,13 +82,17 @@ ruleTester.run('no-named-as-default', rule, { test({ code: 'import foo from "./bar";', errors: [{ - message: 'Using exported name \'foo\' as identifier for default export.', - type: 'ImportDefaultSpecifier' }] }), + message: 'Using exported name \'foo\' as identifier for default import.', + type: 'ImportDefaultSpecifier', + }], + }), test({ code: 'import foo, { foo as bar } from "./bar";', errors: [{ - message: 'Using exported name \'foo\' as identifier for default export.', - type: 'ImportDefaultSpecifier' }] }), + message: 'Using exported name \'foo\' as identifier for default import.', + type: 'ImportDefaultSpecifier', + }], + }), // es7 test({ @@ -48,13 +100,17 @@ ruleTester.run('no-named-as-default', rule, { parser: parsers.BABEL_OLD, errors: [{ message: 'Using exported name \'foo\' as identifier for default export.', - type: 'ExportDefaultSpecifier' }] }), + type: 'ExportDefaultSpecifier', + }], + }), test({ code: 'export foo, { foo as bar } from "./bar";', parser: parsers.BABEL_OLD, errors: [{ message: 'Using exported name \'foo\' as identifier for default export.', - type: 'ExportDefaultSpecifier' }] }), + type: 'ExportDefaultSpecifier', + }], + }), test({ code: 'import foo from "./malformed.js"', @@ -68,7 +124,7 @@ ruleTester.run('no-named-as-default', rule, { testVersion('>= 8.7', () => ({ code: 'import foo from "./export-default-string-and-named"', errors: [{ - message: 'Using exported name \'foo\' as identifier for default export.', + message: 'Using exported name \'foo\' as identifier for default import.', type: 'ImportDefaultSpecifier', }], parserOptions: { ecmaVersion: 2022 }, @@ -76,10 +132,36 @@ ruleTester.run('no-named-as-default', rule, { testVersion('>= 8.7', () => ({ code: 'import foo, { foo as bar } from "./export-default-string-and-named"', errors: [{ - message: 'Using exported name \'foo\' as identifier for default export.', + message: 'Using exported name \'foo\' as identifier for default import.', type: 'ImportDefaultSpecifier', }], parserOptions: { ecmaVersion: 2022 }, })), + + // #1594: Allow importing as default if object is exported both as default and named + test({ + code: 'import something from "./no-named-as-default/misleading-re-exports.js";', + parser: parsers.BABEL_OLD, + errors: [{ + message: 'Using exported name \'something\' as identifier for default import.', + type: 'ImportDefaultSpecifier', + }], + }), + // The only cases that are not covered by the fix + test({ + code: 'import variable from "./no-named-as-default/exports.js";', + errors: [{ + message: 'Using exported name \'variable\' as identifier for default import.', + type: 'ImportDefaultSpecifier', + }], + }), + test({ + code: 'export variable from "./no-named-as-default/exports.js";', + parser: parsers.BABEL_OLD, + errors: [{ + message: 'Using exported name \'variable\' as identifier for default export.', + type: 'ExportDefaultSpecifier', + }], + }), ), });