Add case classes for Ember client and server configuration

[sideeffffect]

Case classes are incredibly useful for configuration, e.g. with PureConfig. They are tricky with regards to backward compatibility. But with the way as done here, backward compatibility will be maintained.

https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html#evolving-code-without-breaking-binary-compatibility

[armanbilge]

Sorry 👎 regardless of the guide, compatibly-evolving case classes is a significant burden on maintainers and (new) contributors.

Have you considered contributing this to PureConfig's http4s module instead?

https://github.com/pureconfig/pureconfig/tree/master/modules/http4s

[sideeffffect]

PureConfig maintainers refuse to have config case classes like this in their repository.

I think this is a good fit or https://github.com/http4s/http4s It is configuration for http4s after all. And the burden isn't so great, especially compared to the huge benefits for the users. New parameters just have to be added to copy and a new apply needs to be created. It's copy-paste, really.

[armanbilge]

PureConfig maintainers refuse to have config case classes like this in their repository.

🤷

And the burden isn't so great, especially compared to the huge benefits for the users. New parameters just have to be added to copy and a new apply needs to be created. It's copy-paste, really.

IMHO this is idealistic :) not all changes are as simple as adding a new parameter. What happens if you want to change an existing one for example? In my experience case classes eventaully cause pain.

I'd be happy to see Scala get a proper data class for this usecase or these case classes to appear in a new library :)

[sideeffffect]

What happens if you want to change an existing one for example? ... happy to see Scala get a proper data class for this usecase

But aren't we in that case in the same situation as with the current Builders (EmberServerBuilder, EmberServerBuilder)? What is problematic about case classes is that adding a new field is counterintuitively backwards incompatible. But other things (e.g. change an existing field, like you write) are compatible the same way as Builders are or data classes ever will be.

So if you're not worried about adding a new field (I made special care to make it easy for the case classes in this PR), what benefit are we getting from having only Builders and staying away from case classes? I totally understand the need to keep things easy in order not to overload maintainers. What is is specifically that you're worried about? That somebody would add a new field without making the necessary changes to keep the backward compatibility? I'm hopeful we can find some way to enable this, because there's a huge demand for this and it would be sad to let it go unsatisfied.

Please hear me out, maybe together we can figure this out 🙏

[valencik]

there's a huge demand for this and it would be sad to let it go unsatisfied

@sideeffffect could you perhaps say more about this demand? A brief search through past issues hasn't really convinced me this is the case.

There's your prior work in #3081 which ultimately hit on the same issue, case classes increase maintenance burden.

Also perhaps an interesting and much older take on configuration: #105

[sideeffffect]

@valencik the reasoning behind this is that if you have a case class, you can derive from it various useful tools, like for example the ConfigReader from PureConfig.

Yes, that old PR #3081 had just plain case classes, which aren't backward compatible. But this PR is different, right? This one is written with compatibility in mind.

My hope is that the maintenance burden wouldn't actually be that high. It's adding a new field that's tricky, and as I've written in the comment, then you "only" need to add it to copy and create a new apply. That's it. Or am I missing something?

[valencik]

Ah, sorry for the confusion, I understand the use case. I'm curious about the demand you mentioned. Are you seeing lots of people ask for this in other places?

[sideeffffect]

In every company I've worked with, people want/need config case classes like this and have to workaround their lack. Having "official" config case classes directly in the http4s project itself would fix the problem for oh so many people once and for good.

[armanbilge]

But aren't we in that case in the same situation as with the current Builders

No, we're not in the same situation. You can easily change a parameter type in the current builder encoding (assuming there is a mapping from the old one) but doing this for a case class will be much much more difficult and involve splitting into Scala 2 and Scala 3 specific sources. I've seen this happen before.

We can argue this ad nauseam :) I am very sorry to be dismissive. But I've done this sort of case class surgery far too many times and I (personally) don't want to take on this maintenance burden. Our goal is to make choices that maximize our chances of being able to make useful changes without breaking compatibility and while it is hard to predict what these changes are and what they will look like, I can say for certain our ecosystem wouldn't work the way it does without this dedication to bincompat.

The best I can offer you is that it is now easier than ever to publish your own lib with sbt-typelevel. So if you are confident the maintenance burden will be minimal then that seems like a good option :)

[rossabaker]

I don't know that this encoding was even feasible when we designed the builders. The meaning of private in case classes and how it suppresses certain features has evolved. So let's look with fresh eyes.

Unchanged:

• We still have to maintain the maximal private constructor.
• We still have to maintain the minimal public constructor.
• We still have to maintain the with* methods.

Pros:

