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

JS-54 Add rule S7060 Modules should not import themselves #4810

Merged
merged 2 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/jsts/src/linter/quickfixes/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ export const quickFixRules = new Set([
'S1128',

// eslint-plugin-import
'S6859',
'S3863',
'S6859',
'S7060',
]);
1 change: 1 addition & 0 deletions packages/jsts/src/rules/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ SonarJS rules for ESLint to help developers produce [Clean Code](https://www.son
| [no-same-argument-assert](https://sonarsource.github.io/rspec/#/rspec/S5863/javascript) | Assertions should not be given twice the same argument | ✅ | | | | | |
| [no-same-line-conditional](https://sonarsource.github.io/rspec/#/rspec/S3972/javascript) | Conditionals should start on new lines | ✅ | | 🔧 | 💡 | | |
| [no-self-compare](https://sonarsource.github.io/rspec/#/rspec/S6679/javascript) | "Number.isNaN()" should be used to check for "NaN" value | ✅ | | 🔧 | 💡 | | |
| [no-self-import](https://sonarsource.github.io/rspec/#/rspec/S7060/javascript) | Module should not import itself | ✅ | | | 💡 | | |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| [no-self-import](https://sonarsource.github.io/rspec/#/rspec/S7060/javascript) | Module should not import itself || | | 💡 | | |
| [no-self-import](https://sonarsource.github.io/rspec/#/rspec/S7060/javascript) | Module should not import itself || | 🔧 | 💡 | | |

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually not - this rule doesn't provide the fix , it's a bug in metadata generation

| [no-small-switch](https://sonarsource.github.io/rspec/#/rspec/S1301/javascript) | "switch" statements should have at least 3 "case" clauses | ✅ | | | | | |
| [no-tab](https://sonarsource.github.io/rspec/#/rspec/S105/javascript) | Tabulation characters should not be used | | ✅ | | | | ❌ |
| [no-table-as-layout](https://sonarsource.github.io/rspec/#/rspec/S5257/javascript) | HTML "<table>" should not be used for layout purposes | ✅ | | | | | |
Expand Down
12 changes: 12 additions & 0 deletions packages/jsts/src/rules/S7060/cb.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

import f from './cb.fixture.js'; // Noncompliant [[qf1]]
// fix@qf1 {{Remove this import}}
// edit@qf1 [[sc=0;ec=32]] {{}}

import f from './cb.fixture'; // Noncompliant

const f = require('./cb.fixture.js'); // Noncompliant [[qf2]]
// fix@qf2 {{Remove this require}}
// edit@qf2 [[sc=0;ec=37]] {{}}


28 changes: 28 additions & 0 deletions packages/jsts/src/rules/S7060/cb.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2024 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
import { check } from '../../../tests/tools';
import { rule } from './';
import path from 'path';

const sonarId = path.basename(__dirname);

describe('Rule S7060', () => {
check(sonarId, rule, __dirname);
});
77 changes: 77 additions & 0 deletions packages/jsts/src/rules/S7060/decorator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2024 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
// https://sonarsource.github.io/rspec/#/rspec/S7060/javascript

import { Rule } from 'eslint';
import { generateMeta, interceptReport } from '../helpers';
import { meta } from './meta';
import * as estree from 'estree';

export function decorate(rule: Rule.RuleModule): Rule.RuleModule {
return interceptReport(
{
...rule,
meta: generateMeta(meta as Rule.RuleMetaData, {
...rule.meta!,
hasSuggestions: true,
}),
},
reportWithQuickFix,
);
}

function reportWithQuickFix(context: Rule.RuleContext, reportDescriptor: Rule.ReportDescriptor) {
if (!('node' in reportDescriptor)) {
return;
}
const { node } = reportDescriptor;
const suggest: Rule.SuggestionReportDescriptor[] = [];
if (node.type === 'ImportDeclaration') {
suggest.push({
desc: 'Remove this import',
fix: fixer => fixer.remove(node),
});
} else if (node.type === 'CallExpression') {
const variableDecl = findRequireVariableDeclaration(node);
if (variableDecl) {
suggest.push({
desc: 'Remove this require',
fix: fixer => fixer.remove(variableDecl),
});
}
}
context.report({
...reportDescriptor,
suggest,
});
}

function findRequireVariableDeclaration(node: estree.Node) {
const parent = getParent(getParent(node));
if (parent.type === 'VariableDeclaration') {
return parent;
}
return undefined;
}

function getParent(node: estree.Node): estree.Node {
// @ts-ignore
return node.parent as estree.Node;
}
24 changes: 24 additions & 0 deletions packages/jsts/src/rules/S7060/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2024 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

import { rules as importRules } from 'eslint-plugin-import';
import { decorate } from './decorator';

export const rule = decorate(importRules['no-self-import']);
34 changes: 34 additions & 0 deletions packages/jsts/src/rules/S7060/meta.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2024 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

// DO NOT EDIT! This file is autogenerated by "npm run generate-meta"

export const meta = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @victor-diez-sonarsource because I am not sure.

  • Since the rule provides a quickfix, should the rule metadata include the key-value fixable: 'code'?
  • Do we need to update the eslint-plugin README to include an entry for this new rule no-self-import? If yes, how?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Yes, it should. npm run generate-meta should put that field in the meta.ts file if the rspec contains the quickfix === 'covered' attribute. Link
  • Yes, that is part of also of the generate-meta script. You will see that it calls cd packages/jsts/src/rules && npm run eslint-docs, which updates the README file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

type: 'suggestion',
docs: {
description: 'Module should not import itself',
recommended: true,
url: 'https://sonarsource.github.io/rspec/#/rspec/S7060/javascript',
requiresTypeChecking: false,
},
fixable: 'code',
};

export const sonarKey = 'S7060';
2 changes: 2 additions & 0 deletions packages/jsts/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ import { rule as S4324 } from './S4324'; // no-return-type-any
import { rule as S5863 } from './S5863'; // no-same-argument-assert
import { rule as S3972 } from './S3972'; // no-same-line-conditional
import { rule as S6679 } from './S6679'; // no-self-compare
import { rule as S7060 } from './S7060'; // no-self-import
import { rule as S1607 } from './S1607'; // no-skipped-tests
import { rule as S1301 } from './S1301'; // no-small-switch
import { rule as S105 } from './S105'; // no-tab
Expand Down Expand Up @@ -785,6 +786,7 @@ const bridgeRules: { [key: string]: Rule.RuleModule } = {
S6958,
S6959,
S7059,
S7060,
S878: eslintRules['no-sequences'],
S881,
S888,
Expand Down
2 changes: 2 additions & 0 deletions packages/jsts/src/rules/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ import { rule as S2755 } from './S2755';
import { rule as S4817 } from './S4817';
import { rule as S6627 } from './S6627';
import { rule as S1607 } from './S1607';
import { rule as S7060 } from './S7060';
import type { Rule, Linter } from 'eslint';
import { rule as S7059 } from './S7059';

Expand Down Expand Up @@ -555,6 +556,7 @@ export const rules: Record<string, Rule.RuleModule> = {
'no-same-argument-assert': S5863,
'no-same-line-conditional': S3972,
'no-self-compare': S6679,
'no-self-import': S7060,
'no-skipped-test': S1607,
'no-small-switch': S1301,
'no-tab': S105,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ public static List<Class<? extends JavaScriptCheck>> getAllChecks() {
NoReturnTypeAnyCheck.class,
NoSameArgumentAssertCheck.class,
NoSelfCompareCheck.class,
NoSelfImportCheck.class,
NoSkippedTestsCheck.class,
NoSparseArraysCheck.class,
NoStaticElementInteractionsCheck.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2024 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.javascript.checks;

import org.sonar.check.Rule;
import org.sonar.plugins.javascript.api.Check;
import org.sonar.plugins.javascript.api.JavaScriptRule;
import org.sonar.plugins.javascript.api.TypeScriptRule;

@TypeScriptRule
@JavaScriptRule
@Rule(key = "S7060")
public class NoSelfImportCheck extends Check {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<h2>Why is this an issue?</h2>
<p>When a module imports itself it has no effect. This means that the import statement does nothing useful and serves no purpose. This can happen
during refactoring or when a developer mistakenly imports the module itself.</p>
<p>To fix the problem remove the self-import statement.</p>
<pre>
// file: foo.js
import foo from './foo'; // Noncompliant

const foo = require('./foo'); // Noncompliant
</pre>
<pre>
// file: index.js
import index from '.'; // Noncompliant

const index = require('.'); // Noncompliant
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> MDN web docs - <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules">Modules</a> </li>
<li> MDN web docs - <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import">import</a> </li>
<li> Node.js docs - <a href="https://nodejs.org/api/modules.html#requireid">Node.js require</a> </li>
</ul>

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"title": "Module should not import itself",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-7060",
"sqKey": "S7060",
"scope": "All",
"quickfix": "covered",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM",
"RELIABILITY": "MEDIUM"
},
"attribute": "MODULAR"
},
"compatibleLanguages": [
"JAVASCRIPT",
"TYPESCRIPT"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@
"S6957",
"S6958",
"S6959",
"S7059"
"S7059",
"S7060"
]
}