Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): new lint rule useImportRestrictions #4638

Closed
wants to merge 4 commits into from
Closed
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
39 changes: 0 additions & 39 deletions crates/rome_analyze/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,45 +343,6 @@ This allows the rule to be configured inside `rome.json` file like:
}
```

In this specific case, we don't want the configuration to replace all the standard React hooks configuration,
so to have more control on the deserialization of options, we can implement the trait `DeserializableRuleOptions`.

In the example below we also deserialize to a struct with a more user-friendly schema.

This code run only once when the analyzer is first called.

```rust,ignore

impl DeserializableRuleOptions for ReactExtensiveDependenciesOptions {
fn try_from(value: serde_json::Value) -> Result<Self, serde_json::Error> {
#[derive(Debug, Deserialize)]
#[serde(deny_unknown_fields)]
struct Options {
#[serde(default)]
hooks: Vec<(String, usize, usize)>,
#[serde(default)]
stables: HashSet<StableReactHookConfiguration>,
}

let options: Options = serde_json::from_value(value)?;

let mut default = ReactExtensiveDependenciesOptions::default();
for (k, closure_index, dependencies_index) in options.hooks.into_iter() {
default.hooks_config.insert(
k,
ReactHookConfiguration {
closure_index,
dependencies_index,
},
);
}
default.stable_config.extend(options.stables.into_iter());

Ok(default)
}
}
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this block from the CONTRIBUTING guide since DeserializableRuleOptions is nowhere to be found anymore, so I assume it's no longer relevant.

A rule can retrieve its option with:

```rust,ignore
Expand Down
1 change: 1 addition & 0 deletions crates/rome_diagnostics_categories/src/categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ define_categories! {
"lint/nursery/noGlobalIsFinite": "https://docs.rome.tools/lint/rules/noGlobalIsFinite",
"lint/nursery/useArrowFunction": "https://docs.rome.tools/lint/rules/useArrowFunction",
"lint/nursery/noVoid": "https://docs.rome.tools/lint/rules/noVoid",
"lint/nursery/noNestedModuleImports": "https://docs.rome.tools/lint/rules/noNestedModuleImports",
// Insert new nursery rule here


Expand Down
3 changes: 2 additions & 1 deletion crates/rome_js_analyze/src/analyzers/nursery.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
use bpaf::Bpaf;
use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_deserialize::{
json::{has_only_known_keys, VisitJsonNode},
DeserializationDiagnostic, VisitNode,
};
use rome_js_syntax::JsModuleSource;
use rome_json_syntax::{AnyJsonValue, JsonLanguage, JsonSyntaxNode};
use rome_rowan::{AstNode, SyntaxNode, SyntaxTokenText};
use serde::{Deserialize, Serialize};
use std::{path::Path, str::FromStr};

#[cfg(feature = "schemars")]
use schemars::JsonSchema;

declare_rule! {
/// Forbids importing from nested modules.
///
/// For larger code bases, it can be undesirable to let any file arbitrarily import any other
/// files. Arbitrary imports can lead to cycles that may be hard to debug, so it may be
/// advisable to specify which files may import from which other files.
///
/// A useful rule of thumb is that for modules that consist of several files, only the module's
/// `index.js` or `index.ts` may be imported directly from outside that module, while symbols
/// from other files should only be considered "public" if they're re-exported from the index.
///
/// This rule treats nested imports as an attempt to access "private" internals of a module.
/// Only exports defined by the `index.js` or `index.ts` are allowed to be imported externally.
/// Effectively, this means that you may not directly import any files or subdirectories that
/// are not siblings to the file you're in, or any of its ancestors.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is about nested modules. What about parent and ancestors modules?

/// This rule only applies to relative imports, since the API from external dependencies is
/// often out of your control.
///
/// ## Examples
///
/// ### Invalid
///
/// ```js,expect_diagnostic
/// import { privateInternals } from "../aunt/cousin";
Copy link
Contributor

Choose a reason for hiding this comment

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

I could add the file extension to clarify what is being imported. This is required in ESM.

Suggested change
/// import { privateInternals } from "../aunt/cousin";
/// import { privateInternals } from "../aunt/cousin.js";

It might help to add an explanation of why the example was rejected.

