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

Introduce Input::builder API #543

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jul 29, 2024

This PR introduces a new Input::builder API.

#[salsa::input]
struct MyInput {
    required_field: bool,

    #[default]
    optional_field: usize,
}

let input = MyInput::builder(true)
    .optional_field(20)
    .required_field_durability(Durability::HIGH)
    .new(&db);

It also adds support for the new input-field-attribute #[default]. It specifies that the value should be initialized with $ty::default() unless it is specified when using the builder API. Optional fields don't need to be specified when calling Input::new. I intentionally kept the default attribute simple for now. We can add support for calling a function, or initializing with a const value in the future.

One downside of the macro-generated builder is that at least RustRover (IntelliJ) completely breaks down and fails to provide any auto-completion. I yet have to try if rust analyzer does any better.

Edit: Ra does better! It does suggest the different builder methods, except new. new doesn't show up for some reason.

Fixes #385
Fixes #476 (not in its entirety but a start)

Design

This API inverts the original design where db was passed to builder and the required fields to new. There are a few reasons for this:

  • Passing db in new requires that the builder stores a reference to IngredientImpl<C>.
    • It requires passing around some more lifetimes that aren't necessary with the proposed design
    • Configuration isn't referenceable in the builder module. That means, the generated Builder would need to be generic over Configuration (or use some other trickery to avoid naming Configuration
    • It should allow for builder(required_fields).optional(some_query(db)).new(&mut db) without having to store the optional value in a temporary variable (once we support optional fields).
  • Enforcing that all mandatory fields are passed when constructing the builder should simplify implementing support for optional fields where the macro code can generate $fields` using either the default or provided value.

Alternative Designs

I'm not too fond of the *_durability methods. An alternative could be introducing a #[durability=Durability::HIGH] field and the builders and setters would respect that durability. The downside of this is that it is less flexible and it is unclear whether builder.durability should override the durability for those fields (I see reasons for and against it).

Why not use type-states?

I thought about it but it felt overly-complicated to me and goes beyond Salsa's scope. If someone wants more advanced builders than using any other builder crate on top of salsa should be possible (create a single function that takes all input values and generate a builder for that function).

Tests

I did add a test but I haven't figured out a way to automatically verify if a tracked function was validated using deep or shallow comparison.
I did do some manual testing by adding dbg statements that show that Salsa does use the shallow comparison for inputs with Durability::HIGH.

Copy link

netlify bot commented Jul 29, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 547663f
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/66b1c91119a8080008a5e0c8

Copy link

codspeed-hq bot commented Jul 29, 2024

CodSpeed Performance Report

Merging #543 will not alter performance

Comparing MichaReiser:input-builder (fbc7b8c) with master (d6df21f)

Summary

✅ 1 untouched benchmarks

@MichaReiser MichaReiser force-pushed the input-builder branch 5 times, most recently from 12572b2 to 8e646ec Compare July 29, 2024 13:13
@MichaReiser MichaReiser marked this pull request as ready for review July 29, 2024 13:13
Self::builder($($required_field_id,)*).new(db)
}

pub fn builder($($required_field_id: $required_field_ty),*) -> <Self as $zalsa_struct::HasBuilder>::Builder
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 would be nice if we could avoid the extra required_fields macro argument but I couldn't figure out how to use macro_if in a function declaration.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, you'd have to wrap the whole function. seems fine this way.

@MichaReiser
Copy link
Contributor Author

I plan to rebase this PR tomorrow

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Looks great! I'd be happy to merge.

Self::builder($($required_field_id,)*).new(db)
}

pub fn builder($($required_field_id: $required_field_ty),*) -> <Self as $zalsa_struct::HasBuilder>::Builder
Copy link
Member

Choose a reason for hiding this comment

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

yeah, you'd have to wrap the whole function. seems fine this way.

components/salsa-macro-rules/src/setup_input_struct.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser added this pull request to the merge queue Aug 6, 2024
Merged via the queue into salsa-rs:master with commit 4c8e52a Aug 6, 2024
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.

Generate builders methods Add an API like new_with_durability to construct input with specific durability
2 participants