Panic when reading fields of tracked structs from older revisions

[xffxff]

use test_log::test;

#[salsa::jar(db = Db)]
struct Jar(MyInput, MyTracked, final_result, intermediate_result);

trait Db: salsa::DbWithJar<Jar> {}

#[salsa::input(jar = Jar)]
struct MyInput {
    field: u32,
}

#[salsa::tracked(jar = Jar)]
struct MyTracked {
    field: u32,
}

#[salsa::tracked(jar = Jar)]
fn final_result(db: &dyn Db, tracked: MyTracked) -> u32 {
    tracked.field(db) * 2
}

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

#[salsa::db(Jar)]
#[derive(Default)]
struct Database {
    storage: salsa::Storage<Self>,
}

impl salsa::Database for Database {}

impl Db for Database {}


#[test]
fn execute() {
    let mut db = Database::default();

    let input = MyInput::new(&mut db, 22);
    let tracked = intermediate_result(&db, input);

    dbg!(final_result(&db, tracked));

    input.set_field(&mut db).to(24);
    dbg!(final_result(&db, tracked));
}

it will output

[salsa-2022-tests/tests/hello_world.rs:46] final_result(&db, tracked) = 22
[salsa-2022-tests/tests/hello_world.rs:49] final_result(&db, tracked) = 22

not sure if this is expected behavior, or if we should get

[salsa-2022-tests/tests/hello_world.rs:46] final_result(&db, tracked) = 22
[salsa-2022-tests/tests/hello_world.rs:49] final_result(&db, tracked) = 24

[nikomatsakis]

@xffxff Actually, I believe the expected behavior should be a panic -- I've been meaning to open an issue about this!

[nikomatsakis]

So, the problem in this example is that it is bogus to take a tracked struct from an older revision and continue using it after altering the inputs. However, we don't detect this! And even simpler test would just be to read:

dbg!(tracked.field(db));

That will currently print 22 but should (imo) panic.

The offending code is this case:

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

Lines 173 to 178 in f0fe691

	QueryOrigin::BaseInput | QueryOrigin::Field => {
	// BaseInput: This value was `set` by the mutator thread -- ie, it's a base input and it cannot be out of date.
	// Field: This value is the value of a field of some tracked struct S. It is always updated whenever S is created.
	// So if a query has access to S, then they will have an up-to-date value.
	return true;
	}

QueryOrigin::Field should act the same as QueryOrigin::Assigned, I believe:

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

Lines 165 to 172 in f0fe691

	QueryOrigin::Assigned(_) => {
	// If the value was assigneed by another query,
	// and that query were up-to-date,
	// then we would have updated the `verified_at` field already.
	// So the fact that we are here means that it was not specified
	// during this revision or is otherwise stale.
	return false;
	}

This would indicate that the value is considered dirty. Attempting to execute the function will panic.

Alternatively, I think we could just panic immediately in maybe_changed_after -- that might even be better. The panic message would be something like "accessing field of tracked struct {:?} from older revision".

[nikomatsakis]

@xffxff would you be up to take a crack at fixing this?

[xffxff]

would you be up to take a crack at fixing this?

Yes, I'd love to