Skip to content

Commit

Permalink
feat: resolve with hint (#5178)
Browse files Browse the repository at this point in the history
<!--
  Thank you for submitting a pull request!

  We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request.

  Upon submission, your pull request will be automatically assigned with reviewers.

  If you want to learn more about contributing to this project, please visit: https://github.com/web-infra-dev/rspack/blob/main/CONTRIBUTING.md.
-->

## Summary

Related #4348
Added resolve hint diagnostics.

prefer-relative:
```
ERROR in ./index.js
  × Resolve error: Can't resolve 'foo.js' in '<PROJECT_ROOT>/tests/diagnostics/factorize/prefer-relative-resolve-suggestions'
   ╭────
 1 │ import "foo.js";
   ·        ────────
   ╰────
  help: Did you mean './foo.js'?
        
        Requests that should resolve in the current directory need to start with './'.
        Requests that start with a name are treated as module requests and resolve within module directories (node_modules).
        
        If changing the source code is not an option, there is also a resolve options called 'preferRelative'
        which tries to resolve these kind of requests in the current directory too.

```

fully-specified:
```
ERROR in ./index.js
  × Resolve error: Can't resolve './foo' in '<PROJECT_ROOT>/tests/diagnostics/factorize/fully-specified-resolve-suggestions'
   ╭────
 1 │ import "./foo";
   ·        ───────
   ╰────
  help: Did you mean './foo.js'?
        
        The request './foo' failed to resolve only because it was resolved as fully specified,
        probably because the origin is strict EcmaScript Module,
        e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"'.
        
        The extension in the request is mandatory for it to be fully specified.
        Add the extension to the request.

```

<!-- Can you explain the reasoning behind implementing this change? What problem or issue does this pull request resolve? -->

<!-- It would be helpful if you could provide any relevant context, such as GitHub issues or related discussions. -->

## Test Plan

See snapshot diagnostic changes:
- rspack/tests/diagnostics/factorize/fully-specified-resolve-suggestions
- rspack/tests/diagnostics/factorize/prefer-relative-resolve-suggestions

Other snapshot changes are caused by "error.separator!".

<!-- Can you please describe how you tested the changes you made to the code? -->

## Require Documentation?

<!-- Does this PR require documentation? -->

- [x] No
- [ ] Yes, the corresponding rspack-website PR is \_\_
  • Loading branch information
h-a-n-a authored Jan 11, 2024
1 parent 55ac9e5 commit a3f7ec1
Show file tree
Hide file tree
Showing 39 changed files with 388 additions and 127 deletions.
1 change: 1 addition & 0 deletions crates/rspack_core/src/context_module_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ impl ContextModuleFactory {
let resolve_args = ResolveArgs {
context: data.context.clone(),
importer: None,
issuer: data.issuer.as_deref(),
specifier,
dependency_type: dependency.dependency_type(),
dependency_category: dependency.category(),
Expand Down
59 changes: 59 additions & 0 deletions crates/rspack_core/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,62 @@ pub fn map_box_diagnostics_to_module_parse_diagnostics(
.map(|e| rspack_error::miette::Error::new(ModuleParseError::new(e, loaders)).into())
.collect()
}

///////////////////// Diagnostic helpers /////////////////////

/// Wrap diagnostic with additional help message.
#[derive(Debug, Error)]
#[error("{0}")]
pub struct WithHelp(Box<dyn Diagnostic + Send + Sync>, Option<String>);

impl WithHelp {
pub fn with_help(mut self, help: impl Into<String>) -> Self {
let mut help = help.into();
if let Some(prev) = self.0.help().map(|h| h.to_string()) {
help = format!("{prev}\n{help}");
}
self.1 = Some(help);
self
}
}

impl From<Box<dyn Diagnostic + Send + Sync>> for WithHelp {
fn from(value: Box<dyn Diagnostic + Send + Sync>) -> Self {
Self(value, None)
}
}

impl miette::Diagnostic for WithHelp {
fn code<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
(*self.0).code()
}

fn severity(&self) -> Option<miette::Severity> {
(*self.0).severity()
}

fn help<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
// Use overwritten help message instead.
self.1.as_ref().map(Box::new).map(|h| h as Box<dyn Display>)
}

fn url<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
(*self.0).url()
}

fn source_code(&self) -> Option<&dyn miette::SourceCode> {
(*self.0).source_code()
}

fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> {
(*self.0).labels()
}

fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn miette::Diagnostic> + 'a>> {
(*self.0).related()
}

fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
(*self.0).diagnostic_source()
}
}
53 changes: 1 addition & 52 deletions crates/rspack_core/src/normal_module_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ impl NormalModuleFactory {

let resolve_args = ResolveArgs {
importer,
issuer: data.issuer.as_deref(),
context: if context_scheme != Scheme::None {
self.options.context.clone()
} else {
Expand Down Expand Up @@ -337,58 +338,6 @@ impl NormalModuleFactory {
Err(err) => {
data.add_file_dependencies(file_dependencies);
data.add_missing_dependencies(missing_dependencies);
// let mut file_dependencies = Default::default();
// let mut missing_dependencies = Default::default();
// let mut from_cache_result = from_cache;
// if !data
// .resolve_options
// .as_ref()
// .and_then(|x| x.fully_specified)
// .unwrap_or(false)
// {
// let new_args = ResolveArgs {
// importer,
// context: if context_scheme != Scheme::None {
// self.options.context.clone()
// } else {
// data.context.clone()
// },
// specifier: request_without_match_resource,
// dependency_type: dependency.dependency_type(),
// dependency_category: dependency.category(),
// resolve_options: data.resolve_options.take(),
// span: dependency.span(),
// resolve_to_context: false,
// optional,
// missing_dependencies: &mut missing_dependencies,
// file_dependencies: &mut file_dependencies,
// };
// let (resource_data, from_cache) = match self
// .cache
// .resolve_module_occasion
// .use_cache(new_args, |args| resolve(args, plugin_driver))
// .await
// {
// Ok(result) => result,
// Err(err) => (Err(err), false),
// };
// from_cache_result = from_cache;
// if let Ok(ResolveResult::Resource(resource)) = resource_data {
// // TODO: Here windows resolver will return normalized path.
// // eg. D:\a\rspack\rspack\packages\rspack\tests\fixtures\errors\resolve-fail-esm\answer.js
// if let Some(_extension) = resource.path.extension() {
// // let resource = format!(
// // "{request_without_match_resource}.{}",
// // extension.to_string_lossy()
// // );
// // diagnostics[0].add_notes(vec![format!("Did you mean '{resource}'?
// // BREAKING CHANGE: The request '{request_without_match_resource}' failed to resolve only because it was resolved as fully specified
// // (probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '\"type\": \"module\"').
// // The extension in the request is mandatory for it to be fully specified.
// // Add the extension to the request.")]);
// }
// }
// }
return Err(err);
}
}
Expand Down
18 changes: 18 additions & 0 deletions crates/rspack_core/src/options/resolve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,21 @@ impl From<TsconfigReferences> for oxc_resolver::TsconfigReferences {
}
}

macro_rules! impl_resolve_by_dependency {
($ident:ident) => {
pub fn $ident(&self, cat: Option<&DependencyCategory>) -> Option<bool> {
cat
.and_then(|cat| {
self
.by_dependency
.as_ref()
.and_then(|by_dep| by_dep.get(cat).and_then(|d| d.$ident))
})
.or(self.$ident)
}
};
}

impl Resolve {
pub fn merge_by_dependency(mut self, dependency_type: DependencyCategory) -> Self {
let Some(mut by_dependency) = self.by_dependency.as_mut().map(std::mem::take) else {
Expand All @@ -139,6 +154,9 @@ impl Resolve {
pub fn merge(self, value: Self) -> Self {
clever_merge::merge_resolve(self, value)
}

impl_resolve_by_dependency!(fully_specified);
impl_resolve_by_dependency!(prefer_relative);
}

type DependencyCategoryStr = Cow<'static, str>;
Expand Down
1 change: 1 addition & 0 deletions crates/rspack_core/src/plugin/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ pub struct NormalModuleAfterResolveArgs<'a> {
#[derive(Debug)]
pub struct ResolveArgs<'a> {
pub importer: Option<&'a ModuleIdentifier>,
pub issuer: Option<&'a str>,
pub context: Context,
pub specifier: &'a str,
pub dependency_type: &'a DependencyType,
Expand Down
128 changes: 123 additions & 5 deletions crates/rspack_core/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,26 @@ mod resolver_impl;

use std::{fmt, path::PathBuf};

use rspack_error::Error;
use once_cell::sync::Lazy;
use regex::Regex;
use rspack_error::{DiagnosticExt, Error};
use rspack_loader_runner::DescriptionData;
use sugar_path::{AsPath, SugarPath};

pub use self::factory::{ResolveOptionsWithDependencyType, ResolverFactory};
pub use self::resolver_impl::{ResolveInnerOptions, Resolver};
use crate::diagnostics::WithHelp;
use crate::{ResolveArgs, SharedPluginDriver};

static RELATIVE_PATH_REGEX: Lazy<Regex> =
Lazy::new(|| Regex::new(r"^\.\.?\/").expect("should init regex"));

static PARENT_PATH_REGEX: Lazy<Regex> =
Lazy::new(|| Regex::new(r"^\.\.[\/]").expect("should init regex"));

static CURRENT_DIR_REGEX: Lazy<Regex> =
Lazy::new(|| Regex::new(r"^(\.[\/])").expect("should init regex"));

/// A successful path resolution or an ignored path.
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum ResolveResult {
Expand Down Expand Up @@ -55,13 +68,112 @@ impl Resource {
}
}

pub fn resolve_for_error_hints(
args: ResolveArgs<'_>,
plugin_driver: &SharedPluginDriver,
) -> Option<String> {
let dep = ResolveOptionsWithDependencyType {
resolve_options: args.resolve_options.clone(),
resolve_to_context: args.resolve_to_context,
dependency_category: *args.dependency_category,
};

let base_dir = args.context.clone();
let base_dir = base_dir.as_ref();

let fully_specified = dep
.resolve_options
.as_ref()
.and_then(|o| o.fully_specified(Some(args.dependency_category)))
.unwrap_or_default();

let prefer_relative = dep
.resolve_options
.as_ref()
.and_then(|o| o.prefer_relative(Some(args.dependency_category)))
.unwrap_or_default();

// Try to resolve without fully specified
if fully_specified {
let mut dep = dep.clone();
dep.resolve_options = dep.resolve_options.map(|mut options| {
options.fully_specified = Some(false);
options
});
let resolver = plugin_driver.resolver_factory.get(dep);
match resolver.resolve(base_dir, args.specifier) {
Ok(ResolveResult::Resource(resource)) => {
let relative_path = resource.path.relative(args.context.as_path());
let suggestion = if let Some((_, [prefix])) = CURRENT_DIR_REGEX
.captures_iter(args.specifier)
.next()
.map(|c| c.extract())
{
// If the specifier is a relative path pointing to the current directory,
// we can suggest the path relative to the current directory.
format!("{}{}", prefix, relative_path.to_string_lossy())
} else if PARENT_PATH_REGEX.is_match(args.specifier) {
// If the specifier is a relative path to which the parent directory is,
// then we return the relative path directly.
relative_path.to_string_lossy().to_string()
} else {
// If the specifier is a package name like or some arbitrary alias,
// then we return the full path.
resource.path.to_string_lossy().to_string()
};
return Some(format!("Did you mean '{}'?
The request '{}' failed to resolve only because it was resolved as fully specified,
probably because the origin is strict EcmaScript Module,
e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '\"type\": \"module\"'.
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.", suggestion, args.specifier));
}
Err(_) => return None,
_ => {}
}
}

// Try to resolve with relative path if request is not relative
if !RELATIVE_PATH_REGEX.is_match(args.specifier) && !prefer_relative {
let dep = dep.clone();
let module_directories = dep
.resolve_options
.as_deref()
.or(Some(&plugin_driver.options.resolve))
.and_then(|o| o.modules.as_ref().map(|m| m.join(", ")))
.expect("module directories should exist");
let resolver = plugin_driver.resolver_factory.get(dep);
let request = format!("./{}", args.specifier);
match resolver.resolve(base_dir, &request) {
Ok(ResolveResult::Resource(_)) => {
return Some(format!(
"Did you mean './{}'?
Requests that should resolve in the current directory need to start with './'.
Requests that start with a name are treated as module requests and resolve within module directories ({module_directories}).
If changing the source code is not an option, there is also a resolve options called 'preferRelative'
which tries to resolve these kind of requests in the current directory too.",
args.specifier
));
}
Err(_) => return None,
_ => {}
}
}

None
}

/// Main entry point for module resolution.
pub async fn resolve(
mut args: ResolveArgs<'_>,
args: ResolveArgs<'_>,
plugin_driver: &SharedPluginDriver,
) -> Result<ResolveResult, Error> {
let dep = ResolveOptionsWithDependencyType {
resolve_options: args.resolve_options.take(),
resolve_options: args.resolve_options.clone(),
resolve_to_context: args.resolve_to_context,
dependency_category: *args.dependency_category,
};
Expand All @@ -71,7 +183,7 @@ pub async fn resolve(

let mut context = Default::default();
let resolver = plugin_driver.resolver_factory.get(dep);
let result = resolver
let mut result = resolver
.resolve_with_context(base_dir, args.specifier, &mut context)
.map_err(|error| error.into_resolve_error(&args));

Expand All @@ -80,5 +192,11 @@ pub async fn resolve(
.missing_dependencies
.extend(context.missing_dependencies);

result
if result.is_err()
&& let Some(hint) = resolve_for_error_hints(args, plugin_driver)
{
result = result.map_err(|err| WithHelp::from(err).with_help(hint).boxed())
};

result.map_err(Error::new_boxed)
}
Loading

1 comment on commit a3f7ec1

@rspack-bot
Copy link

Choose a reason for hiding this comment

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

📝 Benchmark detail: Open

name base current %
10000_development-mode + exec 1.64 s ± 15 ms 1.63 s ± 18 ms -0.36%
10000_development-mode_hmr + exec 1.01 s ± 7.6 ms 999 ms ± 12 ms -1.21%
10000_production-mode + exec 2.83 s ± 57 ms 2.81 s ± 25 ms -0.82%
threejs_development-mode_10x + exec 2 s ± 14 ms 1.99 s ± 18 ms -0.47%
threejs_development-mode_10x_hmr + exec 1.33 s ± 12 ms 1.31 s ± 10 ms -1.27%
threejs_production-mode_10x + exec 5.99 s ± 36 ms 6.02 s ± 124 ms +0.64%

Please sign in to comment.