Add 'db lifetime to BuiltinDeriveMacroId - #23060
Conversation
rustbot
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
3299abc to
43c5b6d
Compare
This comment has been minimized.
This comment has been minimized.
d3f8893 to
d5387cb
Compare
There was a problem hiding this comment.
I'm a bit hesitant to merge this while @dfireBird has its lifetimes PR in flight; this PR is much easier to review but the work there is definitely higher...
| ) { | ||
| // Lazily initialised when we first encounter a `#[doc = macro!()]`. | ||
| let mut expander: Option<DocMacroExpander<'db>> = None; | ||
| let mut expander = LazyCell::new(|| { |
There was a problem hiding this comment.
Why this change? (LazyCell has an additional branch).
There was a problem hiding this comment.
Mostly code clarity I'd say? LazyCell makes it very clear what's happening: a value that is initialized lazily.
LazyCellhas an additional branch
You mean to check whether the value has been initialized or not? But I think Option::get_or_insert_with does a similar check for None, no?
Anyway I don't feel too strongly about this, happy to revert.
There was a problem hiding this comment.
LazyCell needs to check for reentrance.
I wouldn't oppose if this was written this way to begin with, but I do oppose changing it in an unrelated PR.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Why this test? Is it for the DynMap? Most of it is type gymnastics, I don't think it needs tests.
There was a problem hiding this comment.
It was a leftover from Lukas' code snippet – I'll remove it if you say it's unnecessary
| } | ||
|
|
||
| #[salsa::interned(debug, unsafe(no_lifetime), revisions = usize::MAX)] | ||
| #[salsa::interned(debug)] |
There was a problem hiding this comment.
I wouldn't remove revisions = usize::MAX yet. We need to take a conscious decision to enable interneds GC and in what frequency.
There was a problem hiding this comment.
Note that it was only recently added in during salsa upgrade (4d59eee#diff-8a8808fa1cbb71d05f842b69e4bda92728af59464406b4aa58df635cea4c2571L357-R357), because salsa started requiring it when no_lifetime is present. Still, I'll re-add it for now.
| lang_items: &'db LangItems, | ||
| resolver: &'a Resolver<'db>, | ||
| store: &'db ExpressionStore, | ||
| store: &'a ExpressionStore, |
There was a problem hiding this comment.
Why this change? It should remain 'db.
There was a problem hiding this comment.
Basically, because of this:
error[E0597]: `store` does not live long enough
--> crates/hir/src/source_analyzer.rs:402:17
|
109 | impl<'db> SourceAnalyzer<'db> {
| --- lifetime `'db` defined here
...
383 | let (store, params, _) = lower_generic_params(
| ----- binding `store` declared here
...
399 | match where_predicate_must_hold(
| ___________________-
400 | | db,
401 | | &self.resolver,
402 | | &store,
| | ^^^^^^ borrowed value does not live long enough
... |
406 | | predicate,
407 | | ) {
| |_____________- argument requires that `store` is borrowed for `'db`
...
421 | }
| - `store` dropped here while still borrowed
here, lower_generic_params creates store: ExpressionStore, and when we try to pass a reference to that to where_predicate_must_hold, the lifetime of that reference doesn't live enough (for 'db), because store is only live inside the function body.
I think it's fair to say that in general, if a struct/enum has a field of type &T (like TyLoweringContext.store: &ExpressionStore), then the lifetime of that reference shouldn't be 'db, because that makes things invariant over 'db.
|
Happy to wait for @dfireBird's PR to land first – I honestly doubted I would be able to drive this to completion, but since I now have, fixing it up as needed (by reading) sounds doable |
This comment has been minimized.
This comment has been minimized.
d5387cb to
c291d7b
Compare
- use `from_id!` for `EnumVariant{,Id}`
- use `?` for consistency with the other methods
- rm needlessly complicated `Ordering` clause
- use chained if-lets to reduce nesting
- rm manual lifetime expansion (`parent_arc` seems to have been an Arc
in the past)
- use `.into()` to turn things into `Definition`s
A lot of these were necessary to allow the next commit to compile, but a few are miscellaneous.
c291d7b to
4539b3a
Compare
|
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. |
3 participants
Part of #22868