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.

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

#[tracked] emitted accumulated uses non_local_definitions #607

Open
Veykril opened this issue Oct 25, 2024 · 4 comments
Open

#[tracked] emitted accumulated uses non_local_definitions #607

Veykril opened this issue Oct 25, 2024 · 4 comments
Labels
bikeshed 🚴‍♀️ Debating API details and the like

Comments

@Veykril
Copy link
Contributor

Veykril commented Oct 25, 2024

Meaning rust-analyzer is unable to figure out what accumulated is, which is bad if we want to use salsa in our codebase. The trick used here is mainly to get around lack of defsite hygiene, as otherwise a user couldn't create a tracked function named accumulated. Given salsa already forbids certain field names i feel like it would be alright to forbid the accumulated function name as well. That simplifies a couple of things for the tracked macro in general I'd say.

@Veykril Veykril added the bikeshed 🚴‍♀️ Debating API details and the like label Oct 25, 2024
@nikomatsakis
Copy link
Member

Hmm. It makes me grouchy how bad our (meaning Rust's) support is for hygiene. Can you point at what the "non-local definition" is in particular?

@Veykril
Copy link
Contributor Author

Veykril commented Oct 25, 2024

It's for making this test work

//! Demonstrates the workaround of wrapping calls to
//! `accumulated` in a tracked function to get better
//! reuse.

The macros basically create a fn_name uninhabited struct and implement an inherent function on that (within a local item scope to access some locally defined context names). Specifically here

#[allow(non_local_definitions)]
impl $fn_name {
pub fn accumulated<$db_lt, A: salsa::Accumulator>(
$db: &$db_lt dyn $Db,
$($input_id: $input_ty,)*
) -> Vec<A> {
use salsa::plumbing as $zalsa;
let key = $zalsa::macro_if! {
if $needs_interner {
$Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*))
} else {
$zalsa::AsId::as_id(&($($input_id),*))
}
};
$Configuration::fn_ingredient($db).accumulated_by::<A>($db, key)
}
$zalsa::macro_if! { $is_specifiable =>
pub fn specify<$db_lt>(
$db: &$db_lt dyn $Db,
$($input_id: $input_ty,)*
value: $output_ty,
) {
let key = $zalsa::AsId::as_id(&($($input_id),*));
$Configuration::fn_ingredient($db).specify_and_record(
$db,
key,
value,
)
}
}
$zalsa::macro_if! { if0 $lru { } else {
#[allow(dead_code)]
fn set_lru_capacity(db: &dyn $Db, value: usize) {
$Configuration::fn_ingredient(db).set_capacity(value);
}
} }
}

@Veykril
Copy link
Contributor Author

Veykril commented Oct 25, 2024

All so to prevent leaking the $Configuration I believe

@nikomatsakis
Copy link
Member

Ah, I see. Yes, I do want to prevent leaking the Configuration name, and I remember there being something annoying so overcome scoping. One option might be to leverage a plumbing trait for the actual implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bikeshed 🚴‍♀️ Debating API details and the like
Projects
None yet
Development

No branches or pull requests

2 participants