Skip to content

fix(lsp): inherit missing fmt and lint config from parent scopes #23547

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

Merged
bartlomieju merged 1 commit into from
Apr 24, 2024
Merged

fix(lsp): inherit missing fmt and lint config from parent scopes #23547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
154 changes: 102 additions & 52 deletions cli/lsp/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,7 @@ impl ConfigData {
async fn load(
config_file_specifier: Option<&ModuleSpecifier>,
scope: &ModuleSpecifier,
parent: Option<(&ModuleSpecifier, &ConfigData)>,
settings: &Settings,
file_fetcher: Option<&FileFetcher>,
) -> Self {
Expand All @@ -1182,8 +1183,14 @@ impl ConfigData {
" Resolved Deno configuration file: \"{}\"",
config_file.specifier.as_str()
);
Self::load_inner(Some(config_file), scope, settings, file_fetcher)
.await
Self::load_inner(
Some(config_file),
scope,
parent,
settings,
file_fetcher,
)
.await
}
Err(err) => {
lsp_warn!(
Expand All @@ -1192,7 +1199,7 @@ impl ConfigData {
err
);
let mut data =
Self::load_inner(None, scope, settings, file_fetcher).await;
Self::load_inner(None, scope, parent, settings, file_fetcher).await;
data
.watched_files
.insert(specifier.clone(), ConfigWatchedFileType::DenoJson);
Expand All @@ -1210,13 +1217,14 @@ impl ConfigData {
}
}
} else {
Self::load_inner(None, scope, settings, file_fetcher).await
Self::load_inner(None, scope, parent, settings, file_fetcher).await
}
}

