Allow implementations of SpanContents to own their backing data #411
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
SpanContentstrait. (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:
SpanContentsmuch 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 weirdSpanContentsimplementation that does not return consistent values from its getters, which the current implementation ofNamedSourcedoes not handle correctly.Cons:
NamedSourceand some user-definedSpanContentswrappers slightly less efficient. As discussed above,NamedSourceand other user-defined wrappers are now forced to retain the innerSpanContentsinstead 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.SpanContentsin a way similar to the current implementation ofNamedSource(see the changes toNamedSourcefor 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
SpanContentsto borrow its data from somewhere else that lives for'abecause'amay outliveSelf.This is the current signature of the source of all
SpanContents,SourceCode::read_span:Because of the
+ 'abound 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 calldataon for the exact same lifetime ('ain the context ofread_span). The only difference is that the borrow returned bydatais now allowed to originate within theSpanContents(causing the caveats I mentioned above).Errata
NamedSourceintended to override the language all the time, or only whenlanguageis notNone? (the previous behavior is all of the time but this feels like the wrong behavior)I ran
cargo testlocally with these changes and all tests pass.