Allow implementations of SpanContents to own their backing data #411
+59
−29
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the lifetime parameter on the
SpanContents
trait. (addresses #410). This is a breaking change.Up to authors/maintainers whether to merge this or not, here are the overall pros and cons of this approach:
Pros:
SpanContents
much more flexible, as it is no longer forced to borrow its data from the correspondingSourceCode
.SpanContents
, as opposed to taking its data reference, calling the getters once and constructing a newMietteSpanContents<'data>
. This probably doesn't matter that much, but it is technically possible to create a weirdSpanContents
implementation that does not return consistent values from its getters, which the current implementation ofNamedSource
does not handle correctly.Cons:
NamedSource
and some user-definedSpanContents
wrappers slightly less efficient. As discussed above,NamedSource
and other user-defined wrappers are now forced to retain the innerSpanContents
instead of copying the relevant data out of it, requiring the addition of an indirection if the innerSpanContents
' type is not statically known. In practice many such usages will retain similar performance (at least in release builds) due to vtable inlining, but the fact remains that this change will result in equivalent or worse performance.SpanContents
in a way similar to the current implementation ofNamedSource
(see the changes toNamedSource
for an example of what this entails).Semantic differences
Here are the implementations with all of their lifetime parameters unelided, current:
and proposed:
The key difference in semantics between the two is that the former has an independent borrow lifetime which may outlive
Self
, i.e. the current definition can be used as below, while my proposed implementation cannot.However, this flexibility w.r.t. the lifetime of the borrow comes at a cost, forcing every implementation of
SpanContents
to borrow its data from somewhere else that lives for'a
because'a
may outliveSelf
.This is the current signature of the source of all
SpanContents
,SourceCode::read_span
:Because of the
+ 'a
bound on the trait object, the lifetime ofSpanContents
' borrow must be equal to the lifetime ofSelf
. With this added constraint, the two approaches above become almost equivalent, most importantly they are both valid to calldata
on for the exact same lifetime ('a
in the context ofread_span
). The only difference is that the borrow returned bydata
is now allowed to originate within theSpanContents
(causing the caveats I mentioned above).Errata
NamedSource
intended to override the language all the time, or only whenlanguage
is notNone
? (the previous behavior is all of the time but this feels like the wrong behavior)I ran
cargo test
locally with these changes and all tests pass.