Skip to content

Commit ed16c70

Browse files
committed
feat: implement noPrivateImports
1 parent f6a62e5 commit ed16c70

File tree

25 files changed

+816
-445
lines changed

25 files changed

+816
-445
lines changed

crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs

+2-5
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/biome_configuration/src/analyzer/linter/rules.rs

+182-182
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/biome_dependency_graph/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@ mod dependency_graph;
22
mod module_visitor;
33
mod resolver_cache;
44

5-
pub use dependency_graph::{DependencyGraph, Import, ModuleDependencyData};
5+
pub use dependency_graph::{DependencyGraph, Export, Import, ModuleDependencyData};

crates/biome_diagnostics_categories/src/categories.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ define_categories! {
101101
"lint/correctness/noNodejsModules": "https://biomejs.dev/linter/rules/no-nodejs-modules",
102102
"lint/correctness/noNonoctalDecimalEscape": "https://biomejs.dev/linter/rules/no-nonoctal-decimal-escape",
103103
"lint/correctness/noPrecisionLoss": "https://biomejs.dev/linter/rules/no-precision-loss",
104+
"lint/correctness/noPrivateImports": "https://biomejs.dev/linter/rules/no-private-imports",
104105
"lint/correctness/noRenderReturnValue": "https://biomejs.dev/linter/rules/no-render-return-value",
105106
"lint/correctness/noSelfAssign": "https://biomejs.dev/linter/rules/no-self-assign",
106107
"lint/correctness/noSetterReturn": "https://biomejs.dev/linter/rules/no-setter-return",
@@ -169,7 +170,6 @@ define_categories! {
169170
"lint/nursery/noNestedTernary": "https://biomejs.dev/linter/rules/no-nested-ternary",
170171
"lint/nursery/noNoninteractiveElementInteractions": "https://biomejs.dev/linter/rules/no-noninteractive-element-interactions",
171172
"lint/nursery/noOctalEscape": "https://biomejs.dev/linter/rules/no-octal-escape",
172-
"lint/nursery/noPackagePrivateImports": "https://biomejs.dev/linter/rules/no-package-private-imports",
173173
"lint/nursery/noProcessEnv": "https://biomejs.dev/linter/rules/no-process-env",
174174
"lint/nursery/noProcessGlobal": "https://biomejs.dev/linter/rules/no-process-global",
175175
"lint/nursery/noReactSpecificProps": "https://biomejs.dev/linter/rules/no-react-specific-props",

crates/biome_js_analyze/src/lint/correctness.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,4 @@ pub mod use_is_nan;
4949
pub mod use_jsx_key_in_iterable;
5050
pub mod use_valid_for_direction;
5151
pub mod use_yield;
52-
declare_lint_group! { pub Correctness { name : "correctness" , rules : [self :: no_children_prop :: NoChildrenProp , self :: no_const_assign :: NoConstAssign , self :: no_constant_condition :: NoConstantCondition , self :: no_constant_math_min_max_clamp :: NoConstantMathMinMaxClamp , self :: no_constructor_return :: NoConstructorReturn , self :: no_empty_character_class_in_regex :: NoEmptyCharacterClassInRegex , self :: no_empty_pattern :: NoEmptyPattern , self :: no_flat_map_identity :: NoFlatMapIdentity , self :: no_global_object_calls :: NoGlobalObjectCalls , self :: no_inner_declarations :: NoInnerDeclarations , self :: no_invalid_builtin_instantiation :: NoInvalidBuiltinInstantiation , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: no_invalid_new_builtin :: NoInvalidNewBuiltin , self :: no_invalid_use_before_declaration :: NoInvalidUseBeforeDeclaration , self :: no_new_symbol :: NoNewSymbol , self :: no_nodejs_modules :: NoNodejsModules , self :: no_nonoctal_decimal_escape :: NoNonoctalDecimalEscape , self :: no_private_imports :: NoPrivateImports , self :: no_precision_loss :: NoPrecisionLoss , self :: no_render_return_value :: NoRenderReturnValue , self :: no_self_assign :: NoSelfAssign , self :: no_setter_return :: NoSetterReturn , self :: no_string_case_mismatch :: NoStringCaseMismatch , self :: no_switch_declarations :: NoSwitchDeclarations , self :: no_undeclared_dependencies :: NoUndeclaredDependencies , self :: no_undeclared_variables :: NoUndeclaredVariables , self :: no_unnecessary_continue :: NoUnnecessaryContinue , self :: no_unreachable :: NoUnreachable , self :: no_unreachable_super :: NoUnreachableSuper , self :: no_unsafe_finally :: NoUnsafeFinally , self :: no_unsafe_optional_chaining :: NoUnsafeOptionalChaining , self :: no_unused_function_parameters :: NoUnusedFunctionParameters , self :: no_unused_imports :: NoUnusedImports , self :: no_unused_labels :: NoUnusedLabels , self :: no_unused_private_class_members :: NoUnusedPrivateClassMembers , self :: no_unused_variables :: NoUnusedVariables , self :: no_void_elements_with_children :: NoVoidElementsWithChildren , self :: no_void_type_return :: NoVoidTypeReturn , self :: use_array_literals :: UseArrayLiterals , self :: use_exhaustive_dependencies :: UseExhaustiveDependencies , self :: use_hook_at_top_level :: UseHookAtTopLevel , self :: use_import_extensions :: UseImportExtensions , self :: use_is_nan :: UseIsNan , self :: use_jsx_key_in_iterable :: UseJsxKeyInIterable , self :: use_valid_for_direction :: UseValidForDirection , self :: use_yield :: UseYield ,] } }
52+
declare_lint_group! { pub Correctness { name : "correctness" , rules : [self :: no_children_prop :: NoChildrenProp , self :: no_const_assign :: NoConstAssign , self :: no_constant_condition :: NoConstantCondition , self :: no_constant_math_min_max_clamp :: NoConstantMathMinMaxClamp , self :: no_constructor_return :: NoConstructorReturn , self :: no_empty_character_class_in_regex :: NoEmptyCharacterClassInRegex , self :: no_empty_pattern :: NoEmptyPattern , self :: no_flat_map_identity :: NoFlatMapIdentity , self :: no_global_object_calls :: NoGlobalObjectCalls , self :: no_inner_declarations :: NoInnerDeclarations , self :: no_invalid_builtin_instantiation :: NoInvalidBuiltinInstantiation , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: no_invalid_new_builtin :: NoInvalidNewBuiltin , self :: no_invalid_use_before_declaration :: NoInvalidUseBeforeDeclaration , self :: no_new_symbol :: NoNewSymbol , self :: no_nodejs_modules :: NoNodejsModules , self :: no_nonoctal_decimal_escape :: NoNonoctalDecimalEscape , self :: no_precision_loss :: NoPrecisionLoss , self :: no_private_imports :: NoPrivateImports , self :: no_render_return_value :: NoRenderReturnValue , self :: no_self_assign :: NoSelfAssign , self :: no_setter_return :: NoSetterReturn , self :: no_string_case_mismatch :: NoStringCaseMismatch , self :: no_switch_declarations :: NoSwitchDeclarations , self :: no_undeclared_dependencies :: NoUndeclaredDependencies , self :: no_undeclared_variables :: NoUndeclaredVariables , self :: no_unnecessary_continue :: NoUnnecessaryContinue , self :: no_unreachable :: NoUnreachable , self :: no_unreachable_super :: NoUnreachableSuper , self :: no_unsafe_finally :: NoUnsafeFinally , self :: no_unsafe_optional_chaining :: NoUnsafeOptionalChaining , self :: no_unused_function_parameters :: NoUnusedFunctionParameters , self :: no_unused_imports :: NoUnusedImports , self :: no_unused_labels :: NoUnusedLabels , self :: no_unused_private_class_members :: NoUnusedPrivateClassMembers , self :: no_unused_variables :: NoUnusedVariables , self :: no_void_elements_with_children :: NoVoidElementsWithChildren , self :: no_void_type_return :: NoVoidTypeReturn , self :: use_array_literals :: UseArrayLiterals , self :: use_exhaustive_dependencies :: UseExhaustiveDependencies , self :: use_hook_at_top_level :: UseHookAtTopLevel , self :: use_import_extensions :: UseImportExtensions , self :: use_is_nan :: UseIsNan , self :: use_jsx_key_in_iterable :: UseJsxKeyInIterable , self :: use_valid_for_direction :: UseValidForDirection , self :: use_yield :: UseYield ,] } }

crates/biome_js_analyze/src/lint/correctness/no_private_imports.rs

+144-93
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
use biome_analyze::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
22
use biome_console::{fmt::Display, markup};
3-
use biome_dependency_graph::{DependencyGraph, Import, ModuleDependencyData};
3+
use biome_dependency_graph::{Export, ModuleDependencyData};
44
use biome_deserialize_macros::Deserializable;
55
use biome_js_syntax::{
6-
AnyJsImportClause, AnyJsImportLike, JsDefaultImportSpecifier, JsLanguage, JsModuleSource,
7-
inner_string_text,
6+
AnyJsImportClause, AnyJsImportLike, AnyJsNamedImportSpecifier, JsModuleSource, JsSyntaxToken,
87
};
9-
use biome_rowan::{AstNode, SyntaxNode, SyntaxResult, TextRange, TokenText};
8+
use biome_rowan::{AstNode, SyntaxResult, Text, TextRange};
109
use camino::Utf8Path;
1110
use serde::{Deserialize, Serialize};
11+
use std::str::FromStr;
1212

1313
#[cfg(feature = "schemars")]
1414
use schemars::JsonSchema;
@@ -124,7 +124,7 @@ pub struct NoPrivateImportsOptions {
124124
#[derive(Clone, Copy, Debug, Default, Deserialize, Deserializable, Eq, PartialEq, Serialize)]
125125
#[cfg_attr(feature = "schemars", derive(JsonSchema))]
126126
#[serde(rename_all = "camelCase")]
127-
enum Visibility {
127+
pub enum Visibility {
128128
#[default]
129129
Public,
130130
Package,
@@ -141,6 +141,20 @@ impl Display for Visibility {
141141
}
142142
}
143143

144+
impl FromStr for Visibility {
145+
type Err = ();
146+
147+
fn from_str(s: &str) -> Result<Self, Self::Err> {
148+
match s {
149+
"public" => Ok(Visibility::Public),
150+
"package" => Ok(Visibility::Package),
151+
"private" => Ok(Visibility::Private),
152+
_ => Err(()),
153+
}
154+
}
155+
}
156+
157+
#[derive(Debug)]
144158
pub struct NoPrivateImportsState {
145159
range: TextRange,
146160

@@ -158,33 +172,39 @@ impl Rule for NoPrivateImports {
158172
type Options = NoPrivateImportsOptions;
159173

160174
fn run(ctx: &RuleContext<Self>) -> Self::Signals {
175+
let self_path = ctx.file_path();
161176
let Some(file_imports) = ctx.imports_for_path(ctx.file_path()) else {
162177
return Vec::new();
163178
};
164179

165180
let node = ctx.query();
166-
let Some(target_data) = file_imports
181+
let Some(target_path) = file_imports
167182
.get_import_by_node(node)
168183
.and_then(|import| import.resolved_path.as_ref().ok())
169-
.and_then(|target_path| ctx.imports_for_path(&target_path))
170184
else {
171185
return Vec::new();
172186
};
173187

188+
let Some(target_data) = ctx.imports_for_path(target_path) else {
189+
return Vec::new();
190+
};
191+
174192
let options = GetRestrictedImportOptions {
175-
dependency_graph: ctx
176-
.get_service()
177-
.expect("Dependency graph must be initialised"),
193+
self_path,
194+
target_path,
178195
target_data,
179196
default_visibility: ctx.options().default_visibility,
180197
};
181198

182199
let result = match node {
183200
AnyJsImportLike::JsModuleSource(node) => {
184-
get_restricted_imports_from_module_source(node, options)
201+
get_restricted_imports_from_module_source(node, &options)
185202
}
186-
AnyJsImportLike::JsCallExpression(node) => todo!(),
187-
AnyJsImportLike::JsImportCallExpression(node) => todo!(),
203+
204+
// TODO: require() and import() calls should also be handled here, but tracking the
205+
// bindings to get the used symbol names is not easy. I think we can leave it
206+
// for future opportunities.
207+
_ => Ok(Vec::new()),
188208
};
189209

190210
result.unwrap_or_default()
@@ -210,8 +230,11 @@ impl Rule for NoPrivateImports {
210230
}
211231

212232
struct GetRestrictedImportOptions<'a> {
213-
/// Reference to the dependency graph for looking up additional imports.
214-
dependency_graph: &'a DependencyGraph,
233+
/// The self module path we're importing to.
234+
self_path: &'a Utf8Path,
235+
236+
/// The target module path we're importing from.
237+
target_path: &'a Utf8Path,
215238

216239
/// Dependency data of the target module we're importing from.
217240
target_data: ModuleDependencyData,
@@ -223,18 +246,69 @@ struct GetRestrictedImportOptions<'a> {
223246

224247
fn get_restricted_imports_from_module_source(
225248
node: &JsModuleSource,
226-
options: GetRestrictedImportOptions,
249+
options: &GetRestrictedImportOptions,
227250
) -> SyntaxResult<Vec<NoPrivateImportsState>> {
251+
let path = options.target_path.to_string();
252+
228253
let results = match node.syntax().parent().and_then(AnyJsImportClause::cast) {
229-
Some(AnyJsImportClause::JsImportCombinedClause(node)) => todo!(),
230-
Some(AnyJsImportClause::JsImportDefaultClause(node)) => get_restricted_import(
231-
node.default_specifier()
232-
.map(JsDefaultImportSpecifier::into_syntax)?,
233-
&options,
234-
)?
235-
.into_iter()
236-
.collect(),
237-
Some(AnyJsImportClause::JsImportNamedClause(node)) => todo!(),
254+
Some(AnyJsImportClause::JsImportCombinedClause(node)) => {
255+
let range = node.default_specifier()?.range();
256+
get_restricted_import(&Text::Static("default"), options)
257+
.map(|visibility| NoPrivateImportsState {
258+
range,
259+
path: path.clone(),
260+
visibility,
261+
})
262+
.into_iter()
263+
.chain(
264+
node.specifier()?
265+
.as_js_named_import_specifiers()
266+
.map(|specifiers| specifiers.specifiers())
267+
.into_iter()
268+
.flatten()
269+
.flatten()
270+
.filter_map(get_named_specifier_import_name)
271+
.filter_map(|name| {
272+
get_restricted_import(
273+
&Text::Borrowed(name.token_text_trimmed()),
274+
options,
275+
)
276+
.map(|visibility| NoPrivateImportsState {
277+
range: name.text_trimmed_range(),
278+
path: path.clone(),
279+
visibility,
280+
})
281+
}),
282+
)
283+
.collect()
284+
}
285+
Some(AnyJsImportClause::JsImportDefaultClause(node)) => {
286+
let range = node.default_specifier()?.range();
287+
get_restricted_import(&Text::Static("default"), options)
288+
.map(|visibility| NoPrivateImportsState {
289+
range,
290+
path,
291+
visibility,
292+
})
293+
.into_iter()
294+
.collect()
295+
}
296+
Some(AnyJsImportClause::JsImportNamedClause(node)) => node
297+
.named_specifiers()?
298+
.specifiers()
299+
.into_iter()
300+
.flatten()
301+
.filter_map(get_named_specifier_import_name)
302+
.filter_map(|name| {
303+
get_restricted_import(&Text::Borrowed(name.token_text_trimmed()), options).map(
304+
|visibility| NoPrivateImportsState {
305+
range: name.text_trimmed_range(),
306+
path: path.clone(),
307+
visibility,
308+
},
309+
)
310+
})
311+
.collect(),
238312
Some(
239313
AnyJsImportClause::JsImportBareClause(_)
240314
| AnyJsImportClause::JsImportNamespaceClause(_),
@@ -245,79 +319,56 @@ fn get_restricted_imports_from_module_source(
245319
Ok(results)
246320
}
247321

248-
/// Returns `Some` signal if the given `specifier_node` references an import
249-
/// that is more private than allowed.
250-
fn get_restricted_import(
251-
specifier_node: SyntaxNode<JsLanguage>,
252-
options: &GetRestrictedImportOptions,
253-
) -> SyntaxResult<Option<NoPrivateImportsState>> {
254-
let symbol_name = specifier_node.text_trimmed();
255-
256-
if !module_path.starts_with('.') {
257-
return None;
258-
}
259-
260-
let mut path_parts: Vec<_> = module_path.text().split('/').collect();
261-
let mut index_filename = None;
262-
263-
// TODO. The implementation could be optimized further by using
264-
// `Path::new(module_path.text())` for further inspiration see `use_import_extensions` rule.
265-
if let Some(extension) = get_extension(&path_parts) {
266-
if !SOURCE_EXTENSIONS.contains(&extension) {
267-
return None; // Resource files are exempt.
322+
fn get_named_specifier_import_name(specifier: AnyJsNamedImportSpecifier) -> Option<JsSyntaxToken> {
323+
match specifier {
324+
AnyJsNamedImportSpecifier::JsNamedImportSpecifier(specifier) => {
325+
specifier.name().ok().and_then(|name| name.value().ok())
268326
}
269-
270-
if let Some(basename) = get_basename(&path_parts) {
271-
if INDEX_BASENAMES.contains(&basename) {
272-
// We pop the index file because it shouldn't count as a path,
273-
// component, but we store the file name so we can add it to
274-
// both the reported path and the suggestion.
275-
index_filename = path_parts.last().copied();
276-
path_parts.pop();
277-
}
278-
}
279-
}
280-
281-
let is_restricted = path_parts
282-
.iter()
283-
.filter(|&&part| part != "." && part != "..")
284-
.count()
285-
> 1;
286-
if !is_restricted {
287-
return None;
327+
AnyJsNamedImportSpecifier::JsShorthandNamedImportSpecifier(specifier) => specifier
328+
.local_name()
329+
.ok()
330+
.and_then(|binding| binding.as_js_identifier_binding()?.name_token().ok()),
331+
_ => None,
288332
}
333+
}
289334

290-
let mut suggestion_parts = path_parts[..path_parts.len() - 1].to_vec();
291-
292-
// Push the index file if it exists. This makes sure the reported path
293-
// matches the import path exactly.
294-
if let Some(index_filename) = index_filename {
295-
path_parts.push(index_filename);
296-
297-
// Assumes the user probably wants to use an index file that has the
298-
// same name as the original.
299-
suggestion_parts.push(index_filename);
300-
}
335+
/// Returns `Some` signal if the given `import_name` references an import
336+
/// that is more private than allowed.
337+
fn get_restricted_import(
338+
import_name: &Text,
339+
options: &GetRestrictedImportOptions,
340+
) -> Option<Visibility> {
341+
let visibility = options
342+
.target_data
343+
.exports
344+
.get(import_name)
345+
.and_then(|export| match export {
346+
Export::Own(export) => export.jsdoc_comment.as_deref().and_then(parse_visibility),
347+
348+
// TODO: Should we follow re-exports here? I think re-exports don't inherit the
349+
// visibility from where the name is declared; e.g. package-private symbols can be
350+
// re-exported from index.js to make it public. Thus we can fallback to the
351+
// default visibility if they're re-exported and not added any visibility there.
352+
_ => None,
353+
})
354+
.unwrap_or(options.default_visibility);
301355

302-
Some(NoPrivateImportsState {
303-
range,
304-
path: path_parts.join("/"),
305-
suggestion: suggestion_parts.join("/"),
306-
})
307-
}
356+
let is_restricted = match visibility {
357+
Visibility::Public => false,
358+
Visibility::Private => true,
359+
Visibility::Package => options.target_path.parent() != options.self_path.parent(),
360+
};
308361

309-
fn get_basename<'a>(path_parts: &'_ [&'a str]) -> Option<&'a str> {
310-
path_parts.last().map(|&part| match part.find('.') {
311-
Some(dot_index) if dot_index > 0 && dot_index < part.len() - 1 => &part[..dot_index],
312-
_ => part,
313-
})
362+
is_restricted.then_some(visibility)
314363
}
315364

316-
fn get_extension<'a>(path_parts: &'_ [&'a str]) -> Option<&'a str> {
317-
path_parts.last().and_then(|part| match part.find('.') {
318-
Some(dot_index) if dot_index > 0 && dot_index < part.len() - 1 => {
319-
Some(&part[dot_index + 1..])
320-
}
321-
_ => None,
322-
})
365+
/// Parses a JSDoc comment to find the first `@public`, `@package`, or `@private` tag.
366+
fn parse_visibility(jsdoc_comment: &str) -> Option<Visibility> {
367+
jsdoc_comment
368+
.lines()
369+
.find_map(|line| {
370+
line.strip_prefix("@")
371+
.and_then(|tag| tag.split_whitespace().next())
372+
})
373+
.and_then(|tag| Visibility::from_str(tag).ok())
323374
}

0 commit comments

Comments
 (0)