internal: Add a lot more database lifetimes

[Veykril]

No description provided.

[ChayimFriedman2]

To be honest I'm not a fan of all of our IDs gaining lifetimes. I'd much rather a permanent option for disabling the lifetime for both interneds and tracked structs, considering it isn't required for safety.

But, at the very least, I assume we want to wait for @dfireBird's PR to be merged.

[Veykril]

I'd much rather a permanent option for disabling the lifetime for both interneds and tracked structs, considering it isn't required for safety.

Huh? How is that not required for safety? The entire point of the lifetimes is to prevent usage of stale things

[Veykril]

Also the entire lifetime escape mechanism was only introduced to ease our transition, it was never meant to be a permanent thing.

[flodiebold]

FWIW, I agree with Chayim, the lifetimes clutter everything up for questionable gain.

[ChayimFriedman2]

Huh? How is that not required for safety? The entire point of the lifetimes is to prevent usage of stale things

I might be wrong, but things I've heard and my knowledge of the Salsa codebase imply this is false.

Furthermore you fundamentally cannot rely on the lifetimes for safety (without branding) since you can use the same ID to access a different database.

It can help correctness a bit but I don't think this is worth it.

Also the entire lifetime escape mechanism was only introduced to ease our transition, it was never meant to be a permanent thing.

I know that, I suggest we keep it (and extend it to tracked structs).

[Veykril]

Fair, I do agree its a bit ugly to have the lifetime infect everything.

I might be wrong, but things I've heard and my knowledge of the Salsa codebase imply this is false.

Furthermore you fundamentally cannot rely on the lifetimes for safety (without branding) since you can use the same ID to access a different database.

I do think salsa relies on the lifetime for safety though fwiw (with the intern gc and what not, at this point), though you are right that the mixing of databases might be problematic 🤔 Will look into that actually.

Though yea, maybe adding the attribute for tracked structs would be good either way (the attr should also be marked unsafe for that matter though)

[ChayimFriedman2]

So, you were right. I was able to construct a UB example when using the interneds GC.

Given that, I do support this. IMO preventing unsafety and the need for global reasoning is worth the lifetimes.

[Veykril]

Is it a small self contained example, curious to see in that case.

But, at the very least, I assume we want to wait for dfireBird's PR to be merged.

Also naturally will wait on the lifetime PR to land first, rebasing this PR is a lot easier than other PRs on top of this

[ChayimFriedman2]

Is it a small self contained example, curious to see in that case.

use salsa::Setter;

#[salsa::interned(revisions = 1, no_lifetime)]
struct Interned {
    #[returns(ref)]
    s: String,
}

#[salsa::input(debug)]
struct Input {
    i: String,
}

#[salsa::tracked]
fn foo(db: &dyn salsa::Database, i: Input) -> Interned {
    Interned::new(db, i.i(db))
}

#[test]
fn test() {
    let mut db = salsa::DatabaseImpl::default();
    let input = Input::builder("0".to_owned()).new(&db);
    let foo_input = Input::new(&db, "abc def dsgdf".to_owned());
    let foo_input2 = Input::new(&db, String::new());
    let interned = foo(&db, foo_input);

    input.set_i(&mut db).to("123".to_owned());

    let s = &**interned.s(&db);
    foo(&db, foo_input2);
    dbg!(s);
}

[ada4a]

Oops, I've been working on implementing what's apparently a subset of this PR^^ Glad to see that my changes seem to match up with what was done here

View changes since this review

[ada4a]

I think this is identical to:

Suggested change

	pub fn method_resolution<'a>(&self, expr: ExprId) -> Option<(FunctionId, GenericArgs<'a>)> {
	pub fn method_resolution(&self, expr: ExprId) -> Option<(FunctionId, GenericArgs<'static>)> {

which is imo clearer than introducing another lifetime generic?

[Veykril]

Oh dear, the fact this compiles is not good. This is a pretty severe soundness bug as that lifetime is just straight up wrong

[ada4a]

Oh... In that case you would probably be unhappy to learn that the same pattern occurs throughout that impl block

[Veykril]

yea i put a fixme on the actual method that does the bad transmute

[ChayimFriedman2]

We already talked about that, this is pretty much unavoidable and it only makes GC potentially unsound, which is why we marked it unsafe.

[Veykril]

Right, that was that thing.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.