Fix debug impls section and other mistakes in the tutorial #1030
Conversation
✅ Deploy Preview for salsa-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
CodSpeed Performance ReportMerging #1030 will not alter performanceComparing Summary
|
| ### The `returns(ref)` annotation | ||
|
|
||
| You may have noticed that `parse_statements` is tagged with `#[salsa::tracked(returns(ref))]`. | ||
| Ordinarily, when you call a tracked function, the result you get back is cloned out of the database. | ||
| The `returns(ref)` attribute means that a reference into the database is returned instead. | ||
| So, when called, `parse_statements` will return an `&Vec<Statement>` rather than cloning the `Vec`. | ||
| This is useful as a performance optimization. | ||
| (You may recall the `returns(ref)` annotation from the [ir](./ir.md) section of the tutorial, | ||
| where it was placed on struct fields, with roughly the same meaning.) |
There was a problem hiding this comment.
It would be nice if we move this block to the ir section and explain the returns(ref) (and deref etc) there
There was a problem hiding this comment.
I think it's better to explain it in the parser.md file because ir only talks about structs (input, tracked, interned) whereas parser section talks about tracked functions, I think adding returns(ref) there makes more sense.
There was a problem hiding this comment.
It's just explaining it in different words, I got confused by "parse_statements is tagged with returns(ref)" but it could say "if we tag parse_statements with returns(ref), then this happens".
There was a problem hiding this comment.
The reason I suggested moving it to ir is because many structs in the ir module have fields attributed with returns(ref)
There was a problem hiding this comment.
Yeah that could make sense. I added an explanation of returns for both functions and structs in the parser section, it could be split into returns for structs in ir and then returns for functions in parser.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you. This is great. I've one small suggestion
|
@MichaReiser I added separate explanations for both I also added an example that made it easier for me to understand how the combinatorics of |
book/src/tutorial/ir.md
Outdated
| In order to better illustrate the workings of each `returns` annotation, we will | ||
| use this simple struct example: | ||
|
|
||
| ```rust | ||
| /// Number wraps an i32 and is Copy. | ||
| #[derive(PartialEq, Eq, Copy, Debug)] | ||
| struct Number(i32); | ||
|
|
||
| // Dummy clone implementation that logs the Clone::clone call. | ||
| impl Clone for Number { | ||
| fn clone(&self) -> Self { | ||
| println!("Cloning {self:?}..."); | ||
| Number(self.0) | ||
| } | ||
| } | ||
|
|
||
| // Deref into the wrapped i32 and log the call. | ||
| impl std::ops::Deref for Number { | ||
| type Target = i32; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| println!("Dereferencing {self:?}..."); | ||
| &self.0 | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The `Number` struct can be copied, cloned or derefed. Now let's define a salsa | ||
| struct with a `Number` field so we can explain the different `returns` | ||
| annotations: | ||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| We will also need a salsa db for this example: | ||
|
|
||
| ```rust | ||
| /// Salsa database to use in our example. | ||
| #[salsa::db] | ||
| #[derive(Clone, Default)] | ||
| struct NumDb { | ||
| storage: salsa::Storage<Self>, | ||
| } | ||
|
|
||
| #[salsa::db] | ||
| impl salsa::Database for NumDb {} | ||
| ``` | ||
|
|
||
| And finally a little program to test each annotation: | ||
|
|
||
| ```rust | ||
| let db: NumDb = Default::default(); | ||
| let input = Input::new(&db, Number(42)); | ||
|
|
||
| // Access the number field. | ||
| let number = input.number(&db); | ||
| eprintln!("number: {number:?}"); | ||
| ``` | ||
|
|
||
| ### `returns(clone)` (default behavior) | ||
|
|
||
| If our field contains no `returns` attribute at all or contains the | ||
| `returns(clone)` then accessing the field returns a clone. | ||
|
|
||
| Implicit clone: | ||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| Explicit clone: | ||
|
|
||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| #[returns(clone)] | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| Output of the program in both cases is: | ||
|
|
||
| ``` | ||
| Cloning Number(42)... | ||
| number: Number(42) | ||
| ``` | ||
|
|
||
| Type of the `number` variable is: | ||
|
|
||
| ```rust | ||
| let number: Number = input.number(&db); | ||
| ``` | ||
|
|
||
| ### `returns(ref)` | ||
|
|
||
| This attribute simply returns a reference instead of calling `Clone::clone`: | ||
|
|
||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| #[returns(ref)] | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| Output: | ||
|
|
||
| ``` | ||
| number: Number(42) | ||
| ``` | ||
|
|
||
| Type of `number`: | ||
|
|
||
| ```rust | ||
| let number: &Number = input.number(&db); | ||
| ``` | ||
|
|
||
| ### `returns(deref)` | ||
|
|
||
| Using this annotation invokes our implementation of `std::ops::Deref`, therefore | ||
| also changing the returned type in this example to `&i32` which is our `&Deref::Target`. | ||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| #[returns(deref)] | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| Output: | ||
|
|
||
| ``` | ||
| Dereferencing Number(42)... | ||
| number: 42 | ||
| ``` | ||
|
|
||
| Type of `number`: | ||
|
|
||
| ```rust | ||
| let number: &i32 = input.number(&db); | ||
| ``` | ||
|
|
||
| ### `returns(copy)` | ||
|
|
||
| If the field type is `Copy`, then `returns(copy)` can returned an owned copy of | ||
| the value without calling `Clone::clone`. | ||
|
|
||
| ```rust | ||
| #[salsa::input] | ||
| struct Input { | ||
| #[returns(copy)] | ||
| number: Number, | ||
| } | ||
| ``` | ||
|
|
||
| Output: | ||
|
|
||
| ``` | ||
| number: Number(42) | ||
| ``` | ||
|
|
||
| Type of `number`: | ||
|
|
||
| ```rust | ||
| let number: Number = input.number(&db); | ||
| ``` |
There was a problem hiding this comment.
I find this section too long. I think what I would do instead is use an existing definition from the ir file or we define a made up struct and use the simple bullet list above to explain the different return types:
clone: Reading the field returns a clone (requiresClone), e.g.Option<String>copy: Reading the field returns a copy (requiresCopy), e.g.Option<String>ref: Reading the field returns a reference, e.g.&Option<String>deref: Reading the field returns the dereferenced field (requiresSalsaAsDeref), e.g.Option<&str>asref: Reading the ... (requiresSalsaAsRef), e.g.Option<&String>
book/src/tutorial/parser.md
Outdated
| We will use a modified version of the example from the [IR](./ir.md#the-returns-attribute-for-struct-fields) | ||
| chapter to explain how the `returns` annotation works for function return types | ||
| instead of struct fields. |
There was a problem hiding this comment.
I would skip that entire section. The semantics are exactly the same and pointing to the ir documentation should be enough
There was a problem hiding this comment.
@MichaReiser I changed it. Still using the made up struct but only bullet list explanation for each returns.
2 participants
I was going through the tutorial and found some things that are outdated making some sections confusing. Particulary:
returns(ref)annotation", appears to be outdated because it starts with:wich is not true. The definition of
parse_statementsin the example code is:DebugWithDbwhich seems to have been removed in favor ofsalsa::Database::attach. I included an explanation of how that works instead.Rest of fixes are typos.