Skip to content

Commit

Permalink
ESLINTJS-49 Rule no-implicit-dependencies doesn't work (#4819)
Browse files Browse the repository at this point in the history
  • Loading branch information
ericmorand-sonarsource authored Sep 19, 2024
1 parent 9c65fbd commit 7dc3241
Show file tree
Hide file tree
Showing 15 changed files with 124 additions and 32 deletions.
7 changes: 6 additions & 1 deletion its/eslint8-plugin-sonarjs/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@ module.exports = [
languageOptions: { sourceType: 'commonjs' },
},
plugin.configs.recommended,
{ rules: { 'sonarjs/accessor-pairs': 'error' } },
{
rules: {
'sonarjs/accessor-pairs': 'error',
'sonarjs/no-implicit-dependencies': 'error',
},
},
];
7 changes: 6 additions & 1 deletion its/eslint8-plugin-sonarjs/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@ export default [
languageOptions: { sourceType: 'commonjs' },
},
plugin.configs.recommended,
{ rules: { 'sonarjs/accessor-pairs': 'error' } },
{
rules: {
'sonarjs/accessor-pairs': 'error',
'sonarjs/no-implicit-dependencies': 'error',
},
},
];
4 changes: 4 additions & 0 deletions its/eslint8-plugin-sonarjs/file.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// S4328
import * as Bar from 'bar';
import * as CrossSpawn from 'cross-spawn';