• We wouldn't have to maintain copy anymore.
• Plays nicer with some popular frameworks.
  • But: there are usually ways around this (e.g., PureConfig's ConfigReader)
    • But but: someone has to maintain that somewhere, and for dependency reasons, not in this repo.
• Users can query the fields on a running backend
  • But: they provided the fields in the first place, there are other ways, just maybe less convenient
• Users get a .toString
• Users get a .hashCode and .equals
  • But: not all fields have sensible definitions of those

Cons:

• We have to maintain the builder and the case class through a deprecation cycle
• Breaking change when we drop the deprecated builders
  • But: easy scalafix. I'm a hard no on this without a scalafix.
• Loss of freedom to evolve the representation
  • When a boolean becomes a ternary, we get two booleans and represent invalid state.
  • When a setting is obsolete, we expose obsolete state.
  • Can generally deprecate and/or delegate the with* methods when this happens
  • But: addition is much more common than change or deletion
  • But: can maybe demote fields to deprecated defs? I haven't tried.
• Product methods get silent semantic changes on each addition
  • But: nobody uses Product anyway

In a greenfield, the case class encoding, with privates as described in that link, is compelling. (I'd want to practice some breaking changes before I was convinced.) I'm not sure I see enough demand or upside yet to justify a breaking change.

[rossabaker]

involve splitting into Scala 2 and Scala 3 specific sources

Why is this? The only difference I spotted in the doc is the syntax.

[armanbilge]

Why is this? The only difference I spotted in the doc is the syntax.

Because of Mirror.

[sideeffffect]

Just to be clear, I'm not proposing any breaking change, let alone removing the builders. The builders are more general, and server a little bit different purpose IIUC. The config classes are designed only for data (which means equals/hashCode is well defined there).

To the best of my knowledge (but I don't know everything, of course 😅 ), my current proposal should work for both Scala 2 and 3. How would Mirror break this?

[sideeffffect]

You can easily change a parameter type in the current builder encoding (assuming there is a mapping from the old one) but doing this for a case class will be much much more difficult

Could you please explain this a little bit more. I don't really understand the different between case class and a Builder in this situation. Maybe even a super small example would make it clear to me. Thank you for engaging with me on this, I appreciate it. 🙏

Again, I very much value http4s maintainers' time and wouldn't want to burden them unreasonably.

[armanbilge]

Maybe even a super small example would make it clear to me.

Sure.

Suppose I have:

case class Bar(value: String)
case class Foo(bar: Bar)

And I want to change it to:

case class Baz(value: String)
case class Foo(baz: Baz)

How would you do that compatibly? :)

I know this seems stupidly trivial. A real-world example of this kind of change is swapping a type from something that is JVM-only for an isomorphic type that exists on all three platforms.

Ross offered another scenario where this sort of thing can happen.

When a boolean becomes a ternary, we get two booleans and represent invalid state.

[rossabaker]

I think it can be binary and source (with deprecations) compatible using the encoding in the scala-lang.org article. We can't preserve semantic compatibility, which is a new problem.

Addition is easy, and just like the guide:

• Add baz: Baz to private constructor.
• Add default baz to public constructor.
• Add withBaz(baz: Baz): Foo.

Removal less so:

• Deprecate old public constructor with Bar
• Deprecate withBar.
• Add new public constructor with Baz
• Remove bar from private constructor
• Create deprecated def bar: Bar.
• bar and withBar can operate through baz if it's isomorphic, or be a dummy and no-op value if not.
• ... I think this passes MiMa? Maybe the deprecated bar needs to be a val?

We've changed the semantics of Serializable, Product, hashCode, equals, and toString, but not their interfaces. (I only mentioned Product changes in my earlier comment, but it's the same for all.) We've maybe also broken the expectation that Foo.default.withBar(bar).bar === bar, if bar is not related to baz. None of this impacts builders, because builders are "write only".

Here's where I get fuzzy, because I don't use Scala 3 except to fix crossbuilds: I see how we've similarly broken things derived from Mirror semantically. But the generated mirrors are not part of the ABI, right? And anything we derive with them produce code against the stable public API, no? Now we have the same kind of problems, but extended past the universal APIs, but I don't see how separate Scala 2 or 3 branches help. I don't doubt you're right, but I'm missing a piece of the puzzle.

[armanbilge]

Hmm, I may have lost the thread on this a bit 😅 what are we aiming for here?

If we are saying so long as it's source-compatible and binary-compatible, ship it, I don't care if it crashes at runtime, then yeah, I suppose this is good.

If we decide we do care about such things (I think this is the "semantic compatibility" Ross is referring to?) then it gets a lot harder. Firstly, the tooling won't help us stay compatible here, and secondly, it will require mucking about with Scala version-specific APIs for derivation.

In any case, to aid exploration, I created a little sandbox in armanbilge/sandbox@0ab5302. That sandbox includes:

1. personOld and personNew, representing two versions of a library. You can use personNew/mimaReportBinaryIssues to check their compatibility.

2. lib, a 3rd-party library using derivation that depends on personOld.

3. app, an application depending on lib but depending on personNew. You can use app/run to run it.

The current demo shows that even simply adding a field to a case class while preserving binary- and source-compatibility as explained in the guide can cause errors at runtime. Additionally, this particular error occurs only on Scala 3, not Scala 2.

[rossabaker]

This is both an illuminating and horrifying example. Thank you! When I say semantic changes, I mean things that compile, but whose behavior changes:

• Person("ross").toString silently changes from "Person(ross)" to `"Person(ross,-1)".
• Person.fromProduct(p) silently changed from needing at least one element in p to at least two.

The first one? Thou shalt not use toString for serialization.

The second one works fine if the derivation is kept with the class. Thou shalt not derive orphan instances? Great, but where is the full list of these commandments, and who is going to make a linter out of them? Are we going to keep blaming apps one crash at a time?

[sideeffffect]

Thanks for investigating this with me, I really appreciate it 🙇

Thankfully the problem with fromProduct is solvable by manually overriding

  def fromProduct(p: Product): Person = p.productArity match {
    case 1 =>
      Person(
        p.productElement(0).asInstanceOf[String]
      )
    case 2 =>
      Person(
        p.productElement(0).asInstanceOf[String],
        p.productElement(1).asInstanceOf[Int]
      )
  }

I know, it's yet another step to do when adding a new field, but I think it's still easy (albeit a bit embarrassing). Then don't have the Scala 3 problem with as shown here armanbilge/sandbox@0ab5302

btw I mentioned this fromProduct issue in https://contributors.scala-lang.org/t/can-we-make-adding-a-parameter-with-a-default-value-binary-compatible/6132/8?u=sideeffffect

Person("ross").toString changes

Not a problem IMHO.

And removing a member isn't backward compatible regardless if you do case class or a builder, right?

[armanbilge]

Nice job! But I think this is just the tip of the iceberg :) in armanbilge/sandbox@c361606 I demonstrated another runtime crash due to a semantic incompatibility, even after applying your fromProduct fix. Try appOld/run vs appNew/run.

[sideeffffect]

But that's not a problem with case classes, let alone http4s. That's just a bug in Scala 3 derivation of ConfigReader in Pureconfig. It has no support (yet) for default arguments. pureconfig/pureconfig#1488 It would fail with plain

case class Person(name: String, age: Int = -1)

and

{ name = foo }

https://scastie.scala-lang.org/HdRYEUzCRme8LH4zffzlPQ

[armanbilge]

Hmm, I'm not sure I agree.

1. It requires us to encode defaults in a very specific way, with no help from tooling. So, one more commandment to remember.

2. Scala 3 Mirror which is used for derivation doesn't appear to encode any notion of defaults. So is it really fair to expect derived typeclasses to know about it?

[sideeffffect]

Maybe I'm misunderstanding your post, but it's not that it requires us to encode defaults in a specific way, it just doesn't work. But picking up defaults is super important feature of PureConfig, so I hope they will fix it one way or another, PureConfig wouldn't be as useful without it.

[sideeffffect]

Hello again @armanbilge @rossabaker @valencik . Are you guys open to continuing the discussion about this? Do you think we could find a common ground about this?

[armanbilge]

but it's not that it requires us to encode defaults in a specific way, it just doesn't work.

Sure, but to make it work requires encoding defaults in a specific way, that is not part of Mirror :)

Are you guys open to continuing the discussion about this?

I'm open to seeing a demo that this actually works on Scala 2 and 3. I understand that's a bit tricky at the moment since Scala 3 support is still spotty.

However at the moment I'm still not convinced that we can actually evolve compatibly here without making additional assumptions or following "commandments". I think compatible evolution of case class like datatypes that support derivation still needs to be addressed at the language level.

[sideeffffect]

Would this count as the demo? armanbilge/sandbox#1
MiMa is happy:

+personNew/mimaReportBinaryIssues

Yes, PureConfig doesn't work with default values in Scala 3. But that's a PureConfig issue, not a binary incompatibility issue -- you don't get a runtime crash.

[armanbilge]

Would this count as the demo? armanbilge/sandbox#1

I wrote a comment there.

MiMa is happy:

As I established in #7066 (comment), satisfying MiMa doesn't mean we can be confident that it won't crash at runtime for some user of our library.

[sideeffffect]

I wrote a comment there.

Replied there too 🙂

MiMa doesn't mean we can be confident that it won't crash at runtime

But is that really true? The code is binary compatible, there are no (potentially used) members that suddenly disappear. It's true that PureConfig for Scala 3 doesn't work with default arguments. But there is no crash. If you try parsing a config that doesn't have all the fields of the case class, Scala 3 PureConfig won't be able to parse it. But it won't crash, it will just fail parsing. That's just a bug in Scala 3 PureConfig, it's definitely not a problem of binary compatibility in http4s.

Of course I agree that it would be better if Scala 3 PureConfig worked with default arguments. But that's a responsibility of the PureConfig developers.

Let's suppose Scala 3 PureConfig is suddenly fixed, but somebody, could be me, create my own, buggy derivation library for something, will it also be a reason for not accepting a PR like this one? Or what if I created my own configuration library ala PureConfig that didn't suffer from this problem, would it suddenly make this PR acceptable? It seems to me that you're saying that if there is (could?) be a library, that could use the code (configuration case classes in our case) in this PR in a buggy way, we can't accept the PR. But isn't that overly strict? Is this strictness even applied to other PRs? I don't think bugs in unrelated (not a dependency) projects should hinder the http4s project. Aren't we letting the perfect to be the enemy of the good with this approach?

[sideeffffect]

The problem lies only with PureConfig's buggy implementation. There are other Scala 3 libraries that don't break with case classes and their default values and work well when trying to decode an "old" string. One example is uPickle. As a proof, I've prepared this PR:

armanbilge/sandbox#2

I hope that this shows that case classes (with some special care, of course) can be part of http4s and don't pose a backward compatibility risk.
/cc @armanbilge @rossabaker

[armanbilge]

Thanks!

The problem lies only with PureConfig's buggy implementation.

I don't think I agree that it is a bug. As I state in scala/docs.scala-lang#2788 (comment), the Mirror API offers no way to indicate default values, so currently this can only be done in ad-hoc ways, such as the strategy uPickle is using. Since default values are so important, it seems like it should graduate to a language-level feature before we settle on an ad-hoc strategy.

[sideeffffect]

we settle on an ad-hoc strategy

By "we" you mean http4s? I'm asking, because this doesn't have anything to do with http4s. This is a concern of other libraries, e.g. uPickle, right?

It's not that I'm against "graduate to a language-level feature". But I don't think deficient (regardless if we call it a "bug" or not) implementations of other, non-http4s, libraries should hold http4 back.

Why couldn't http4s and uPickle users enjoy nice features? Only because PureConfig doesn't do as good job as uPickle does? Users which would like to use http4s with PureConfig should take it up with PureConfig developers. http4s shouldn't (and can't) be responsible for all Scala libraries or even the language itself.

[armanbilge]

By "we" you mean http4s? I'm asking, because this doesn't have anything to do with http4s.

Yes, I mean http4s. It does concern us. How do we add default values? There is no place to add them in the Mirror API.

Only because PureConfig doesn't do as good job as uPickle does? Users which would like to use http4s with PureConfig should take it up with PureConfig developers

To clarify, even if PureConfig did fix this "bug", I still believe this is an issue because default values are not actually supported by Mirror, and Mirror is the way to enable and support derivation on Scala 3.

http4s shouldn't (and can't) be responsible for all Scala libraries or even the language itself.

Exactly :)

[sideeffffect]

How do we add default values? There is no place to add them in the Mirror API.

The way I did it in the PR I linked above seems like a good way. It works with at least some Scala 3 libraries, e.g. uPickle.

default values are not actually supported by Mirror

Yes, that is true.

Mirror is the way to enable and support derivation on Scala 3

A way. A way which apparently doesn't work well with default arguments (at least currently). But there are libraries which apparently don't use it, or use it differently from what PureConfig does and thus work fine with default arguments. One such is uPickle.

---

To recapitulate, case classes (with special care) don't pose risk for backward compatibility via

• bytecode -- you won't get method not found errors at runtime
• default arguments -- you'll be able to "decode" old serialized values which contain only a subset of current fields, if you use a library which supports working with default arguments, like PureConfig for Scala 2 or uPickle for any version of Scala (PureConfig for Scala 3 just doesn't support default arguments at all, but that is a bit different issue from evolving case classes in a backward compatible manner)

But in contrast to the bytecode of http4s artifacts, the 3rd party libraries http4s users use are not under http4s maintainers control. And thus, I think, shouldn't be reasons for accepting/rejecting a PR to http4s.

I think I've presented the case as best as I could, I don't think I can add or do anything more. Now it's up to the @http4s maintainers to decide on this PR.