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

Singleton type subtyping fix #240

Merged
merged 7 commits into from
Nov 19, 2021

Conversation

kitlangton
Copy link
Member

@kitlangton kitlangton commented Nov 19, 2021

Fixed a little issue we ran into with sub typing and dotty.

Also, a couple of questions, now that I'm understanding some more of this 😄

  1. Why the Pickling? Is that for performance/making sure the generated macro code isn't too big?
  2. Why the difference between a base and an inheritance db?
  3. Oh, and would it be possible to make Tag Serializable? :)
  4. UPDATE: Also! Would it be cool if I added scalafmt to the project? 😄

@pshirshov
Copy link
Member

pshirshov commented Nov 19, 2021

  1. Why the Pickling? Is that for performance/making sure the generated macro code isn't too big?
    

Performance. We've tried many different approaches and selected the quickest/most compact library we've found (though we had to fix some bugs there). Also, as you can see, we incorporated the library into our code, the ability to maintain a fork was a factor too.

  1. Why the difference between a base and an inheritance db?
    

One of them holds links between unparameterized type constructors with all the arguments data omitted (e.g. List isSubtypeOf Seq, something similar to how subtype checks work on Class) and another one - between proper parameterized types (e.g. List[Int] isSubtypeOf Seq[Int]). Unfortunately both the predicates seem to be necessary in case we want to support runtime tag combination, I've tried different models and this one gave me the best quality of the simulation.

  1. Oh, and would it be possible to make Tag Serializable? :)
    

Yes, but you won't want to use java serialization for it. From what I can remember , ScajaJS doesn't support java serialization, also it's slower, also less portable.

  1. UPDATE: Also! Would it be cool if I added scalafmt to the project? smile
    

@neko-kai will do it, thanks for the reminder.

@neko-kai
Copy link
Member

@kitlangton
Thank you! 🙏

  1. Yes, it's for avoiding trees. There was 20x perf difference between splicing trees and packing into a string. One optimization we don't do here (that we do elsewhere) is setting tree types via low-level API to avoid typechecking entirely. However the impact from this won't be as dramatic and I'm not sure whether/how to do that in Scala 3 (and whether it's not doing that on its own already for typed quotes)
  2. Base / fullDB is fully parameterized inheritance tree, inheritanceDB contains just the unparameterized type constructors.
  3. If MiMa passes sure. The reason not do it was that Tag is a summoning wrapper, not data, but it could still end up in a closure in raw form, so why not.
  4. I'll add it now, but I'll exclude boopickle to not make merging from upstream even harder if it would be necessary.

@neko-kai neko-kai merged commit 0bd6683 into zio:develop Nov 19, 2021
@neko-kai
Copy link
Member

Ah, wait, scalafmt is already enabled for the project, it just didn't work in Scala 3 sources because runner.dialect wasn't set to scala3 😅

@kitlangton
Copy link
Member Author

You both rock. You also both answered all of my questions simultaneously :-) but slightly differently, so it’s extra helpful! Thanks again you two

@kitlangton
Copy link
Member Author

I believe it’s possible to set different dialects/settings for different folders, if that helps.

@neko-kai
Copy link
Member

I believe Scala3 dialect is a superset of all the others in scalafmt right now, so it won't be necessary until something breaks...

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.

3 participants