Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(lsp): move sloppy import resolution from loader to resolver #23751

Merged
merged 8 commits into from
May 9, 2024
Merged

fix(lsp): move sloppy import resolution from loader to resolver #23751

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
821d987
Add some test helpers
nathanwhit May 8, 2024
2e9772f
Add test
nathanwhit May 8, 2024
178e025
Prefer original specifier if it exists
nathanwhit May 8, 2024
8377587
fix and remove sloppy import warning because it is not worth maintain…
dsherret May 9, 2024
d572ea5
reduce some clones
dsherret May 9, 2024
130f3c7
Remove open_docs being passed in and don't rely on that
dsherret May 9, 2024
8e34b1f
Merge branch 'main' into lsp-sloppy-imports-bug
dsherret May 9, 2024
1d8e394
Revert the cache changes.
dsherret May 9, 2024
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
Prev Previous commit
Revert the cache changes.
  • Loading branch information
@dsherret
dsherret committed May 9, 2024
commit 1d8e394c6c39f4e62a3aeed54556b31428d32434
4 changes: 1 addition & 3 deletions cli/lsp/completions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,7 +803,6 @@ mod tests {
use crate::cache::HttpCache;
use crate::lsp::documents::Documents;
use crate::lsp::documents::LanguageId;
use crate::lsp::resolver::LspResolver;
use crate::lsp::search::tests::TestPackageSearchApi;
use deno_core::resolve_url;
use deno_graph::Range;
Expand All @@ -821,8 +820,7 @@ mod tests {
location.to_path_buf(),
crate::cache::RealDenoCacheEnv,
));
let resolver = Arc::new(LspResolver::new(cache.clone()));
let mut documents = Documents::new(cache, resolver);
let mut documents = Documents::new(cache);
for (specifier, source, version, language_id) in fixtures {
let specifier =
resolve_url(specifier).expect("failed to create specifier");
Expand Down
5 changes: 2 additions & 3 deletions cli/lsp/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,8 +1589,7 @@ mod tests {
location.to_path_buf(),
RealDenoCacheEnv,
));
let resolver = Arc::new(LspResolver::new(cache.clone()));
let mut documents = Documents::new(cache.clone(), resolver);
let mut documents = Documents::new(cache.clone());
for (specifier, source, version, language_id) in fixtures {
let specifier =
resolve_url(specifier).expect("failed to create specifier");
Expand All @@ -1612,7 +1611,7 @@ mod tests {
.unwrap();
config.tree.inject_config_file(config_file).await;
}
let resolver = LspResolver::new(cache.clone())
let resolver = LspResolver::default()
.with_new_config(&config, cache, None, None)
.await;
StateSnapshot {
Expand Down
11 changes: 5 additions & 6 deletions cli/lsp/documents.rs
View file
Open in desktop
Copy link
Member

@dsherret dsherret May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before this PR -> ./foo.js resolves to ./foo.d.ts
After this PR ->./foo.js resolves to ./foo.js

This will cause typescript to resolve the incorrect file I think? Sloppy imports is informed by the provided ResolutionMode that will resolve .d.ts for ResolutionMode::Types and .js for ResolutionMode::Execution. Probably we need to surface that up to a higher level instead of only always doing ResolutionMode::Types (see in resolve_unstable_sloppy_import).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm my thinking was that this matches the behavior without sloppy imports enabled. Without sloppy imports we don't resolve foo.js to foo.d.ts, since we don't actually do any special resolution for file specifiers. So it seems inconsistent to change that behavior for an import that isn't actually sloppy

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's different than Deno's default resolution. For sloppy imports it does TypeScript's Node resolution behaviour where it prefers .d.ts files when resolving the types, but .js files when executing the code.

Copy link
Member

@dsherret dsherret May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This actually might be a bug/design issue elsewhere as well. This is a fully resolved specifier so it's strange sloppy import resolution is happening here (in resolve_specifier). Probably it should only happen when resolving import specifiers (ex. code with (specifier: &str, base: &Url) params)

Copy link
Member

Choose a reason for hiding this comment

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

I did a commit that moves the sloppy import resolution from the loader to the resolver. This should fix the issue if you try it out now (and resolve to the .d.ts file).

This also seems to fix #23197 for me.

Original file line number Diff line number Diff line change
Expand Up @@ -855,14 +855,14 @@ pub struct Documents {
}

impl Documents {
pub fn new(cache: Arc<dyn HttpCache>, resolver: Arc<LspResolver>) -> Self {
pub fn new(cache: Arc<dyn HttpCache>) -> Self {
Self {
cache: cache.clone(),
config: Default::default(),
dirty: true,
open_docs: HashMap::default(),
file_system_docs: Default::default(),
resolver,
resolver: Default::default(),
npm_specifier_reqs: Default::default(),
has_injected_types_node_package: false,
}
Expand Down Expand Up @@ -1478,8 +1478,7 @@ mod tests {
location.to_path_buf(),
RealDenoCacheEnv,
));
let resolver = Arc::new(LspResolver::new(cache.clone()));
let documents = Documents::new(cache.clone(), resolver);
let documents = Documents::new(cache.clone());
(documents, location, cache)
}

Expand Down Expand Up @@ -1632,7 +1631,7 @@ console.log(b, "hello deno");
)
.await;

let resolver = LspResolver::new(cache.clone())
let resolver = LspResolver::default()
.with_new_config(&config, cache.clone(), None, None)
.await;
documents.update_config(
Expand Down Expand Up @@ -1681,7 +1680,7 @@ console.log(b, "hello deno");
)
.await;

let resolver = LspResolver::new(cache.clone())
let resolver = LspResolver::default()
.with_new_config(&config, cache.clone(), None, None)
.await;
documents.update_config(&config, &resolver, cache, &workspace_files);
Expand Down
5 changes: 2 additions & 3 deletions cli/lsp/language_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,8 +463,7 @@ impl Inner {
crate::cache::RealDenoCacheEnv,
));
let cache = global_cache.clone();
let resolver = Arc::new(LspResolver::new(cache.clone()));
let documents = Documents::new(cache.clone(), resolver.clone());
let documents = Documents::new(cache.clone());
let cache_metadata = cache::CacheMetadata::new(cache.clone());
let performance = Arc::new(Performance::default());
let config = Config::default();
Expand Down Expand Up @@ -501,7 +500,7 @@ impl Inner {
module_registry,
npm_search_api,
performance,
resolver,
resolver: Default::default(),
ts_fixable_diagnostics: Default::default(),
ts_server,
url_map: Default::default(),
Expand Down
27 changes: 12 additions & 15 deletions cli/lsp/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,11 @@ pub struct LspResolver {
unstable_sloppy_imports: bool,
}

impl LspResolver {
pub fn new(cache: Arc<dyn HttpCache>) -> Self {
let jsr_resolver = Arc::new(JsrCacheResolver::new(cache.clone(), None));
impl Default for LspResolver {
fn default() -> Self {
Self {
cache,
graph_resolver: create_graph_resolver(None, None, None),
jsr_resolver,
jsr_resolver: None,
npm_resolver: None,
node_resolver: None,
npm_config_hash: LspNpmConfigHash(0),
Expand All @@ -81,7 +79,9 @@ impl LspResolver {
unstable_sloppy_imports: false,
}
}
}

impl LspResolver {
pub async fn with_new_config(
&self,
config: &Config,
Expand Down Expand Up @@ -110,12 +110,11 @@ impl LspResolver {
npm_resolver.as_ref(),
node_resolver.as_ref(),
);
let jsr_resolver = Arc::new(JsrCacheResolver::new(
let jsr_resolver = Some(Arc::new(JsrCacheResolver::new(
cache.clone(),
config_data.and_then(|d| d.lockfile.clone()),
));
let redirect_resolver =
Some(Arc::new(RedirectResolver::new(cache.clone())));
)));
let redirect_resolver = Some(Arc::new(RedirectResolver::new(cache)));
let graph_imports = config_data
.and_then(|d| d.config_file.as_ref())
.and_then(|cf| cf.to_maybe_imports().ok())
Expand All @@ -138,7 +137,6 @@ impl LspResolver {
})
.unwrap_or_default();
Arc::new(Self {
cache,
graph_resolver,
jsr_resolver,
npm_resolver,
Expand All @@ -164,7 +162,6 @@ impl LspResolver {
node_resolver.as_ref(),
);
Arc::new(Self {
cache: self.cache.clone(),
graph_resolver,
jsr_resolver: self.jsr_resolver.clone(),
npm_resolver,
Expand All @@ -178,7 +175,7 @@ impl LspResolver {
}

pub fn did_cache(&self) {
self.jsr_resolver.did_cache()
self.jsr_resolver.as_ref().inspect(|r| r.did_cache());
}

pub async fn set_npm_package_reqs(
Expand Down Expand Up @@ -208,19 +205,19 @@ impl LspResolver {
&self,
req_ref: &JsrPackageReqReference,
) -> Option<ModuleSpecifier> {
self.jsr_resolver.jsr_to_registry_url(req_ref)
self.jsr_resolver.as_ref()?.jsr_to_registry_url(req_ref)
}

pub fn jsr_lookup_export_for_path(
&self,
nv: &PackageNv,
path: &str,
) -> Option<String> {
self.jsr_resolver.lookup_export_for_path(nv, path)
self.jsr_resolver.as_ref()?.lookup_export_for_path(nv, path)
}

pub fn jsr_lookup_req_for_nv(&self, nv: &PackageNv) -> Option<PackageReq> {
self.jsr_resolver.lookup_req_for_nv(nv)
self.jsr_resolver.as_ref()?.lookup_req_for_nv(nv)
}

pub fn maybe_managed_npm_resolver(&self) -> Option<&ManagedCliNpmResolver> {
Expand Down
11 changes: 4 additions & 7 deletions cli/lsp/tsc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use crate::cache::HttpCache;
use crate::lsp::cache::CacheMetadata;
use crate::lsp::documents::Documents;
use crate::lsp::logging::lsp_warn;
use crate::lsp::resolver::LspResolver;
use crate::tsc;
use crate::tsc::ResolveArgs;
use crate::tsc::MISSING_DEPENDENCY_SPECIFIER;
Expand Down Expand Up @@ -4408,15 +4407,14 @@ deno_core::extension!(deno_tsc,
specifier_map: Arc<TscSpecifierMap>,
},
state = |state, options| {
let resolver = Arc::new(LspResolver::new(options.cache.clone()));
state.put(State::new(
Arc::new(StateSnapshot {
project_version: 0,
assets: Default::default(),
cache_metadata: CacheMetadata::new(options.cache.clone()),
config: Default::default(),
documents: Documents::new(options.cache.clone(), resolver.clone()),
resolver,
documents: Documents::new(options.cache.clone()),
resolver: Default::default(),
}),
options.specifier_map,
options.performance,
Expand Down Expand Up @@ -5085,8 +5083,7 @@ mod tests {
location.to_path_buf(),
RealDenoCacheEnv,
));
let resolver = Arc::new(LspResolver::new(cache.clone()));
let mut documents = Documents::new(cache.clone(), resolver);
let mut documents = Documents::new(cache.clone());
for (specifier, source, version, language_id) in fixtures {
let specifier =
resolve_url(specifier).expect("failed to create specifier");
Expand All @@ -5112,7 +5109,7 @@ mod tests {
.unwrap(),
)
.await;
let resolver = LspResolver::new(cache.clone())
let resolver = LspResolver::default()
.with_new_config(&config, cache.clone(), None, None)
.await;
StateSnapshot {
Expand Down
Loading