replaceError(with ...) as @autoclosure?

[randomeizer]

Just wondering if it would make sense to have this:

https://github.com/pointfreeco/swift-parsing/blob/85e1c50d03999a2b53d30ddae4d0f2a3d872b335/Sources/Parsing/Parsers/ReplaceError.swift#L36-38

as:

@inlinable
  public func replaceError(with output: @autoclosure () -> Output) -> Parsers.ReplaceError<Self> {
    .init(output: output, upstream: self)
  }

(with requisite adjustment in Parsers.ReplaceError's init). Otherwise a potentially-expensive default output is always getting generated, even if it's never used.

[tgrapperon]

Hey @randomeizer, as you were posting this, I was submitting the PR #132. I've tried the @autoclosure approach, but I removed it to not collide (in terms of code completion) with the (Error) -> Output variant I'm proposing. This closure can be used to fulfil the same objective as @autoclosure, but an @autoclosure variant is also possible if my autocompletion argument is moot.

[stephencelis]

Interesting idea! I think we'd probably want to benchmark such a thing to see how it impacts parsing. I imagine most constant Output values are relatively simple (on the level of Ints and single-field structs/enums), so I wonder if the closure ends up being more expensive depending on the situation.

Notably, the equivalent operation in combine does not employ an auto-closure:

https://developer.apple.com/documentation/combine/publisher/replaceerror(with:)

[tgrapperon]

I've closed #132, but I'll benchmark different scenarios later out of curiosity.

[randomeizer]

If the parser is being saved, the constant Output would just get created once, whereas the closure would be called every time the parser is called.

That said, you could have a closure and a constant override, where the constant version just wraps it on construction:

extension Parser {
  public func replaceError(with output: Output) -> Parsers.ReplaceError<Self> {
    .init(output: { output }, upstream: self)  
  }
}

I think that would conflict with the @autoclosure version, but you could still have a regular closure-based version.

Anyway, happy to park it until ParsingError matures.

[tgrapperon]

@stephencelis Here are the benchmark results for various scenarios.

Parse {
  "1".map {[]}.replaceError(with: Array<Int>(repeating: 0, count: 1_000_000))
}

I've tested the time it takes to define this parser, and the time it takes to run a predefined value and fail when parsing "2".

• Value is the current implementation;
• Value - Autoclosure, the implementation discussed here, evaluated in init;
• Function - Autoclosure, the variant where ReplaceOutput works with a function all the way, like in Use a closure () -> Output in ReplaceError #132.

name                                                time          std        iterations
---------------------------------------------------------------------------------------
ReplaceError.Create Parser - Value                  256000.000 ns ±  30.91 %       4128
ReplaceError.Create Parser - Value - Autoclosure    240848.000 ns ±  26.69 %       5434
ReplaceError.Create Parser - Function - Autoclosure     43.000 ns ± 322.91 %    1000000
ReplaceError.Run - Value                               421.000 ns ± 102.84 %    1000000
ReplaceError.Run - Value - Autoclosure                 420.000 ns ± 115.13 %    1000000
ReplaceError.Run - Function - Autoclosure           242065.000 ns ±  22.96 %       5179

I've also tested a very small Output

Parse {
  "1".map { _ in 2}.replaceError(with: 3)
}

name                                                time       std         iterations
-------------------------------------------------------------------------------------
ReplaceError.Create Parser - Value                   50.000 ns ± 463.41 %     1000000
ReplaceError.Create Parser - Value - Autoclosure     50.000 ns ± 1071.19 %    1000000
ReplaceError.Create Parser - Function - Autoclosure  46.000 ns ± 474.13 %     1000000
ReplaceError.Run - Value                            422.000 ns ± 182.36 %     1000000
ReplaceError.Run - Value - Autoclosure              420.000 ns ± 178.13 %     1000000
ReplaceError.Run - Function - Autoclosure           414.000 ns ± 139.26 %     1000000

So overall, using a function that is evaluated lazily in case of failure seems the fastest. There is no significative overhead in defining this function, even when the value is as simple as an Int. It can be problematic if the parser fails often once defined (which is more likely than the opposite), but I guess it's possible to cache the value on the first failure to achieve a performance that is similar to the Value version starting from the second failure.

In the other case, there is no real benefit in autoclosing, as we always evaluate the function immediately anyway.

Do you want me to reopen #132 , remove the (Error) argument and cache the returned value? [Edit: Done]