Implement `IBufferWriter<char>` on `ICharTermAttribute`/`CharTermAttribute` and `OpenStringBuilder`

[NightOwl888]

Is there an existing issue for this?

• I have searched the existing issues

Task description

#1293 (comment)

The BCL IBufferWriter<T> interface is purpose-built for allowing arbitrary code to grow and write to a buffer when the source has either a known or unknown buffer length. This allows for scenarios with complex business logic that would be wasteful to repeat if the buffer passed in is not already large enough, without the baggage of System.Text.StringBuilder, which cannot even be converted to a ReadOnlySpan<char> without doing an extra allocation.

IAppendable is an interface from Java that performs a similar operation and IBufferWriter<T> has the potential to eliminate it from the Lucene.NET codebase. However, for now we should limit this addition to CharTermAttribute and the Normalizer2 in ICU4N which will give us a cross-library proof of concept.

The IBufferWriter<T> interface should be inherited directly by ICharTermAttribute, which is a breaking API change. However, due to the limited use cases of IAttribute-derived types, this is unlikely to affect many end users. The implementation of the 3 members will then need to be on the CharTermAttribute class.

The implementation should use the existing growth semantics provided by InternalResizeBuffer. See the Append(ReadOnlySpan<char>) implementation for an example.

See the ArrayBufferWriter and its tests to see an optimized implementation example along with some tests we can use to verify the implementation.

[paulirwin]

@NightOwl888 Are you looking to actually use these new methods internally in Lucene.NET as part of this, or just implement the API and make it available to use by end users of the library (and to get the breaking change out of the way)?

[NightOwl888]

The TL;DR is that IBufferWriter<T> is to .NET what the Appendable interface is to Java. For ICharTerrmAttribute this is mainly about compatibility.

---

I made a mistake when I opened this issue - Lucene.Net.ICU does not use Normalizer2 with ICharTermAttribute. Instead, it requests that Normalizer2 write to OpenStringBuilder. So, OpenStringBuilder should take priority over ICharTermAttribute, but since IBufferWriter<char> has only 3 members to implement, these can both be adapted in a single PR. The goal now is to ready these components for the next phase - which is to eliminate System.Text.StringBuilder, ICharSequence, and IAppendable as method parameters.

For the ICharTermAttribute case, there may be use cases within the analysis components where it makes sense to change this, but there is probably a stronger argument to keep the analysis component source code aligned with Lucene. The Append() method overloads will remain on ICharTermAttribute indefinitely, except for the ICharSequence overloads. So, for ICharTermAttribute, this is more to align it with the .NET ecosystem and support 3rd party components that write to this interface than to use it directly.

However, the plan for ICU4N is to eliminate the overloads that write to System.Text.StringBuilder and IAppendable in favor of using IBufferWriter<T> before it is released. System.Text.StringBuilder is not meant to be a cross-library data transport mechanism and does not fully embrace System.Memory types. IAppendable is just baggage left over from Java. Neither of these is the right mechanism to write to a buffer that manages its own growth semantics in .NET. However, IBufferWriter<T> is a near perfect .NET replacement for IAppendable and usually (but not always) a suitable replacement for System.Text.StringBuilder. For those cases where it is not (i.e. cases where the existing buffer must be read from or replaced), there is also the ArrayBufferWriter<T> class, but since it is sealed and only has .NET Core support, it has limited use as a public parameter type for a feature that writes to a buffer of unknown length.

It is more reasonable for 3rd parties to design components that use IBufferWriter<T> as a destination argument than it is with IAppendable, since the latter is defined in J2N. IBufferWriter<T> has existed since early releases of System.Memory, so there are likely many real-world components that already write to it and will certainly be more in the future.

[paulirwin]

@NightOwl888 If we're going through the breaking change of adding IBufferWriter<char> to ICharTermAttribute, it seems like we should go ahead and add ISpanAppendable as well. Thoughts?

Some historical discussion context from when we last tried this but decided against it: #1028 (comment)

I still could be swayed into accepting the breaking change for IBufferWriter<char> without ISpanAppendable, but last time the justification was that we could avoid a breaking change. Now that we're having to do one, it might be an opportune time, and then calling code can avoid the J2N extension method (with its type check) and call the Append(ReadOnlySpan<char>) overload directly.

[NightOwl888]

I think we should go the other direction. Rather than adding ISpanAppendable, we should remove IAppendable. And we should add all of the IAppendable members (except for the overloads that accept ICharSequence) directly to the ICharTermAttribute interface. But we have to wait until we are on the other side of the additions and adaptions before we can do the subtractions.

1. Add IBufferWriter<char>
2. Rework logic that uses ICharSequence from the rest of the project to use ReadOnlySpan<char> or possibly ReadOnlyMemory<char> in some cases.
3. Remove ICharSequence implementation (and add the sensible members to ICharTermAttribute - SubSequence() and HasValue should go away). We would do this from all of the buffer types at the same time.
4. Review and prep any callers that are still using IAppendable to use IBufferWriter<char> or another approach (including ICU4N).
5. Remove IAppendable and ISpanAppendable from ICharTermAttribute and any other buffer types.

If you want to go ahead and add the Append(ReadOnlySpan<char>) overload to ICharTermAttribute now, that is fine. I think I overestimated how valuable this interface is as an extension point. It is used like a key to lookup the concrete type but isn't very useful beyond that. So, it is not likely there are many user implementations of it that require a breaking change.

That said, IBufferWriter<T> is a different approach that requires the caller to write to the memory rather than the receiver. These are not append semantics, so there may be some corner cases where we need IAppendable for some reason. And IAppendable is not as problematic as ICharSequence to keep around if we need to. Hopefully not. Most callers will still be able to call the Append() methods on the concrete types without requiring the interface.

One unfortunate aspect of ICharTermAttribute is the fact that when using interfaces, the compiler won't respect any implicit operators of the underlying concrete type. So, after removing ICharSequence, callers will be required to call .AsSpan() on ICharTermAttribute to pass is into ReadOnlySpan<char> APIs. But on the other hand, once ICharSequence is removed, there is no chance it will accidentally be passed to an API that accepts ICharSequence when using ReadOnlySpan<char> is a better option.

I looked a bit into what it might take to switch to using the concrete CharTermAttribute type as the attribute key, but I am not sure that is the right move. In modern Lucene, CharTermAttribute has a subclass that implements several of the interfaces and the interface keys all map back to that single registration. Maybe there is a way, but it will require more analysis and consideration.