Skip evaluation of a Modifying block iff it doesn't do anything

[DivineDominion]

The Modifying(<Range>) { <Block> } construct always evaluates even if the block is empty.

With TextKit integration, this means

1. an undo group is being started (and ended)
2. NSTextView.shouldChangeText(in:replacementString:) and didChangeText() are being run to guard against unwanted changes

With syntax highlighting in the text storage, you may end up processing the text for what's essentially a no-op.

How to test

To get an empty block, use a for-loop to trigger the buildArray path of the result builder, but without any actual iterations:

Modifying(selectedRange) { _ in
 for _ in 0 ..< 0 {
 Insert(0) { "loop never runs" }
 }
}

I'm not sure whether we can figure out at all whether a result builder produces nothing (i.e. empty array).

Complete test case

This test fails with a thrown error at try buffer.evaluate because the text view doesn't permit changes in the range.

This should not be a problem, because the range is not actually changed.

 func testModifying_EmptyLoopBlock_SkipsEvaluation() throws {
 class TextViewSpy: NSTextView {
 var didCallShouldChangeText = false
 var didCallDidChangeText = false

 override func shouldChangeText(in affectedCharRange: NSRange, replacementString: String?) -> Bool {
 didCallShouldChangeText = true
 return false // Would abort modification an error
 }

 override func didChangeText() {
 didCallDidChangeText = true
 }
 }

 let textViewSpy = TextViewSpy()
 textViewSpy.string = "Lorem ipsum."
 let buffer = NSTextViewBuffer(textView: textViewSpy)
 let selectedRange: SelectedRange = .init(location: 6, length: 5)

 assertBufferState(buffer, "Lorem ipsum.ˇ")

 try buffer.evaluate {
 Modifying(selectedRange) { _ in
 for _ in 0 ..< 0 {
 Insert(0) { "loop never runs" }
 }
 }
 }

 XCTAssertFalse(textViewSpy.didCallShouldChangeText)
 XCTAssertFalse(textViewSpy.didCallDidChangeText)
 }

[DivineDominion]

Current State

So the Modifying construct is of this simplified form:

struct Modifying<Content: Modification> {
 typealias ModificationBody = (SelectedRange) -> Content

 let range: SelectedRange
 let modification: ModificationBody

 init(
 _ range: SelectedRange,
 @ModificationBuilder body: @escaping ModificationBody
 ) { ... }
}

protocol Modification: Expression
where Evaluation == ChangeInLength, Failure == BufferAccessFailure {
// boils down to exposing:
// func evaluate(in buffer: Buffer) -> Result<ChangeInLength, BufferAccessFailure>
}

The result builder doesn't permit mixing, so we have builder code paths for deletion and insertion. Take insertion for example. The relevant part of the builder is this (ignoring all the rest):

@resultBuilder
struct ModificationBuilder { 
 static func buildArray(_ components: [Insert]) -> Insert {
 return Insert(SortedArray(
 unsorted: components.map(\.insertions).joined(),
 areInIncreasingOrder: TextInsertion.arePositionedInIncreasingOrder
 ))
 }
}

No matter if 0, 1, 2, 100 insertions -- they are all concatenated into 1 Insert value.

[DivineDominion]

Approach 1: Awareness of Emptiness

Modification types could expose whether they are empty or not:

protocol Modification: Expression
where Evaluation == ChangeInLength, Failure == BufferAccessFailure {
+ var isEmpty: Bool { get }
}

That makes sense from a collection perspective, which Insert and Delete could take.

But "Is this modification emtpy?" is an odd question.

[DivineDominion]

Approach 2: Either something or nothing

Make the array case a special case and not return Insert directly.

Return either an insert or nothing. For "nothing", we have Identity, which is a no-op.

@resultBuilder
struct ModificationBuilder { 
- static func buildArray(_ components: [Insert]) -> Insert {
- return Insert(SortedArray(
+ static func buildArray(_ components: [Insert]) -> Either<Insert, Identity> {
+ let insertions = SortedArray(
 unsorted: components.map(\.insertions).joined(),
 areInIncreasingOrder: TextInsertion.arePositionedInIncreasingOrder
- ))
+ )
+ guard !insertions.isEmpty else { return .right(Identity()) }
+ return .left(Insert(insertions)
 }
}

This could express a no-op with the .right(Identity()) path.

It's less awkward to ask "do we have a modification or do we do nothing?" than "is this modification empty?"

Consequently, this should be added as a test and also not trigger the text view checks:

Modifying(selectedRange) { Identity() }

The Modifying construct would need a code branch, then, to abort:

if modification is Identity { return }

But actually, we would have Either.right(Identity()), not Identity() directly, so the check would fail, or we need more checks for more special cases. Could also be Either.left. So that's 3 cases -- and I wouldn't know how to nicely express them, given that for switch-case pattern matching with an Either<Left, Right> enum you would need to specify Left/Right. Thankfully, the cases are somewhat limited:

// return a ChangeInLength.empty from the Modifying.evaluate function as a successful no-op
switch theModification {
case _ as Identity: return .success(.empty)
case .right(_) as Either<Insert, Identity>: return .success(.empty)
case .left(_) as Either<Identity, Insert>: return .success(.empty)
case .right(_) as Either<Delete, Identity>: return .success(.empty)
case .left(_) as Either<Identity, Delete>: return .success(.empty)
default: break
}

Variation with protocols

We can use an existential type, any Modification, like this on Either:

extension Either where Left: Modification, Right: Modification {
 var modification: any Modification {
 return switch self {
 case .left(let l): l
 case .right(let r): r
 }
 }
}

Then we could ask someEither.modification is Identity instead.

But how do we get from an opaque type (the Content in Modifying<Content: Modification>) to an Either? With casting or pattern matching like above:

case let either as Either<Insert, Identity> where either.modification is Identity: return .success(.empty)
case let either as Either<Identity, Insert> where either.modification is Identity: return .success(.empty)
case let either as Either<Delete, Identity> where either.modification is Identity: return .success(.empty)
case let either as Either<Identity, Delete> where either.modification is Identity: return .success(.empty)

Meh. That only moved the "do I have to write .left or .right" decision into the computed property.

We would have to use a protocol abstraction:

protocol ModificationContainer {
 var modification: any Modification { get }
}

Then using this base type would do the trick to simplify cases:

switch theModification {
case _ as Identity: return .success(.empty)
case let container as ModificationContainer where container.modification is Identity: return .success(.empty)
default: break
}

Down to two branches here, and two branches in the Either extension. That would at least scale, but is introducing the protocol worth it?

[DivineDominion]

WIP Approach 3: Protocols, but earlier

If we bite the bullet and use a protocol to abstract things away, we might as well use protocols directly:

protocol Noop { }
extension Identity: Noop { }

Then we could check whether theModification is Noop, but I'm not sure if starting with that leads to any different place, actually:

We would still need an Either as a union type for, well, either insertion or identity.

We can't have Insert conform to Noop only iff its array of elements is empty.

So I guess it'll end up with Either<L, R> anyway.

[DivineDominion]

WIP Approach 4: Side effects are the problem

The original question assumes that the approach is sound, but that we need to handle this one edge case of Identity or an empty array in the result builder.

Maybe the problem is not "how do I inspect the result builder's result for non-emptiness", but rather "can I not perform side effects?"

Because the existence of side effects in the current implementation is the driving force to skip the procedure completely if the modification isn't actually modifying anything.