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

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@arendjr
arendjr committed Jul 18, 2023
1 parent cdb81f7 commit f60d42d
Show file tree
Hide file tree
Showing 13 changed files with 174 additions and 153 deletions.
38 changes: 19 additions & 19 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ rome_aria_metadata = { path = "./crates/rome_aria_metadata" }
rome_cli = { path = "./crates/rome_cli" }
rome_console = { version = "0.0.1", path = "./crates/rome_console" }
rome_control_flow = { path = "./crates/rome_control_flow" }
rome_css_factory = { path = "./crates/rome_css_factory" }
rome_css_parser = { path = "./crates/rome_css_parser" }
rome_css_syntax = { path = "./crates/rome_css_syntax" }
rome_deserialize = { version = "0.0.0", path = "./crates/rome_deserialize" }
rome_diagnostics = { version = "0.0.1", path = "./crates/rome_diagnostics" }
rome_diagnostics_categories = { version = "0.0.1", path = "./crates/rome_diagnostics_categories" }
Expand All @@ -51,9 +54,6 @@ rome_json_factory = { version = "0.0.1", path = "./crates/rome_json_fa
rome_json_formatter = { path = "./crates/rome_json_formatter" }
rome_json_parser = { path = "./crates/rome_json_parser" }
rome_json_syntax = { version = "0.0.1", path = "./crates/rome_json_syntax" }
rome_css_factory = { path = "./crates/rome_css_factory" }
rome_css_parser = { path = "./crates/rome_css_parser" }
rome_css_syntax = { path = "./crates/rome_css_syntax" }
rome_lsp = { path = "./crates/rome_lsp" }
rome_markup = { version = "0.0.1", path = "./crates/rome_markup" }
rome_migrate = { path = "./crates/rome_migrate" }
Expand All @@ -65,24 +65,24 @@ rome_text_size = { version = "0.0.1", path = "./crates/rome_text_si
tests_macros = { path = "./crates/tests_macros" }

# Crates needed in the workspace
bitflags = "2.3.1"
bpaf = { version = "0.9.2", features = ["derive"] }
countme = "3.0.1"
dashmap = "5.4.0"
indexmap = "1.9.3"
insta = "1.29.0"
lazy_static = "1.4.0"
quickcheck = "1.0.3"
quickcheck_macros = "1.0.0"
quickcheck = "1.0.3"
bitflags = "2.3.1"
bpaf = { version = "0.9.2", features = ["derive"] }
countme = "3.0.1"
dashmap = "5.4.0"
indexmap = "1.9.3"
insta = "1.29.0"
lazy_static = "1.4.0"
quote = { version = "1.0.28" }
rustc-hash = "1.1.0"
schemars = { version = "0.8.12" }
serde = { version = "1.0.163", features = ["derive"], default-features = false }
serde_json = "1.0.96"
smallvec = { version = "1.10.0", features = ["union", "const_new"] }
tracing = { version = "0.1.37", default-features = false, features = ["std"] }
quote = { version = "1.0.28" }
rustc-hash = "1.1.0"
schemars = { version = "0.8.12" }
serde = { version = "1.0.163", features = ["derive"], default-features = false }
serde_json = "1.0.96"
smallvec = { version = "1.10.0", features = ["union", "const_new"] }
tracing = { version = "0.1.37", default-features = false, features = ["std"] }
# pinning to version 1.18 to avoid multiple versions of windows-sys as dependency
tokio = { version = "~1.18.5" }
tokio = { version = "~1.18.5" }


[profile.dev.package.rome_wasm]
Expand Down
12 changes: 0 additions & 12 deletions crates/rome_console/src/fmt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,18 +176,6 @@ impl<'a> Display for std::fmt::Arguments<'a> {
}
}

impl<'a> Display for std::path::Display<'a> {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
let mut string = self.to_string();
if cfg!(target_os = "windows") {
// Explicitly use forward slashes on Windows for consistency with
// how they are used in JS `import` statements.
string = string.replace('\\', "/");
}
fmt.write_str(&string)
}
}

/// Implement [Display] for types that implement [std::fmt::Display] by calling
/// through to [Formatter::write_fmt]
macro_rules! impl_std_display {
Expand Down
110 changes: 54 additions & 56 deletions crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,31 @@ use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic
use rome_console::markup;
use rome_js_syntax::JsModuleSource;
use rome_rowan::{AstNode, SyntaxTokenText};
use std::{
ffi::OsStr,
path::{Path, PathBuf},
};

const INDEX_BASENAMES: &[&str] = &["index", "mod"];

const SOURCE_EXTENSIONS: &[&str] = &["js", "ts", "cjs", "cts", "mjs", "mts", "jsx", "tsx"];

declare_rule! {
/// Disallows imports from certain modules.
/// Disallows package private imports.
///
/// This rules enforces the following restrictions:
///
/// ## Package private visibility
///
/// All exported symbols are considered to be "package private". This means that modules that
/// reside in the same directory, as well as submodules of those "sibling" modules, are
/// allowed to import them, while any other modules that are further away in the file system
/// are restricted from importing them. A symbol's visibility may be extended by
/// re-exporting from an index file.
/// All exported symbols, such as types, functions or other things that may be exported, are
/// considered to be "package private". This means that modules that reside in the same
/// directory, as well as submodules of those "sibling" modules, are allowed to import them,
/// while any other modules that are further away in the file system are restricted from
/// importing them. A symbol's visibility may be extended by re-exporting from an index file.
///
/// Notes:
///
/// * This rule only applies to relative imports, since the API from external dependencies is
/// often out of your control.
/// * This rule only applies to source imports. Imports for resources such as images or CSS
/// files are exempted.
/// * A future improvement will relax the restriction from "all exported symbols" to those
/// that have an `@package` JSDoc annotation.
/// * This rule only applies to relative imports. External dependencies are exempted.
/// * This rule only applies to imports for JavaScript and TypeScript files. Imports for
/// resources such as images or CSS files are exempted.
///
/// This rule is intended to be extended with additional import restrictions.
/// Please see the tracking issue to follow progress: https://github.com/rome/tools/issues/4678
///
/// Source:
///
/// * https://github.com/uhyo/eslint-plugin-import-access
/// Source: https://github.com/uhyo/eslint-plugin-import-access
///
/// ## Examples
///
Expand All @@ -64,9 +52,6 @@ declare_rule! {
/// // Imports within the same module are always allowed.
/// import { fooPackageVariable } from "./foo.js";
///
/// // Imports within the same module are always allowed.
/// import { fooPackageVariable } from "./foo.js";
///
/// // Resources (anything other than JS/TS files) are exempt.
/// import { barResource } from "../aunt/bar.png";
///
Expand Down Expand Up @@ -105,80 +90,93 @@ impl Rule for UseImportRestrictions {
fn diagnostic(ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
let ImportRestrictionsState { path, suggestion } = state;

let mut diagnostic = RuleDiagnostic::new(
let diagnostic = RuleDiagnostic::new(
rule_category!(),
ctx.query().range(),
markup! {
"Importing package private symbols is not allowed from outside the module directory."
"Importing package private symbols is prohibited from outside the module directory."
},
);

if let Some(suggestion) = suggestion {
diagnostic = diagnostic.note(markup! {
"Please import from "<Emphasis>{suggestion.display()}</Emphasis>" instead "
"(you may need to re-export the symbol(s) from "<Emphasis>{path.display()}</Emphasis>")."
});
}
)
.note(markup! {
"Please import from "<Emphasis>{suggestion}</Emphasis>" instead "
"(you may need to re-export the symbol(s) from "<Emphasis>{path}</Emphasis>")."
});

Some(diagnostic)
}
}

pub(crate) struct ImportRestrictionsState {
/// The path that is being restricted.
path: PathBuf,
path: String,

/// Optional suggestion from which to import instead.
suggestion: Option<PathBuf>,
/// Suggestion from which to import instead.
suggestion: String,
}

fn get_restricted_import(module_path: &SyntaxTokenText) -> Option<ImportRestrictionsState> {
let mut path = PathBuf::from(module_path.text());

if !path.starts_with(".") && !path.starts_with("..") {
if !module_path.starts_with('.') {
return None;
}

let mut path_parts: Vec<&str> = module_path.text().split('/').collect();
let mut index_filename = None;

if let Some(extension) = path.extension() {
if !SOURCE_EXTENSIONS.contains(&extension.to_str().unwrap_or_default()) {
if let Some(extension) = get_extension(&path_parts) {
if !SOURCE_EXTENSIONS.contains(&extension) {
return None; // Resource files are exempt.
}

if let Some(basename) = path.file_stem() {
if INDEX_BASENAMES.contains(&basename.to_str().unwrap_or_default()) {
if let Some(basename) = get_basename(&path_parts) {
if INDEX_BASENAMES.contains(&basename) {
// We pop the index file because it shouldn't count as a path,
// component, but we store the file name so we can add it to
// both the reported path and the suggestion.
index_filename = path.file_name().map(OsStr::to_owned);
path.pop();
index_filename = path_parts.last().cloned();
path_parts.pop();
}
}
}

let is_restricted = path
.components()
.filter(|component| component.as_os_str() != "." && component.as_os_str() != "..")
let is_restricted = path_parts
.iter()
.filter(|&&part| part != "." && part != "..")
.count()
> 1;
if !is_restricted {
return None;
}

let mut suggestion = path.parent().map(Path::to_owned);
let mut suggestion_parts = path_parts[..path_parts.len() - 1].to_vec();

// Push the index file if it exists. This makes sure the reported path
// matches the import path exactly.
if let Some(index_filename) = index_filename {
path.push(&index_filename);
path_parts.push(index_filename);

// Assumes the user probably wants to use an index file that has the
// same name as the original.
if let Some(alternative) = suggestion.as_mut() {
alternative.push(index_filename);
}
suggestion_parts.push(index_filename);
}

Some(ImportRestrictionsState { path, suggestion })
Some(ImportRestrictionsState {
path: path_parts.join("/"),
suggestion: suggestion_parts.join("/"),
})
}

fn get_basename<'a>(path_parts: &'_ [&'a str]) -> Option<&'a str> {
path_parts.last().map(|&part| match part.find('.') {
Some(dot_index) if dot_index > 0 && dot_index < part.len() - 1 => &part[..dot_index],
_ => part,
})
}

fn get_extension<'a>(path_parts: &'_ [&'a str]) -> Option<&'a str> {
path_parts.last().and_then(|part| match part.find('.') {
Some(dot_index) if dot_index > 0 && dot_index < part.len() - 1 => {
Some(&part[dot_index + 1..])
}
_ => None,
})
}
8 changes: 4 additions & 4 deletions ...js_analyze/tests/specs/nursery/useImportRestrictions/invalidPackagePrivateImports.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { fooPackageVariable } from "./sub/foo/index.js";
```
invalidPackagePrivateImports.js:2:36 lint/nursery/useImportRestrictions ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Importing package private symbols is not allowed from outside the module directory.
! Importing package private symbols is prohibited from outside the module directory.
1 │ // Attempt to import from `foo.js` from outside its `sub` module.
> 2 │ import { fooPackageVariable } from "./sub/foo.js";
Expand All @@ -38,7 +38,7 @@ invalidPackagePrivateImports.js:2:36 lint/nursery/useImportRestrictions ━━
```
invalidPackagePrivateImports.js:5:36 lint/nursery/useImportRestrictions ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Importing package private symbols is not allowed from outside the module directory.
! Importing package private symbols is prohibited from outside the module directory.
4 │ // Attempt to import from `bar.ts` from outside its `aunt` module.
> 5 │ import { barPackageVariable } from "../aunt/bar.ts";
Expand All @@ -54,7 +54,7 @@ invalidPackagePrivateImports.js:5:36 lint/nursery/useImportRestrictions ━━
```
invalidPackagePrivateImports.js:8:36 lint/nursery/useImportRestrictions ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Importing package private symbols is not allowed from outside the module directory.
! Importing package private symbols is prohibited from outside the module directory.
7 │ // Assumed to resolve to a JS/TS file.
> 8 │ import { fooPackageVariable } from "./sub/foo";
Expand All @@ -70,7 +70,7 @@ invalidPackagePrivateImports.js:8:36 lint/nursery/useImportRestrictions ━━
```
invalidPackagePrivateImports.js:11:36 lint/nursery/useImportRestrictions ━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Importing package private symbols is not allowed from outside the module directory.
! Importing package private symbols is prohibited from outside the module directory.
10 │ // If the `sub/foo` module is inaccessible, so is its index file.
> 11 │ import { fooPackageVariable } from "./sub/foo/index.js";
Expand Down
8 changes: 6 additions & 2 deletions crates/rome_js_unicode_table/src/tables.rs
View file

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

Loading

0 comments on commit f60d42d

Please sign in to comment.