We could also add more examples:

import { internal } from "./sibling/nephew.js";
import { internal } from "../../index.js";

/// ```
///
/// ### Valid
///
/// ```js
/// import { publicExport } from "./sibling";
/// import { reexportedInternals } from "../aunt";
Comment on lines +47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

I could add the file extension to clarify the examples (I was wondering: is it sibling.js or sibiling/index.js?. Moreover, file extensions are required in ESM.

Suggested change
/// import { publicExport } from "./sibling";
/// import { reexportedInternals } from "../aunt";
/// import { publicExport } from "./sibling.js";
/// import { reexportedInternals } from "../aunt.js";

We could also separate the two examples and add a comment to explain why each one is valid.

We should also add the following example?

import { publicExport } from "./sibling/index.js";

/// ```
///
pub(crate) NoNestedModuleImports {
version: "next",
name: "noNestedModuleImports",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we decide on the rule name?

recommended: false,
}
}

impl Rule for NoNestedModuleImports {
type Query = Ast<JsModuleSource>;
type State = ();
type Signals = Vec<Self::State>;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be best if you used Option instead. You use Vec when you need to return multiple violations.

In this case, we signal with None when the run didn't detect anything, and Some(()) when the rule should trigger.

Doing so, will simply the code:

// just this
is_public_import(path, ctx.options())

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the previous comment.

type Options = ImportOptions;

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let binding = ctx.query();
let Ok(path) = binding.inner_string_text() else {
return Vec::new();
};

if is_public_import(path, ctx.options()) {
Vec::new()
} else {
vec![()]
}
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
ctx.query().inner_string_text().ok().map(|path| {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already query path inside run, wouldn't it be wiser to store it inside the State of the rule? No need to repeat the code.

let path = path.text();
let parent = Path::new(path)
.parent()
.and_then(Path::to_str)
.unwrap_or_default();

RuleDiagnostic::new(
rule_category!(),
ctx.query().range(),
markup! {
"Importing from nested modules is not allowed."
},
)
.note(markup! {
"Please import from "<Emphasis>{parent}</Emphasis>" instead "
"(you may need to re-export from "<Emphasis>{path}</Emphasis>")."
})
})
}
}

fn is_public_import(module_path: SyntaxTokenText, options: &ImportOptions) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could explain what a "public import" is

Copy link
Contributor

Choose a reason for hiding this comment

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

is_nested_or_parent_import?

if !module_path.starts_with('.') {
return true;
}

// Make an exception for loading resoures directly:
for allowed_extension in &options.allowed_extensions {
if module_path.ends_with(allowed_extension) {
return true;
}
}

module_path
.split('/')
.filter(|&part| part != "." && part != "..")
.count()
<= 1
}

/// Options for the rule `noNestedModuleImports`.
#[derive(Default, Deserialize, Serialize, Debug, Clone, Bpaf)]
#[cfg_attr(feature = "schemars", derive(JsonSchema))]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct ImportOptions {
/// List of extensions that are always allowed to be imported.
pub allowed_extensions: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like this configuration. I think we should apply the restriction on extension-less imports (to support TypeScript old-fashion imports) and files endings with {.js,.ts,.cjs,.cts,.mjs,.mts}.

}

impl FromStr for ImportOptions {
type Err = ();

fn from_str(_s: &str) -> Result<Self, Self::Err> {
Ok(Self::default())
}
}

