chore: add clippy

.bazelrc

@@ -7,10 +7,19 @@ build --experimental_convenience_symlinks=ignore
 
 build --@rules_rust//rust/toolchain/channel=nightly
 
+# Check rustfmt
 build --@rules_rust//:rustfmt.toml=//:rustfmt.toml
 build --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
 build --output_groups=+rustfmt_checks
 
+# Check clippy
+build --aspects=@rules_rust//rust:defs.bzl%rust_clippy_aspect
+build --output_groups=+clippy_checks
+build --@rules_rust//:clippy.toml=//:clippy.toml
+
+# Fail on warnings
+build --action_env=RUSTFLAGS=-Dwarnings
+
 build --incompatible_strict_action_env
 
 test --test_output=errors

BUILD.bazel

@@ -1,7 +1,10 @@
 load("@rules_rust//proto/prost:defs.bzl", "rust_prost_toolchain")
 load("@rules_rust//rust:defs.bzl", "rust_library_group")
 
-exports_files(["rustfmt.toml"])
+exports_files([
+ "clippy.toml",
+ "rustfmt.toml",
+])
 
 rust_library_group(
  name = "prost_runtime",

crates/starpls/src/convert.rs

@@ -18,7 +18,7 @@ pub(crate) fn lsp_diagnostic_from_native(
  line_index: &LineIndex,
 ) -> Option<lsp_types::Diagnostic> {
  Some(lsp_types::Diagnostic {
- range: lsp_range_from_text_range(diagnostic.range.range, &line_index)?,
+ range: lsp_range_from_text_range(diagnostic.range.range, line_index)?,
  severity: Some(lsp_severity_from_native(diagnostic.severity)),
  code: None,
  code_description: None,

crates/starpls/src/debouncer.rs

@@ -20,7 +20,7 @@ impl AnalysisDebouncer {
  loop {
  if active {
  match source_rx.recv_timeout(duration) {
- Ok(file_ids) => pending_file_ids.extend(file_ids.into_iter()),
+ Ok(file_ids) => pending_file_ids.extend(file_ids),
  Err(RecvTimeoutError::Disconnected) => break,
  Err(RecvTimeoutError::Timeout) => {
  sink.send(Task::AnalysisRequested(pending_file_ids.drain().collect()))
@@ -32,7 +32,7 @@ impl AnalysisDebouncer {
  match source_rx.recv() {
  Ok(file_ids) => {
  active = true;
- pending_file_ids.extend(file_ids.into_iter());
+ pending_file_ids.extend(file_ids);
  }
  Err(RecvError) => break,
  }

crates/starpls/src/document.rs

@@ -121,7 +121,7 @@ impl DocumentManager {
  }
 
  pub(crate) fn close(&mut self, path: &PathBuf) {
- if let Some(file_id) = self.path_interner.lookup_by_path_buf(&path) {
+ if let Some(file_id) = self.path_interner.lookup_by_path_buf(path) {
  self.has_closed_or_opened_documents = true;
  if let Some(document) = self.documents.get_mut(&file_id) {
  document.source = DocumentSource::Disk;
@@ -284,9 +284,7 @@ impl DefaultFileLoader {
  canonical_repo_res = Some(label.repo().to_string());
  }
 
- if self.workspace_name.as_ref().map(|name| name.as_str()) == Some(label.repo())
- || label.repo().is_empty()
- {
+ if self.workspace_name.as_deref() == Some(label.repo()) || label.repo().is_empty() {
  (self.workspace.clone(), PathBuf::new())
  } else {
  (self.external_output_base.join(label.repo()), PathBuf::new())
@@ -295,7 +293,7 @@ impl DefaultFileLoader {
  RepoKind::Current => {
  // Find the Bazel workspace root.
  let from_path = self.interner.lookup_by_file_id(from);
- match starpls_bazel::resolve_workspace(&from_path)? {
+ match starpls_bazel::resolve_workspace(from_path)? {
  Some(root) => root,
  None => {
  bail!("not in a Bazel workspace")
@@ -358,7 +356,7 @@ impl DefaultFileLoader {
  Ok((file_id, contents))
  }
 
- fn repo_for_path<'a>(&'a self, path: &'a PathBuf) -> Option<&str> {
+ fn repo_for_path<'a>(&'a self, path: &'a Path) -> Option<&str> {
  match path.strip_prefix(&self.external_output_base) {
  Ok(stripped) => stripped
  .components()
@@ -498,10 +496,8 @@ impl FileLoader for DefaultFileLoader {
  let from_dir = from_path.parent().unwrap();
  let has_trailing_slash = path.ends_with(MAIN_SEPARATOR);
  let mut path = from_dir.join(path);
- if !has_trailing_slash {
- if !path.pop() {
- return Ok(None);
- }
+ if !has_trailing_slash && !path.pop() {
+ return Ok(None);
  }
 
  let path = path.canonicalize()?;
@@ -554,7 +550,6 @@ impl FileLoader for DefaultFileLoader {
  ),
  RepoKind::Canonical | RepoKind::Apparent => Some(
  fs::read_dir(&self.external_output_base)?
- .into_iter()
  .filter_map(|entry| {
  let entry = entry.ok()?;
  entry.file_type().ok()?.is_dir().then(|| LoadItemCandidate {
@@ -586,8 +581,7 @@ impl FileLoader for DefaultFileLoader {
  } else {
  self.external_output_base.join(canonical_repo)
  }
- } else if self.workspace_name.as_ref().map(|name| name.as_str())
- == Some(label.repo())
+ } else if self.workspace_name.as_deref() == Some(label.repo())
  || label.repo().is_empty()
  {
  self.workspace.clone()
@@ -653,7 +647,7 @@ fn read_dir_packages_and_targets(
  has_trailing_slash: bool,
 ) -> anyhow::Result<Vec<LoadItemCandidate>> {
  Ok(fs::read_dir(path)?
- .flat_map(|entry| entry)
+ .flatten()
  .filter_map(|entry| {
  entry
  .file_type()
@@ -689,7 +683,7 @@ fn read_dir_packages_and_targets(
 
 fn read_dir_targets(path: impl AsRef<Path>) -> anyhow::Result<Vec<LoadItemCandidate>> {
  Ok(fs::read_dir(path)?
- .flat_map(|entry| entry)
+ .flatten()
  .filter_map(|entry| {
  entry
  .file_type()

crates/starpls/src/event_loop.rs

@@ -320,7 +320,7 @@ fn collect_diagnostics(
  Some(
  diagnostics
  .into_iter()
- .flat_map(|diagnostic| convert::lsp_diagnostic_from_native(diagnostic, &line_index))
+ .flat_map(|diagnostic| convert::lsp_diagnostic_from_native(diagnostic, line_index))
  .collect::<Vec<_>>(),
  )
 }

crates/starpls/src/handlers/requests.rs

@@ -62,7 +62,7 @@ pub(crate) fn goto_definition(
  snapshot
  .analysis_snapshot
  .goto_definition(FilePosition { file_id, pos })?
- .unwrap_or_else(|| Vec::new())
+ .unwrap_or_else(Vec::new)
  .into_iter(),
  );
  Ok(Some(resp))
@@ -88,7 +88,7 @@ pub(crate) fn completion(
  FilePosition { file_id, pos },
  params.context.and_then(|cx| cx.trigger_character),
  )?
- .unwrap_or_else(|| Vec::new())
+ .unwrap_or_else(Vec::new)
  .into_iter()
  .flat_map(|item| {
  let sort_text = Some(item.sort_text());

crates/starpls/src/main.rs

@@ -71,7 +71,7 @@ fn run_server(args: ServerArgs) -> anyhow::Result<()> {
 
  // Initialize the connection with server capabilities. For now, this consists
  // only of `TextDocumentSyncKind.Full`.
- let server_capabilities = serde_json::to_value(&ServerCapabilities {
+ let server_capabilities = serde_json::to_value(ServerCapabilities {
  completion_provider: Some(CompletionOptions {
  trigger_characters: Some(make_trigger_characters(COMPLETION_TRIGGER_CHARACTERS)),
  ..Default::default()

crates/starpls/src/server.rs

@@ -326,7 +326,7 @@ impl Server {
  let files = mem::take(&mut self.pending_files);
  let bazel_client = self.bazel_client.clone();
  self.is_fetching_repos = true;
- self.fetched_repos.extend(repos.clone().into_iter());
+ self.fetched_repos.extend(repos.clone());
  self.task_pool_handle.spawn_with_sender(move |sender| {
  sender
  .send(Task::FetchExternalRepos(FetchExternalReposProgress::Begin(

crates/starpls/src/utils.rs

@@ -117,7 +117,7 @@ where
  origin_selection_range: origin_selection_range.and_then(|range| {
  convert::lsp_range_from_text_range(range, source_line_index)
  }),
- target_range: range.clone()?,
+ target_range: range?,
  target_selection_range: range?,
  target_uri: lsp_types::Url::from_file_path(
  snapshot

crates/starpls_bazel/src/build_language.rs

@@ -7,7 +7,7 @@ use crate::{
 };
 
 pub fn decode_rules(build_language_output: &[u8]) -> anyhow::Result<Builtins> {
- let build_language = BuildLanguage::decode(&build_language_output[..])?;
+ let build_language = BuildLanguage::decode(build_language_output)?;
  Ok(Builtins {
  global: build_language
  .rule
@@ -21,11 +21,11 @@ pub fn decode_rules(build_language_output: &[u8]) -> anyhow::Result<Builtins> {
  }| {
  Value {
  name,
- doc: documentation.unwrap_or_else(|| String::new()),
+ doc: documentation.unwrap_or_else(String::new),
  callable: Some(Callable {
  param: attribute
  .into_iter()
- .filter(|attr| !attr.name.starts_with(&['$', ':']))
+ .filter(|attr| !attr.name.starts_with(['$', ':']))
  .map(|attr| {
  let doc = attr.documentation().to_string();
  let r#type =

crates/starpls_bazel/src/env.rs

@@ -32,12 +32,12 @@ struct ValueJson {
  callable: Option<CallableJson>,
 }
 
-impl Into<Value> for ValueJson {
- fn into(self) -> Value {
+impl From<ValueJson> for Value {
+ fn from(val: ValueJson) -> Self {
  Value {
- name: self.name,
- doc: self.doc,
- callable: self.callable.map(|callable| Callable {
+ name: val.name,
+ doc: val.doc,
+ callable: val.callable.map(|callable| Callable {
  param: callable
  .params
  .into_iter()

crates/starpls_bazel/src/lib.rs

@@ -138,7 +138,7 @@ pub fn load_builtins(path: impl AsRef<Path>) -> anyhow::Result<Builtins> {
 }
 
 pub fn decode_builtins(data: &[u8]) -> anyhow::Result<Builtins> {
- let builtins = Builtins::decode(&data[..])?;
+ let builtins = Builtins::decode(data)?;
  Ok(builtins)
 }
 

crates/starpls_common/src/lib.rs

@@ -140,7 +140,7 @@ impl Parse {
 
 #[salsa::tracked]
 pub fn parse(db: &dyn Db, file: File) -> Parse {
- let parse = parse_module(&file.contents(db), &mut |err| {
+ let parse = parse_module(file.contents(db), &mut |err| {
  Diagnostics::push(
  db,
  Diagnostic {
@@ -164,7 +164,7 @@ struct LineIndexResult {
 
 #[salsa::tracked]
 fn line_index_query(db: &dyn Db, file: File) -> LineIndexResult {
- let line_index = syntax_line_index(&file.contents(db));
+ let line_index = syntax_line_index(file.contents(db));
  LineIndexResult::new(db, line_index)
 }
 

crates/starpls_hir/src/api.rs

@@ -179,7 +179,7 @@ impl LoadItem {
  pub fn name(&self, db: &dyn Db) -> Name {
  match &module(db, self.file).load_items[self.id] {
  def::LoadItem::Direct { name, .. } | def::LoadItem::Aliased { name, .. } => {
- Name::from_str(&name)
+ Name::from_str(name)
  }
  }
  }
@@ -344,7 +344,7 @@ impl Type {
  match self.ty.kind() {
  TyKind::BuiltinFunction(func) => Some(func.doc(db).clone()),
  TyKind::BuiltinType(ty, _) => Some(ty.doc(db).clone()),
- TyKind::Function(func) => return func.doc(db).map(|doc| doc.to_string()),
+ TyKind::Function(func) => func.doc(db).map(|doc| doc.to_string()),
  TyKind::IntrinsicFunction(func, _) => Some(func.doc(db).clone()),
  TyKind::Rule(rule) => rule.doc.as_ref().map(Box::to_string),
  TyKind::Provider(provider) => provider.doc(db),
@@ -434,7 +434,7 @@ impl Type {
  pub fn try_as_inline_struct(&self) -> Option<Struct> {
  match self.ty.kind() {
  TyKind::Struct(strukt) => strukt.as_ref().and_then(|strukt| match strukt {
- &typeck::Struct::Inline { ref call_expr, .. } => Some(Struct {
+ typeck::Struct::Inline { call_expr, .. } => Some(Struct {
  call_expr: call_expr.clone(),
  }),
  _ => None,
@@ -484,7 +484,7 @@ impl Callable {
  )
  .intern(),
  CallableInner::BuiltinFunction(func) => TyKind::BuiltinFunction(func).intern(),
- CallableInner::Rule(ref ty) => ty.clone().into(),
+ CallableInner::Rule(ref ty) => ty.clone(),
  CallableInner::Provider(ref provider) => TyKind::Provider(provider.clone()).intern(),
  CallableInner::ProviderRawConstructor(ref name, ref provider) => {
  TyKind::ProviderRawConstructor(name.clone(), provider.clone()).intern()

crates/starpls_hir/src/def.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashSet, ops::Index};
+use std::{collections::HashSet, fmt, ops::Index};
 
 use either::Either;
 use id_arena::{Arena, Id};
@@ -238,11 +238,7 @@ impl Expr {
  }
  }
 
- fn walk_comp_clause_expressions(
- &self,
- comp_clauses: &Box<[CompClause]>,
- mut f: impl FnMut(ExprId),
- ) {
+ fn walk_comp_clause_expressions(&self, comp_clauses: &[CompClause], mut f: impl FnMut(ExprId)) {
  for comp_clause in comp_clauses.iter() {
  match comp_clause {
  CompClause::For { iterable, targets } => {
@@ -443,10 +439,6 @@ impl Name {
  self.0.as_str()
  }
 
- pub fn to_string(&self) -> String {
- self.0.to_string()
- }
-
  pub(crate) fn new_inline(name: &'static str) -> Self {
  Self::new(SmolStr::new_inline(name))
  }
@@ -456,6 +448,12 @@ impl Name {
  }
 }
 
+impl fmt::Display for Name {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "{}", self.0)
+ }
+}
+
 #[salsa::interned]
 pub(crate) struct LiteralString {
  pub(crate) value: Box<str>,

crates/starpls_hir/src/def/codeflow.rs

@@ -89,6 +89,8 @@ impl<'a> CodeFlowLowerCtx<'a> {
  }
 
  fn lower_stmt(&mut self, stmt: StmtId) {
+ // Enable when collapsible matching is not experimental: https://github.com/rust-lang/rust/issues/51114
+ #![allow(clippy::collapsible_match)]
  match &self.module[stmt] {
  Stmt::Assign { lhs, rhs, .. } => {
  self.lower_assignment_target(*lhs, *rhs);
@@ -122,7 +124,7 @@ impl<'a> CodeFlowLowerCtx<'a> {
  }
  Some(Either::Right(else_stmts)) => {
  self.curr_node = pre_if_node;
- self.lower_stmts(&else_stmts);
+ self.lower_stmts(else_stmts);
  self.push_antecedent(post_if_node, self.curr_node);
  }
  _ => {
@@ -251,7 +253,7 @@ impl<'a> CodeFlowLowerCtx<'a> {
  }
 
  fn lower_comp_clauses(&mut self, comp_clauses: &[CompClause]) {
- for comp_clause in comp_clauses.into_iter() {
+ for comp_clause in comp_clauses.iter() {
  match comp_clause {
  CompClause::For { iterable, targets } => {
  self.lower_expr(*iterable);