function S3776(foo) {
if (foo.bar) {
if (foo.baz) {
Expand Down
7 changes: 6 additions & 1 deletion its/eslint9-plugin-sonarjs/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@ module.exports = [
languageOptions: { sourceType: 'commonjs' },
},
plugin.configs.recommended,
{ rules: { 'sonarjs/accessor-pairs': 'error' } },
{
rules: {
'sonarjs/accessor-pairs': 'error',
'sonarjs/no-implicit-dependencies': 'error',
},
},
];
7 changes: 6 additions & 1 deletion its/eslint9-plugin-sonarjs/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@ export default [
languageOptions: { sourceType: 'commonjs' },
},
plugin.configs.recommended,
{ rules: { 'sonarjs/accessor-pairs': 'error' } },
{
rules: {
'sonarjs/accessor-pairs': 'error',
'sonarjs/no-implicit-dependencies': 'error',
},
},
];
2 changes: 1 addition & 1 deletion packages/bridge/src/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ if (parentPort) {

case 'on-init-linter': {
const { rules, environments, globals, linterId, baseDir, exclusions } = data;
initializeLinter(rules, environments, globals, linterId);
initializeLinter(rules, environments, globals, baseDir, linterId);
if (baseDir) {
loadPackageJsons(baseDir, exclusions);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export async function analyzeProject(input: ProjectAnalysisInput): Promise<Proje
}
const pendingFiles: Set<string> = new Set(inputFilenames);
const watchProgram = input.isSonarlint;
initializeLinter(rules, environments, globals);
initializeLinter(rules, environments, globals, baseDir);
loadTSConfigAndPackageJsonFiles(baseDir, exclusions);
const tsConfigs = getTSConfigsIterator(
inputFilenames,
Expand Down
4 changes: 3 additions & 1 deletion packages/jsts/src/linter/linters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,18 @@ const linters: Linters = {};
* @param inputRules the rules from the active quality profiles
* @param environments the JavaScript execution environments
* @param globals the global variables
* @param workingDirectory the working directory
* @param linterId key of the linter
*/
export function initializeLinter(
inputRules: RuleConfig[],
environments: string[] = [],
globals: string[] = [],
workingDirectory?: string,
linterId = 'default',
) {
debug(`Initializing linter "${linterId}" with ${inputRules.map(rule => rule.key)}`);
linters[linterId] = new LinterWrapper({ inputRules, environments, globals });
linters[linterId] = new LinterWrapper({ inputRules, environments, globals, workingDirectory });
}

/**
Expand Down
5 changes: 4 additions & 1 deletion packages/jsts/src/linter/wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export interface WrapperOptions {
globals?: string[];
ruleBundles?: string[];
customRules?: CustomRule[];
workingDirectory?: string;
}

/**
Expand Down Expand Up @@ -113,7 +114,9 @@ export class LinterWrapper {
* @param options the wrapper's options
*/
constructor(private readonly options: WrapperOptions = {}) {
this.linter = new Linter();
this.linter = new Linter({
cwd: options.workingDirectory,
});
loadBundles(this.linter, options.ruleBundles ?? defaultRuleBundles);
loadCustomRules(this.linter, options.customRules);
this.config = this.createConfig();
Expand Down
Empty file.
19 changes: 17 additions & 2 deletions packages/jsts/src/rules/S4328/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ import { Rule } from 'eslint';
import * as estree from 'estree';
import builtins from 'builtin-modules';
import * as ts from 'typescript';
import { generateMeta, getDependencies } from '../helpers';
import { generateMeta, getDependenciesFromPackageJson, toUnixPath } from '../helpers';
import { FromSchema } from 'json-schema-to-ts';
import { meta, schema } from './meta';
import { Minimatch } from 'minimatch';
import { getManifests } from '../helpers/package-json';
import fs from 'fs';

const messages = {
removeOrAddDependency: 'Either remove this import or add it as a dependency.',
Expand All @@ -35,8 +37,21 @@ const messages = {
export const rule: Rule.RuleModule = {
meta: generateMeta(meta as Rule.RuleMetaData, { messages, schema }),
create(context: Rule.RuleContext) {
// we need to find all the npm manifests from the directory of the analyzed file to the context working directory
const cwd = toUnixPath(context.cwd);
const fileName = toUnixPath(context.filename);
const manifests = getManifests(fileName, cwd, fs);
const dependencies: Set<string> = new Set();

manifests.forEach(manifest => {
const manifestDependencies = getDependenciesFromPackageJson(manifest);

manifestDependencies.forEach(dependency => {
dependencies.add(dependency);
});
});

const whitelist = (context.options as FromSchema<typeof schema>)[0]?.whitelist || [];
const dependencies = getDependencies(context.filename);
const program = context.sourceCode.parserServices?.program;
let options: ts.CompilerOptions, host: ts.ModuleResolutionHost;
if (program) {
Expand Down
19 changes: 8 additions & 11 deletions packages/jsts/src/rules/S4328/unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@
import { RuleTester } from 'eslint';
import { rule } from './';
import path from 'path';
import { clearPackageJsons, loadPackageJsons } from '../helpers';

//reset and search package.json files in rule dir
clearPackageJsons();
loadPackageJsons(__dirname, []);

const fixtures = path.join(__dirname, 'fixtures');
const filename = path.join(fixtures, 'package-json-project/file.js');
Expand Down Expand Up @@ -111,11 +106,6 @@ ruleTester.run('Dependencies should be explicit', rule, {
filename,
options,
},
{
code: `import "dependency";`,
filename: path.join(fixtures, 'bom-package-json-project/file.js'),
options,
},
{
code: `const fs = require("node:fs/promises");`,
filename,
Expand Down Expand Up @@ -186,7 +176,14 @@ ruleTester.run('Dependencies should be explicit', rule, {
},
{
code: `import "foo";`,
filename: '/',
filename: '/file.js',
options,
errors: 1,
},
{
name: 'with a non-parsable manifest',
code: `import "dependency";`,
filename: path.join(fixtures, 'bom-package-json-project/file.js'),
options,
errors: 1,
},
Expand Down
67 changes: 59 additions & 8 deletions packages/jsts/src/rules/helpers/package-json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
import path from 'path';
import { PackageJson } from 'type-fest';
import Path from 'path/posix';
import { type PackageJson } from 'type-fest';
import { searchFiles, File } from './find-files';
import { toUnixPath } from './files';
import { Minimatch } from 'minimatch';
Expand Down Expand Up @@ -79,7 +79,7 @@ export function setPackageJsons(db: Record<string, File<PackageJson>[]>) {
* @returns
*/
export function getDependencies(fileName: string) {
let dirname = path.posix.dirname(toUnixPath(fileName));
let dirname = Path.dirname(toUnixPath(fileName));
const cached = cache.get(dirname);
if (cached) {
return cached;
Expand All @@ -89,7 +89,7 @@ export function getDependencies(fileName: string) {
cache.set(dirname, result);

for (const packageJson of getNearestPackageJsons(fileName)) {
dirname = path.posix.dirname(packageJson.filename);
dirname = Path.dirname(packageJson.filename);
const dirCached = dirCache.get(dirname);
if (dirCached) {
dirCached.forEach(d => result.add(d));
Expand Down Expand Up @@ -117,18 +117,18 @@ export function getNearestPackageJsons(file: string) {
if (getPackageJsonsCount() === 0) {
return results;
}
let currentDir = path.posix.dirname(path.posix.normalize(toUnixPath(file)));
let currentDir = Path.dirname(Path.normalize(toUnixPath(file)));
do {
const packageJson = PackageJsonsByBaseDir[currentDir];
if (packageJson?.length) {
results.push(...packageJson);
}
currentDir = path.posix.dirname(currentDir);
} while (currentDir !== path.posix.dirname(currentDir));
currentDir = Path.dirname(currentDir);
} while (currentDir !== Path.dirname(currentDir));
return results;
}

function getDependenciesFromPackageJson(content: PackageJson) {
export function getDependenciesFromPackageJson(content: PackageJson) {
const result = new Set<string>();
if (content.name) {
addDependencies(result, { [content.name]: '*' });
Expand Down Expand Up @@ -183,3 +183,54 @@ function addDependency(result: Set<string | Minimatch>, dependency: string, isGl
);
}
}

/**
* Returns the project manifests that are used to resolve the dependencies imported by
* the module named `fileName`, up to the passed working directory.
*/
export const getManifests = (
fileName: string,
workingDirectory: string,
fileSystem: {
readdirSync: (path: string) => Array<string>;
readFileSync: (path: string) => Buffer;
},
): Array<PackageJson> => {
// if the fileName is not a child of the working directory, we bail
const relativePath = Path.relative(workingDirectory, fileName);

if (relativePath.startsWith('..')) {
return [];
}

const getManifestsAtPath = (path: string): Array<PackageJson> => {
const results: Array<PackageJson> = [];
const entries = fileSystem.readdirSync(path);

for (const entry of entries) {
const entryUnixPath = toUnixPath(entry);
const absoluteEntryPath = Path.join(path, entryUnixPath);

if (Path.basename(absoluteEntryPath) === 'package.json') {
const content = fileSystem.readFileSync(absoluteEntryPath);

try {
results.push(JSON.parse(content.toString()));
} catch (error) {
console.debug(`Error parsing file ${absoluteEntryPath}: ${error}`);
}
}
}

// we stop as soon as the working directory has been reached
if (path !== workingDirectory) {
const parentDirectory = Path.dirname(path);

results.push(...getManifestsAtPath(parentDirectory));
}

return results;
};

return getManifestsAtPath(Path.dirname(fileName));
};
4 changes: 2 additions & 2 deletions packages/jsts/tests/analysis/analyzer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('analyzeJSTS', () => {
it('should analyze JavaScript code with the given linter', async () => {
const rules = [{ key: 'S4524', configurations: [], fileTypeTarget: ['MAIN'] }] as RuleConfig[];
initializeLinter(rules);
initializeLinter([], [], [], 'empty');
initializeLinter([], [], [], undefined, 'empty');

const filePath = path.join(__dirname, 'fixtures', 'code.js');
const language = 'js';
Expand All @@ -78,7 +78,7 @@ describe('analyzeJSTS', () => {
it('should analyze TypeScript code with the given linter', async () => {
const rules = [{ key: 'S4798', configurations: [], fileTypeTarget: ['MAIN'] }] as RuleConfig[];
initializeLinter(rules);
initializeLinter([], [], [], 'empty');
initializeLinter([], [], [], undefined, 'empty');

const filePath = path.join(__dirname, 'fixtures', 'code.ts');
const tsConfigs = [path.join(__dirname, 'fixtures', 'tsconfig.json')];
Expand Down
2 changes: 1 addition & 1 deletion packages/ruling/tests/tools/testProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function initializeRules(rules: RuleConfig[], customRules?: CustomRule[]) {
}
initializeLinter(rules, DEFAULT_ENVIRONMENTS, DEFAULT_GLOBALS);
const htmlRules = rules.filter(rule => rule.key !== 'S3504');
initializeLinter(htmlRules, DEFAULT_ENVIRONMENTS, DEFAULT_GLOBALS, HTML_LINTER_ID);
initializeLinter(htmlRules, DEFAULT_ENVIRONMENTS, DEFAULT_GLOBALS, undefined, HTML_LINTER_ID);
}
function getProjectName(testFilePath: string) {
const SUFFIX = '.ruling.test.ts';
Expand Down

0 comments on commit 7dc3241

Please sign in to comment.