impl VisitJsonNode for ImportOptions {}
impl VisitNode<JsonLanguage> for ImportOptions {
fn visit_member_name(
&mut self,
node: &JsonSyntaxNode,
diagnostics: &mut Vec<DeserializationDiagnostic>,
) -> Option<()> {
has_only_known_keys(node, &["allowedExtensions"], diagnostics)
}

fn visit_array_member(
&mut self,
element: &SyntaxNode<JsonLanguage>,
diagnostics: &mut Vec<DeserializationDiagnostic>,
) -> Option<()> {
let element = AnyJsonValue::cast(element.clone())?;
if let Some(extension) = element
.as_json_string_value()
.and_then(|extension| extension.inner_string_text().ok())
{
self.allowed_extensions.push(extension.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to be careful with the configuration. A user could add .js to the list allowed extensions and bypass the rule completely

} else {
diagnostics.push(DeserializationDiagnostic::new(markup! {
"The field "<Emphasis>"allowedExtensions"</Emphasis>" must contain an array of strings"
})
.with_range(element.range()));
}
Some(())
}
}
18 changes: 17 additions & 1 deletion crates/rome_js_analyze/src/options.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! This module contains the rules that have options

use crate::analyzers::nursery::no_nested_module_imports::{import_options, ImportOptions};
use crate::semantic_analyzers::nursery::use_exhaustive_dependencies::{
hooks_options, HooksOptions,
};
Expand All @@ -24,6 +25,8 @@ use std::str::FromStr;
pub enum PossibleOptions {
/// Options for `useExhaustiveDependencies` and `useHookAtTopLevel` rule
Hooks(#[bpaf(external(hooks_options), hide)] HooksOptions),
/// Options for `noNestedModuleImports` rule
Imports(#[bpaf(external(import_options), hide)] ImportOptions),
/// Options for `useNamingConvention` rule
NamingConvention(#[bpaf(external(naming_convention_options), hide)] NamingConventionOptions),
/// No options available
Expand All @@ -40,10 +43,18 @@ impl FromStr for PossibleOptions {
}

impl PossibleOptions {
const KNOWN_KEYS: &'static [&'static str] = &["hooks", "strictCase", "enumMemberCase"];
const KNOWN_KEYS: &'static [&'static str] =
&["allowedExtensions", "hooks", "strictCase", "enumMemberCase"];

pub fn extract_option(&self, rule_key: &RuleKey) -> RuleOptions {
match rule_key.rule_name() {
"noNestedModuleImports" => {
let options = match self {
PossibleOptions::Imports(options) => options.clone(),
_ => ImportOptions::default(),
};
RuleOptions::new(options)
}
"useExhaustiveDependencies" | "useHookAtTopLevel" => {
let options = match self {
PossibleOptions::Hooks(options) => options.clone(),
Expand Down Expand Up @@ -82,6 +93,11 @@ impl VisitNode<JsonLanguage> for PossibleOptions {
) -> Option<()> {
let (name, val) = self.get_key_and_value(key, value, diagnostics)?;
match name.text() {
"allowedExtensions" => {
let mut options = ImportOptions::default();
self.map_to_array(&val, &name, &mut options, diagnostics)?;
*self = PossibleOptions::Imports(options);
}
"hooks" => {
let mut options = HooksOptions::default();
self.map_to_array(&val, &name, &mut options, diagnostics)?;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/* should not generate diagnostics */

import "./resources/allowed_icon.png";
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: allowedExtensions.js
---
# Input
```js
/* should not generate diagnostics */

import "./resources/allowed_icon.png";

```


Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"$schema": "../../../../../../npm/rome/configuration_schema.json",
"linter": {
"rules": {
"nursery": {
"noNestedModuleImports": {
"level": "error",
"options": {
"allowedExtensions": [
".png"
]
}
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

import "../aunt/cousin";
import { someSymbol } from "./module/private";
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: invalid.js
---
# Input
```js

import "../aunt/cousin";
import { someSymbol } from "./module/private";

```

# Diagnostics
```
invalid.js:2:8 lint/nursery/noNestedModuleImports ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Importing from nested modules is not allowed.

> 2 │ import "../aunt/cousin";
│ ^^^^^^^^^^^^^^^^
3 │ import { someSymbol } from "./module/private";
4 │

i Please import from ../aunt instead (you may need to re-export from ../aunt/cousin).


```

```
invalid.js:3:28 lint/nursery/noNestedModuleImports ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Importing from nested modules is not allowed.

2 │ import "../aunt/cousin";
> 3 │ import { someSymbol } from "./module/private";
│ ^^^^^^^^^^^^^^^^^^
4 │

i Please import from ./module instead (you may need to re-export from ./module/private).


```


Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems to be empty. We should remove it

Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: invalidConfig.js
---
# Input
```js

```

# Diagnostics
```
invalidConfig.options:10:29 deserialize ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

× The field allowedExtensions must contain an array of strings

8 │ "options": {
9 │ "allowedExtensions": [
> 10 │ 7
│ ^
11 │ ]
12 │ }


```


Loading