async fn load_inner(
config_file: Option<ConfigFile>,
scope: &ModuleSpecifier,
parent: Option<(&ModuleSpecifier, &ConfigData)>,
settings: &Settings,
file_fetcher: Option<&FileFetcher>,
) -> Self {
Expand All @@ -1238,45 +1246,76 @@ impl ConfigData {
.or_insert(ConfigWatchedFileType::DenoJson);
}

// Resolve some config file fields ahead of time
let fmt_options = config_file
.as_ref()
.and_then(|config_file| {
config_file
.to_fmt_config()
.and_then(|o| {
let base_path = config_file
.specifier
.to_file_path()
.map_err(|_| anyhow!("Invalid base path."))?;
FmtOptions::resolve(o, None, &base_path)
})
.inspect_err(|err| {
lsp_warn!(" Couldn't read formatter configuration: {}", err)
})
.ok()
})
.unwrap_or_default();
let lint_options = config_file
.as_ref()
.and_then(|config_file| {
config_file
.to_lint_config()
.and_then(|o| {
let base_path = config_file
.specifier
.to_file_path()
.map_err(|_| anyhow!("Invalid base path."))?;
LintOptions::resolve(o, None, &base_path)
})
.inspect_err(|err| {
lsp_warn!(" Couldn't read lint configuration: {}", err)
})
.ok()
})
.unwrap_or_default();
let lint_rules =
get_configured_rules(lint_options.rules.clone(), config_file.as_ref());
let mut fmt_options = None;
if let Some((_, parent_data)) = parent {
let has_own_fmt_options = config_file
.as_ref()
.is_some_and(|config_file| config_file.json.fmt.is_some());
if !has_own_fmt_options {
fmt_options = Some(parent_data.fmt_options.clone())
}
}
let fmt_options = fmt_options.unwrap_or_else(|| {
config_file
.as_ref()
.and_then(|config_file| {
config_file
.to_fmt_config()
.and_then(|o| {
let base_path = config_file
.specifier
.to_file_path()
.map_err(|_| anyhow!("Invalid base path."))?;
FmtOptions::resolve(o, None, &base_path)
})
.inspect_err(|err| {
lsp_warn!(" Couldn't read formatter configuration: {}", err)
})
.ok()
})
.map(Arc::new)
.unwrap_or_default()
});

let mut lint_options_rules = None;
if let Some((_, parent_data)) = parent {
let has_own_lint_options = config_file
.as_ref()
.is_some_and(|config_file| config_file.json.lint.is_some());
if !has_own_lint_options {
lint_options_rules = Some((
parent_data.lint_options.clone(),
parent_data.lint_rules.clone(),
))
}
}
let (lint_options, lint_rules) = lint_options_rules.unwrap_or_else(|| {
let lint_options = config_file
.as_ref()
.and_then(|config_file| {
config_file
.to_lint_config()
.and_then(|o| {
let base_path = config_file
.specifier
.to_file_path()
.map_err(|_| anyhow!("Invalid base path."))?;
LintOptions::resolve(o, None, &base_path)
})
.inspect_err(|err| {
lsp_warn!(" Couldn't read lint configuration: {}", err)
})
.ok()
})
.map(Arc::new)
.unwrap_or_default();
let lint_rules = Arc::new(get_configured_rules(
lint_options.rules.clone(),
config_file.as_ref(),
));
(lint_options, lint_rules)
});

let vendor_dir = config_file.as_ref().and_then(|c| c.vendor_dir_path());

// Load lockfile
Expand Down Expand Up @@ -1449,9 +1488,9 @@ impl ConfigData {

ConfigData {
config_file: config_file.map(Arc::new),
fmt_options: Arc::new(fmt_options),
lint_options: Arc::new(lint_options),
lint_rules: Arc::new(lint_rules),
fmt_options,
lint_options,
lint_rules,
ts_config: Arc::new(ts_config),
byonm,
node_modules_dir,
Expand Down Expand Up @@ -1601,6 +1640,7 @@ impl ConfigTree {
ConfigData::load(
Some(&config_uri),
folder_uri,
None,
settings,
Some(file_fetcher),
)
Expand All @@ -1616,17 +1656,20 @@ impl ConfigTree {
|| specifier.path().ends_with("/deno.jsonc")
{
if let Ok(scope) = specifier.join(".") {
let entry = scopes.entry(scope.clone());
#[allow(clippy::map_entry)]
if matches!(entry, std::collections::btree_map::Entry::Vacant(_)) {
if !scopes.contains_key(&scope) {
let parent = scopes
.iter()
.rev()
.find(|(s, _)| scope.as_str().starts_with(s.as_str()));
let data = ConfigData::load(
Some(specifier),
&scope,
parent,
settings,
Some(file_fetcher),
)
.await;
entry.or_insert(data);
scopes.insert(scope, data);
}
}
}
Expand All @@ -1639,8 +1682,14 @@ impl ConfigTree {
{
scopes.insert(
folder_uri.clone(),
ConfigData::load(None, folder_uri, settings, Some(file_fetcher))
.await,
ConfigData::load(
None,
folder_uri,
None,
settings,
Some(file_fetcher),
)
.await,
);
}
}
Expand All @@ -1654,6 +1703,7 @@ impl ConfigTree {
let data = ConfigData::load_inner(
Some(config_file),
&scope,
None,
&Default::default(),
None,
)
Expand Down
76 changes: 76 additions & 0 deletions tests/integration/lsp_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11468,6 +11468,8 @@ fn lsp_deno_json_scopes_fmt_config() {
})
.to_string(),
);
temp_dir.create_dir_all("project2/project3");
temp_dir.write("project2/project3/deno.json", json!({}).to_string());
let mut client = context.new_lsp_command().build();
client.initialize_default();
client.did_open(json!({
Expand Down Expand Up @@ -11530,6 +11532,38 @@ fn lsp_deno_json_scopes_fmt_config() {
"newText": "''",
}])
);
// `project2/project3/file.ts` should use the fmt settings from
// `project2/deno.json`, since `project2/project3/deno.json` has no fmt field.
client.did_open(json!({
"textDocument": {
"uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(),
"languageId": "typescript",
"version": 1,
"text": "console.log(\"\");\n",
},
}));
let res = client.write_request(
"textDocument/formatting",
json!({
"textDocument": {
"uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(),
},
"options": {
"tabSize": 2,
"insertSpaces": true,
},
}),
);
assert_eq!(
res,
json!([{
"range": {
"start": { "line": 0, "character": 12 },
"end": { "line": 0, "character": 14 },
},
"newText": "''",
}])
);
client.shutdown();
}

Expand Down Expand Up @@ -11561,6 +11595,8 @@ fn lsp_deno_json_scopes_lint_config() {
})
.to_string(),
);
temp_dir.create_dir_all("project2/project3");
temp_dir.write("project2/project3/deno.json", json!({}).to_string());
let mut client = context.new_lsp_command().build();
client.initialize_default();
let diagnostics = client.did_open(json!({
Expand Down Expand Up @@ -11629,6 +11665,46 @@ fn lsp_deno_json_scopes_lint_config() {
"version": 1,
})
);
client.write_notification(
"textDocument/didClose",
json!({
"textDocument": {
"uri": temp_dir.uri().join("project2/file.ts").unwrap(),
},
}),
);
// `project2/project3/file.ts` should use the lint settings from
// `project2/deno.json`, since `project2/project3/deno.json` has no lint
// field.
let diagnostics = client.did_open(json!({
"textDocument": {
"uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(),
"languageId": "typescript",
"version": 1,
"text": r#"
// TODO: Unused var
const snake_case_var = 1;
console.log(snake_case_var);
"#,
},
}));
assert_eq!(
json!(diagnostics.messages_with_source("deno-lint")),
json!({
"uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(),
"diagnostics": [{
"range": {
"start": { "line": 1, "character": 8 },
"end": { "line": 1, "character": 27 },
},
"severity": 2,
"code": "ban-untagged-todo",
"source": "deno-lint",
"message": "TODO should be tagged with (@username) or (#issue)\nAdd a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123)",
}],
"version": 1,
})
);
client.shutdown();
}

Expand Down