assert failed components/salsa-2022/src/function/specify.rs:130:48

[Chronostasys]

After I added a cycle recover logic to my language server, it triggers assertion failed.

error log:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `DatabaseKeyIndex { ingredient_index: IngredientIndex(32), key_index: Id { value: 17 } }`,
 right: `DatabaseKeyIndex { ingredient_index: IngredientIndex(49), key_index: Id { value: 17 } }`', ~/.cargo/git/checkouts/salsa-61760caba2b17ca5/d4a94fb/components/salsa-2022/src/function/specify.rs:130:48

[xffxff]

@Chronostasys Would it be possible for you to share a minimum, reproducible code with us? This would help us to better understand the issue

[Chronostasys]

@Chronostasys Would it be possible for you to share a minimum, reproducible code with us? This would help us to better understand the issue

Not yet. I've tried it yesterday, as the real-world case is really complecated, it may take more time for me to find out how to reproduce it on a sample project.

[Chronostasys]

After moving the cycle handler to another participant function in the cycle, the panic position changed:

thread 'main' panicked at 'expected a query assigned by `compile_dry_file(11)`, not `Program_emit(11)`'

and during recover, the participants be like:

["compile_dry_file(11)", "Program_emit(11)", "compile_dry_file(12)", "Program_emit(12)", "compile_dry_file(15)", "Program_emit(15)"]

Note that in my case, panic recovery logic is triggered three times in a single revision.

[Chronostasys]

@Chronostasys Would it be possible for you to share a minimum, reproducible code with us? This would help us to better understand the issue

@xffxff
https://github.com/Chronostasys/salsa-panic-example

[xffxff]

A simpler example to reproduce this issue can be found at https://gist.github.com/xffxff/47dbd41a078c8e705aa35d7a9d6f1303. Here's a brief version:

#[salsa::tracked(jar = Jar)]
fn memoized(db: &dyn Db, input: MyInput) -> u32 {
    memoized_a(db, MyTracked::new(db, input.field(db)))
}

#[salsa::tracked(jar = Jar)]
fn memoized_a(db: &dyn Db, tracked: MyTracked) -> u32 {
    MyTracked::new(db, 0);
    return memoized_b(db, tracked);
}

fn recovery_fn(db: &dyn Db, cycle: &salsa::Cycle, input: MyTracked) -> u32 {
    0
}

#[salsa::tracked(jar = Jar, recovery_fn=recovery_fn)]
fn memoized_b(db: &dyn Db, tracked: MyTracked) -> u32 {
    tracked.field(db) + memoized_a(db, tracked)
}

This code will fail with the following error:

thread 'cycle_memoized' panicked at 'assertion failed: `(left == right)`
  left: `DatabaseKeyIndex { ingredient_index: IngredientIndex(5), key_index: Id { value: 1 } }`,
 right: `DatabaseKeyIndex { ingredient_index: IngredientIndex(6), key_index: Id { value: 1 } }`

The issue is Salsa thinks the dependencies (inputs_outputs) of memoized_b(MyTracked { [salsa id]: 0 }) to be:

QueryEdges {
    input_outputs: [
        (
            Input,
            MyTracked(),
        ),
        (
            Output,
            MyTracked(1),
        ),
        (
            Output,
            field(MyTracked { [salsa id]: 1 }),
        ),
        (
            Input,
            field(MyTracked { [salsa id]: 0 }),
        ),
    ],
},

The panic occurs when the query memoized_b(MyTracked { [salsa id]: 0 }) tries to mark the output MyTracked(1) as valid. However, the output MyTracked(1) isn't created by memoized_b(MyTracked { [salsa id]: 0 }), but by memoized_a(MyTracked { [salsa id]: 0 }). The relevant code is:

salsa/components/salsa-2022/src/function/maybe_changed_after.rs

Line 213 in d4a94fb

db.mark_validated_output(database_key_index, dependency_index);

The dependencies of memoized_b(MyTracked { [salsa id]: 0 }) orginates from the following code:

salsa/components/salsa-2022/src/runtime.rs

Lines 393 to 407 in d4a94fb

	dg.for_each_cycle_participant(from_id, &mut from_stack, database_key_index, to_id, |aqs| {
	aqs.iter_mut()
	.skip_while(|aq| {
	match db.cycle_recovery_strategy(aq.database_key_index.ingredient_index) {
	CycleRecoveryStrategy::Panic => true,
	CycleRecoveryStrategy::Fallback => false,
	}
	})
	.for_each(|aq| {
	log::debug!("marking {:?} for fallback", aq.database_key_index.debug(db));
	aq.take_inputs_from(&cycle_query);
	assert!(aq.cycle.is_none());
	aq.cycle = Some(cycle.clone());
	});
	});

salsa/components/salsa-2022/src/runtime/active_query.rs

Lines 132 to 136 in d4a94fb

	pub(crate) fn take_inputs_from(&mut self, cycle_query: &ActiveQuery) {
	self.changed_at = cycle_query.changed_at;
	self.durability = cycle_query.durability;
	self.input_outputs = cycle_query.input_outputs.clone();
	}

When a cycle is detected, the query with recovery_fn, memoized_b(MyTracked { [salsa id]: 0 }) in this case, takes all the inputs and outputs from the cycle participants. However, as the function name take_inputs_from suggests, it might be more appropriate to only take the inputs from the cycle participants. (This might be a bug introduced by #413)

pub(crate) fn take_inputs_from(&mut self, cycle_query: &ActiveQuery) {
    self.changed_at = cycle_query.changed_at;
    self.durability = cycle_query.durability;
    self.input_outputs = cycle_query.input_outputs.iter().filter(|(edge_kind, database_key_index)| {
        *edge_kind == EdgeKind::Input
    })
    .copied().collect::<FxIndexSet<(EdgeKind, DependencyIndex)>>();
}

I'm uncertain if this is the correct solution. Furthermore, I'm unclear why the query withrecovery_fn in a cycle needs to take all the inputs from the cycle participants.