Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes the restriction on the amount of non-void Parsers that can be used in a Parserbuilder #367

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

coenttb
Copy link
Contributor

@coenttb coenttb commented Jan 10, 2025

This pull request eliminates the need for all Take*n overloads except for Take2 while maintaining full backward compatibility.

The Take2.Map parameter pack construct is a deliberate tradeoff. Ideally, we would prefer to use a Conversion with a MapConversion for a more elegant solution. However, consider the following hypothetical TupleConversion:

public struct TupleConversion<each T1, T2>: Conversion {
    public typealias Input = ((repeat each T1), T2)
    public typealias Output = (repeat each T1, T2)

    public func apply(input: ((repeat each T1), T2)) -> (repeat each T1, T2) {
        let (first, second) = input
        return (repeat each first, second)
    }

    public func unapply(output: (repeat each T1, T2)) -> ((repeat each T1), T2) {
        guard let tuple = output as? ((repeat each T1), T2) else {
            fatalError("Could not convert output tuple type")
        }
        return tuple
    }
}

While functional, this approach depends on macOS 14 for its use of parameter packs in generics, which makes it incompatible with earlier versions. As a result, introducing Take2.Map strikes a practical balance: it ensures backward compatibility while adding only minimal complexity to the surface area.

For verification, refer to VariadicTests.swift, which includes passing tests.

PS: I recommend exploring further if and to what extent the existing Map infrastructure can be leveraged over Take2.Map.

coenttb and others added 2 commits January 10, 2025 07:00
… used in a Parserbuilder.

This update removes the need for all Take*n overloads except Take2, and is fully backwards compatible.

The parameter pack buildblock relies on a new Take2.Map, which is a tradeoff. On the one hand, we'd prefer to use a Conversion and MapConversion here as the elegant solution. However, consider this mock/theoretical TupleConversion:

public struct TupleConversion<each T1, T2>: Conversion {
  public typealias Input = ((repeat each T1), T2)
  public typealias Output = (repeat each T1, T2)

  public func apply( input: ((repeat each T1), T2)) -> (repeat each T1, T2) {
    let (first, second) = input
    return (repeat each first, second)
  }

  public func unapply( output: (repeat each T1, T2)) -> ((repeat each T1), T2) {
    guard let tuple = output as? ((repeat each T1), T2) else {
      fatalError("Could not convert output tuple type")
    }
    return tuple
  }
}

This would require macOS 14, as it uses parameter packs in generics. This is not fully backwards compatible. I therefore am of the opinion that the tradeoff with introducing Take2.Map is worth it, as this approach maintains backwards compatibility, while only adding a little surface area.

See also VariadicTests.swift for passing test.
Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! I'll bump the package file on main after merging since parameter packs require 5.9, but that's been around for awhile now.

Feel free to experiment with your TupleParser using compiler conditionals or availability checking in another PR if you think it'll improve performance on platforms that support it!

@stephencelis stephencelis merged commit d34ca0b into pointfreeco:main Jan 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants