Delete `RcEq<T>` and instead provide `impl PartialEq for RsSnippet`. `salsa` requires that all query keys and query results can be compared for equality. Key equality is needed for basic `salsa` functionality (memoization of previous computations). Result equality is needed to determine that a changed input doesn't affect the result of a given computation (and therefore computations transitively depending on the result don't need to be redone). Before this CL, equality of some query results was provided by `RcEq<T>` in Crubit's `salsa_utils.rs` (inspired by Chalk's `ArcEq`). This worked but pointer-based-equality would unnecessary treat equivalent results as different. This doesn't matter in practice, because Crubit never resets `salsa` inputs (i.e. only calls `set_ir` once) and so `salsa` would never need to check equality of `RcEq`. Still, having real, non-pointer-based equality should help with incremental computations in the future (if they ever become used by Crubit). And deleting `salsa_utils.rs` also means the code is smaller and easier to understand. PiperOrigin-RevId: 476146950
diff --git a/rs_bindings_from_cc/BUILD b/rs_bindings_from_cc/BUILD index 4692df2..357a593 100644 --- a/rs_bindings_from_cc/BUILD +++ b/rs_bindings_from_cc/BUILD
@@ -363,15 +363,6 @@ ], ) -rust_library( - name = "salsa_utils", - srcs = ["salsa_utils.rs"], - deps = [ - "@crate_index//:proc-macro2", - "@crate_index//:quote", - ], -) - cc_library( name = "src_code_gen", srcs = ["src_code_gen.cc"], @@ -393,7 +384,6 @@ srcs = ["src_code_gen.rs"], deps = [ ":ir", - ":salsa_utils", "//common:arc_anyhow", "//common:ffi_types", "//common:token_stream_printer",
diff --git a/rs_bindings_from_cc/salsa_utils.rs b/rs_bindings_from_cc/salsa_utils.rs deleted file mode 100644 index 9b50951..0000000 --- a/rs_bindings_from_cc/salsa_utils.rs +++ /dev/null
@@ -1,61 +0,0 @@ -// Part of the Crubit project, under the Apache License v2.0 with LLVM -// Exceptions. See /LICENSE for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -//! Copyable, equality-comparable types for Salsa. -//! -//! TODO(jeanpierreda): give this module a better name. - -use proc_macro2::TokenStream; -use quote::ToTokens; -use std::ops::Deref; -use std::rc::Rc; - -/// A wrapper for a smart pointer, which implements `Eq` as pointer equality. -/// -/// This was directly inspired by Chalk's `ArcEq`, which does the same. -#[derive(Debug, Default)] -#[repr(transparent)] -pub struct RcEq<T>(pub Rc<T>); - -impl<T> Clone for RcEq<T> { - fn clone(&self) -> Self { - Self(Rc::clone(&self.0)) - } -} - -impl<T> PartialEq<RcEq<T>> for RcEq<T> { - fn eq(&self, other: &Self) -> bool { - std::ptr::eq(&*self.0, &*other.0) - } -} - -impl<T> Eq for RcEq<T> {} - -impl<T> Deref for RcEq<T> { - type Target = T; - fn deref(&self) -> &T { - &*self.0 - } -} - -impl<T> From<T> for RcEq<T> { - fn from(x: T) -> Self { - RcEq::new(x) - } -} - -impl<T> RcEq<T> { - pub fn new(x: T) -> Self { - RcEq(Rc::new(x)) - } -} - -impl<T> ToTokens for RcEq<T> -where - T: ToTokens, -{ - fn to_tokens(&self, tokens: &mut TokenStream) { - self.0.as_ref().to_tokens(tokens) - } -}
diff --git a/rs_bindings_from_cc/src_code_gen.rs b/rs_bindings_from_cc/src_code_gen.rs index 60e874c..ac8f005 100644 --- a/rs_bindings_from_cc/src_code_gen.rs +++ b/rs_bindings_from_cc/src_code_gen.rs
@@ -9,7 +9,6 @@ use once_cell::sync::Lazy; use proc_macro2::{Ident, Literal, TokenStream}; use quote::{format_ident, quote, ToTokens}; -use salsa_utils::RcEq; use std::collections::{BTreeSet, HashMap, HashSet}; use std::ffi::{OsStr, OsString}; use std::fmt::Write as _; @@ -88,7 +87,7 @@ fn generate_func( &self, func: Rc<Func>, - ) -> Result<Option<RcEq<(RsSnippet, RsSnippet, Rc<FunctionId>)>>>; + ) -> Result<Option<Rc<(RsSnippet, RsSnippet, Rc<FunctionId>)>>>; fn overloaded_funcs(&self) -> Rc<HashSet<Rc<FunctionId>>>; } @@ -183,6 +182,32 @@ } } +impl Eq for RsSnippet {} + +impl PartialEq for RsSnippet { + fn eq(&self, other: &Self) -> bool { + fn to_comparable_tuple(_x: &RsSnippet) -> (&BTreeSet<Ident>, String) { + // TokenStream doesn't implement `PartialEq`, so we convert to an equivalent + // `String`. This is a bit expensive, but should be okay (especially + // given that this code doesn't execute at this point). Having a + // working `impl PartialEq` helps `salsa` reuse unchanged memoized + // results of previous computations (although this is a bit + // theoretical, since right now we don't re-set `salsa`'s inputs - we only call + // `set_ir` once). + // + // TODO(lukasza): If incremental `salsa` computations are ever used in the + // future, we may end up hitting the `panic!` below. At that point + // it should be okay to just remove the `panic!`, but we should also + // 1) think about improving performance of comparing `TokenStream` + // for equality and 2) add unit tests covering this `PartialEq` `impl`. + panic!("This code is not expected to execute in practice"); + #[allow(unreachable_code)] + (&_x.features, _x.tokens.to_string()) + } + to_comparable_tuple(self) == to_comparable_tuple(other) + } +} + /// If we know the original C++ function is codegenned and already compatible /// with `extern "C"` calling convention we skip creating/calling the C++ thunk /// since we can call the original C++ directly. @@ -983,7 +1008,7 @@ fn generate_func( db: &dyn BindingsGenerator, func: Rc<Func>, -) -> Result<Option<RcEq<(RsSnippet, RsSnippet, Rc<FunctionId>)>>> { +) -> Result<Option<Rc<(RsSnippet, RsSnippet, Rc<FunctionId>)>>> { let ir = db.ir(); let mut features = BTreeSet::new(); let mut param_types = func @@ -1240,7 +1265,7 @@ } } - Ok(Some(RcEq::new(( + Ok(Some(Rc::new(( RsSnippet { features, tokens: api_func }, thunk.into(), Rc::new(function_id),