Skip to content

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Feb 8, 2025

Use the Fallback trick similar to what the derive(Update) does to constrain the tracked-fn's return type to Update.

This removes the need to implement Update for types with a 'static lifetime.

I also added a few Update implementations that are needed for Ruff. The not good news is, Ruff now panics with some assertions and I haven't figured out the root cause yet.

Edit: Okay, the panics go away if I back out of the coarse-grained dependency changes... So something goes wrong there, uff.

@netlify
Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit c48a9df
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67a8c4ce5808a000083c92b1

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 8, 2025

CodSpeed Performance Report

Merging #680 will not alter performance

Comparing MichaReiser:micha/tracked-fn-update-fallback (c48a9df) with master (538eaad)

Summary

✅ 9 untouched benchmarks

| ^^^^^^^^^^^^^^^^^
| |
| lifetime `'db` defined here
| requires that `'db` must outlive `'static`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errors are, unfortunately, not very helpful

unsafe fn maybe_update<'db>(old_pointer: *mut Self::Output<'db>, new_value: Self::Output<'db>) -> bool {
unsafe {
use $zalsa::UpdateFallback;
salsa::plumbing::UpdateDispatch::<Self::Output<'db>>::maybe_update(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at hte actual diagnostic being emitted (it differs slightly due to having more spans to introspect when testing outside of an expansion I think)

error[E0521]: borrowed data escapes outside of function
  --> tests\tracked_fn_no_eq.rs:55:9
   |
52 | fn assert_update_on_return_type<'db>(old_pointer: *mut &'db u32, new_value: &'db u32) -> ! {
   |                                 ---  ----------- `old_pointer` is a reference that is only valid in the function body
   |                                 |
   |                                 lifetime `'db` defined here
...
55 |         salsa::plumbing::UpdateDispatch::<&u32>::maybe_update(old_pointer, new_value);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |         |
   |         `old_pointer` escapes the function body here
   |         argument requires that `'db` must outlive `'static`
   |
   = note: requirement occurs because of a mutable pointer to `&u32`
   = note: mutable pointers are invariant over their type parameter
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

The triggering span seems to be the maybe_update identifier (and/or the full path span), so to improve the diagnostics here we'd need to pass that path / identifier from the proc-macro to the decl macro with an adjusted span

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things that are "bad" about the diagnostic:

  • The error isn't very actionable: I have to know that the solution here is to implement the Update trait
  • The span point into some non existing code. Ideally, we'd use the span of the return type (possibly?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I improved the span. I'm not very familiar with macros so maybe there's a better way of doing this rather than generating the entire function in the proc macro?

Anyway, the error message now looks like this which seems decent

error[E0599]: the function or associated item `maybe_update` exists for struct `UpdateDispatch<ParsedModule>`, but its trait bounds were not satisfied
   --> crates/ruff_db/src/parsed.rs:24:50
    |
24  | pub fn parsed_module(db: &dyn Db, file: File) -> ParsedModule {
    |                                                  ^^^^^^^^^^^^ function or associated item cannot be called on `UpdateDispatch<ParsedModule>` due to unsatisfied trait bounds
...
45  | pub struct ParsedModule {
    | ----------------------- doesn't satisfy `ParsedModule: PartialEq` or `ParsedModule: Update`
    |
   ::: /Users/micha/astral/salsa/src/update.rs:32:5
    |
32  |     pub struct Dispatch<D>(PhantomData<D>);
    |     ---------------------- doesn't satisfy `_: UpdateFallback<ParsedModule>`
    |
note: if you're trying to build a new `UpdateDispatch<ParsedModule>`, consider using `UpdateDispatch::<D>::new` which returns `UpdateDispatch<_>`
   --> /Users/micha/astral/salsa/src/update.rs:41:9
    |
41  |         pub fn new() -> Self {
    |         ^^^^^^^^^^^^^^^^^^^^
    = note: the following trait bounds were not satisfied:
            `ParsedModule: Update`
            `ParsedModule: PartialEq`
            which is required by `UpdateDispatch<ParsedModule>: UpdateFallback<ParsedModule>`
note: the trait `Update` must be implemented
   --> /Users/micha/astral/salsa/src/update.rs:134:1
    |
134 | pub unsafe trait Update {
    | ^^^^^^^^^^^^^^^^^^^^^^^
help: consider annotating `ParsedModule` with `#[derive(PartialEq)]`
    |
45  + #[derive(PartialEq)]
46  | pub struct ParsedModule {
    |

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It at least points users in the right direction -> add Partialeq or implement Update

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-diagnosticon_unimplemented-attribute is useful here? (unsure given we go through the fallback trick)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that might work. Let me create an issue for this

…ent `Update` for `smallvec` and `compact_str`
@MichaReiser MichaReiser force-pushed the micha/tracked-fn-update-fallback branch from fad1f46 to acbee94 Compare February 9, 2025 14:48
@MichaReiser MichaReiser added this pull request to the merge queue Feb 10, 2025
Merged via the queue into salsa-rs:master with commit 351d9cf Feb 10, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants