String interpolator parsers by stephenjudkins · Pull Request #339 · typelevel/cats-parse

stephenjudkins · 2021-12-30T23:17:39Z

Fixes #329.

I just dragged this over the finish line and am open to any suggested changes. Was a useful if somewhat painful way to dip my toes into Scala 3

codecov-commenter · 2021-12-30T23:22:55Z

Codecov Report

Merging #339 (3d5164a) into main (ae38c13) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
- Coverage   96.56%   96.37%   -0.20%     
==========================================
  Files           9        9              
  Lines        1049     1049              
  Branches       94       94              
==========================================
- Hits         1013     1011       -2     
- Misses         36       38       +2

Impacted Files	Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala	`96.18% <0.00%> (-0.24%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae38c13...3d5164a. Read the comment docs.

johnynek · 2021-12-31T00:17:59Z

cc @s5bug

johnynek

This is really awesome! I'm so excited you tackled this.

johnynek · 2021-12-31T00:18:56Z

core/shared/src/main/scala-2/cats/parse/StringInterpolation.scala

+object StringInterpolation {
+
+  implicit class Helper(val sc: StringContext) extends AnyVal {
+    def parser(args: Any*): Parser[Any] = macro StringInterpolationMacros.parser


I am not very familiar with macros honestly... I always have to look up details.

So, I guess that since this is a white box macro that the type gets refined (as checked in the test below)?

Yup correct, this needs to be a whitebox macro to get the type refined at each call site.

johnynek · 2021-12-31T00:24:10Z

core/shared/src/main/scala-2/cats/parse/StringInterpolation.scala

+        val (matched, last) = stringParts.splitAt(args.length)
+
+        val parsers: List[Tree] = matched.zip(args).map { case (a, b) =>
+          if (a.isEmpty) b.tree else q"${lit(a)} *> $b "


I'm wondering about what happens if b is a Parser0. It seems we could often handle it (as long as there is at least one string value, or another Parser. But I guess we can't get the type of b right?

should we test that with Parser0? I guess we could make sure it works as long as there is a leading Parser or string?

johnynek · 2021-12-31T00:27:26Z

core/shared/src/test/scala/cats/parse/StringInterpolatorTest.scala

+    assertEquals(
+      arrow.parseAll("spam,1234 -> quux"),
+      Right((true, BigInt(1234), 42))
+    )


could we add a few assertions on compileErrors(...)

https://scalameta.org/munit/docs/assertions.html

it would be good to see some failures and make sure they fail in both scala 2 and 3 (although the messages may be different).

johnynek · 2021-12-31T00:29:10Z

core/shared/src/test/scala/cats/parse/StringInterpolatorTest.scala

+    val foo: Parser[Boolean] = Parser.string("spam").as(true) | Parser.string("eggs").as(false)
+    val bar: Parser[BigInt] = Numbers.bigInt
+    val quux: Parser[Int] = Parser.string("quux").as(42)
+


can we exercise with 0 args, 1 arg up to say 3 (to exercise non-tuples)
so, I guess val x: Parser[Unit] = parser"foo" should work. val x: Parser[A] = parser"foo$aParser" and then at 2 we should have tuples?

or do we require 2 or more (or 1 if we wrap with tuple1)?

Wrote tests to exercise these cases. Here's how it currently works:

empty string: compile error

non-empty string with no interpolated parsers: Parser[Unit] equivalent to Parser.string("...")

string with single interpolated Parser[A]: Parser[A]

string with n interpolated Parser[{A, B, C...}]: Parser[(A, B, C...)]

johnynek · 2021-12-31T00:48:15Z

core/shared/src/test/scala/cats/parse/StringInterpolatorTest.scala

+  test("parser string interpolator") {
+    val foo: Parser[Boolean] = Parser.string("spam").as(true) | Parser.string("eggs").as(false)
+    val bar: Parser[BigInt] = Numbers.bigInt
+    val quux: Parser[Int] = Parser.string("quux").as(42)


could we write this as parser"quux".as(42)? does parser bind tighter than method application?

Yes, it does

s5bug · 2021-12-31T04:02:38Z

This is awesome! Thank you so much!

stephenjudkins · 2021-12-31T06:43:30Z

Added more code to deal with Parser0. Turns out, in Scala 2.x since we're dealing with untyped trees things just work out like we'd expect. Scala 3 needs more type plumbing to get the same behavior working

A change from previous discussions is that parser"" now generates a Parser.unit:Parser0[Unit]

regadas

this looks great! thanks for taking this 😄 just left some minor things.

I was thinking ... would it be worth separating this into it's own module?

regadas · 2021-12-31T12:04:30Z

core/shared/src/main/scala-2/cats/parse/StringInterpolation.scala

+
+object StringInterpolation {
+
+  implicit class Helper(val sc: StringContext) extends AnyVal {


Suggested change

implicit class Helper(val sc: StringContext) extends AnyVal {

implicit class Helper(private val sc: StringContext) extends AnyVal {

regadas · 2021-12-31T12:13:53Z

core/shared/src/main/scala-3/cats/parse/StringInterpolation.scala

+    transparent inline final def parser(inline args: Any*): Any = ${ parserImpl('sc, 'args) }
+  }
+
+  def parserImpl(sc: Expr[StringContext], args: Expr[Seq[Any]])(using q: Quotes) = {


Suggested change

def parserImpl(sc: Expr[StringContext], args: Expr[Seq[Any]])(using q: Quotes) = {

private def parserImpl(sc: Expr[StringContext], args: Expr[Seq[Any]])(using q: Quotes) = {

regadas · 2021-12-31T12:19:51Z

core/shared/src/test/scala/cats/parse/StringInterpolatorTest.scala

+
+  }
+
+  test("including a Parser0") {


could we also assert val p: Parser0[Unit] = parser"" ?

regadas · 2021-12-31T12:32:02Z

core/shared/src/main/scala-2/cats/parse/StringInterpolation.scala

+
+}
+
+class StringInterpolationMacros(val c: whitebox.Context) {


I believe we could make this one private

Suggested change

class StringInterpolationMacros(val c: whitebox.Context) {

private class StringInterpolationMacros(val c: whitebox.Context) {

johnynek · 2021-12-31T18:54:04Z

Rather than nit on this, I'm going to merge and then will iterate.

I'll send a followup PR with @regadas 's suggestions.

Thanks so much!

I look forward to a release with this!

stephenjudkins · 2021-12-31T21:13:33Z

Well, it's functional now in but I thought on it, and: the Parser vs Parser0 aspect of this is currently not ideal and inconsistent between Scala 2/3. I'm send a followup PR with some failing/non-compiling tests as a starting point for discussion

johnynek · 2021-12-31T22:18:39Z

@stephenjudkins take a look at my PR where I also tried to factor the code similarly and added a few more tests.

String interpolator parsers

a2363f8

stephenjudkins force-pushed the sdj/string-interpolator-parsers branch from 1305673 to a2363f8 Compare December 30, 2021 23:19

johnynek reviewed Dec 31, 2021

View reviewed changes

stephenjudkins added 5 commits December 30, 2021 21:59

more tests, fixed more cases

b31347f

scalafmt

661a016

+scalafmtAll

fa2ae1c

Explicitly support including and generating Parser0

a50826d

Support interpolating/generating Parser0

3d5164a

regadas reviewed Dec 31, 2021

View reviewed changes

johnynek merged commit e0a50f4 into typelevel:main Dec 31, 2021

johnynek mentioned this pull request Dec 31, 2021

Return Parser if parser macro finds a non-empty string #340

Closed

johnynek mentioned this pull request Mar 28, 2022

Remove parser interpolator #399

Merged


		object StringInterpolation {

		implicit class Helper(val sc: StringContext) extends AnyVal {

	implicit class Helper(val sc: StringContext) extends AnyVal {
	implicit class Helper(private val sc: StringContext) extends AnyVal {

	def parserImpl(sc: Expr[StringContext], args: Expr[Seq[Any]])(using q: Quotes) = {
	private def parserImpl(sc: Expr[StringContext], args: Expr[Seq[Any]])(using q: Quotes) = {

	class StringInterpolationMacros(val c: whitebox.Context) {
	private class StringInterpolationMacros(val c: whitebox.Context) {

Uh oh!

Conversation

stephenjudkins commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnynek commented Dec 31, 2021

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenjudkins Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s5bug commented Dec 31, 2021

Uh oh!

stephenjudkins commented Dec 31, 2021

Uh oh!

regadas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek commented Dec 31, 2021

Uh oh!

stephenjudkins commented Dec 31, 2021

Uh oh!

johnynek commented Dec 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stephenjudkins commented Dec 30, 2021 •

edited

Loading

codecov-commenter commented Dec 30, 2021 •

edited

Loading

johnynek Dec 31, 2021 •

edited

Loading

stephenjudkins Dec 31, 2021 •

edited

Loading