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

Fix: SplitByType macros #133

Merged
merged 3 commits into from
Nov 16, 2024
Merged

Fix: SplitByType macros #133

merged 3 commits into from
Nov 16, 2024

Conversation

HollandDM
Copy link
Contributor

@HollandDM HollandDM commented Nov 14, 2024

This is the continuation of #116 and addresses these issues

  • Failed to compile on Scala >= 3.3.4: Scala 3.3.4 has a fix for this issue, which unfortunately breaks the way we build up the macros. I changed the building process to use Varargs, which should be more durable than our previous ad-hoc mechanism. Compiled with scala 3.3.3, 3.3.4 and 3.5.2, so it should be good now.
  • Anonymous class: The compiler is complaining about all those { case t: T => t} and { case _: V => v } partial function that we inlined into our macros. As @raquo said, this should not be a problem for us, because PartialFunction in scala is a trait, so it always initiates new instance when defined, inlined or not. IMO, we don't have to fix this. But to please the compiler, I've created some dummy classes that act as the placeholders for those partial functions. The macro implementation will replace them with the actual partial functions.
  • Using too many cases at once slows down the compiler, eventually resulting in an OOO crash: My previous ad-hoc way to build up macro's data is quite dumb and slow, as it traversed the AST to extract relevant data everytime a new handleCase was added. The new mechanism takes advantage of scala's internal Varargs, so the extraction will be a lot quicker. I added a test which contains 100 handleCases and run it in sbt with -javaopt = -Xmx1024m -Xms1024m and everything seems to be fine.

@HollandDM HollandDM requested a review from raquo as a code owner November 14, 2024 08:13
@HollandDM
Copy link
Contributor Author

@raquo the CI survived 100 cases test, I think this should be good enough for your use cases

@raquo raquo merged commit f19d48b into raquo:master Nov 16, 2024
1 check passed
@raquo
Copy link
Owner

raquo commented Nov 16, 2024

Thanks! Everything looks good now, I've tested it in the laminar-demo, and it behaves as expected.

I made one small change – handleValue(Foo)(...) now accepts :=> Out by-name instead of a Signal[V] => Out callback, to match Waypoint's collectStatic API. There's not much point in listening to a signal of a single singleton value, at least in the applications I'm imagining, and it's easy enough to just use handleCase for more complex use cases. So I think it's a good tradeoff to avoid the need for _ => for every static page in the URL router.

I still need to do a bunch more work on this, incl. writing docs and ideally a test in Laminar repo, but I plan to release this and some other changes in 17.2 soon.

@raquo
Copy link
Owner

raquo commented Nov 16, 2024

Yall can try this in 17.2.0-M1 preview. More info in discord

@raquo
Copy link
Owner

raquo commented Nov 17, 2024

@HollandDM Currently the macros require an import, but I would like to make them available without any special imports.

To make extension methods available without an import, typically I would for example move extension [Self[+_] <: Observable[_], I, O](inline matchSplitObservable: SplitMatchOneObservable[Self, I, O]) methods from SplitOneMacros right into the SplitMatchOneObservable object, to make these methods available on SplitMatchOneObservable type without imports.

I would do the same for extension [Self[+_] <: Observable[_], I](inline observable: BaseObservable[Self, I]) { – move it into the companion object of Observable, where other such implicit conversions are defined. Except I would need to trick Scala 2 into compiling, so uh... probably I would make object Observable extend a new MacroImplicits trait, which would be empty for Scala 2 but would contain this extension method in Scala 3. That's fine I guess... (though if you have better ideas – please share)

My question for you is – would it be ok to move your macro extension methods this way, or is there some reason, whether functional or conventional / stylistic, where this approach would be frowned upon? If it weren't for macros, I would be fine with this, just wanted to check with you if there are any macro-specific considerations for this. I haven't started on this yet.

@yurique
Copy link
Contributor

yurique commented Nov 17, 2024

make object Observable extend a new MacroImplicits trait, which would be empty for Scala 2 but would contain this extension method in Scala 3.

that is how we all do it :)

@HollandDM
Copy link
Contributor Author

@raquo scala 3 macros tend to be in the same files as the inline def which translated into them. So maybe you will have to move some macro functions as well. I'm not sure what would happen if they are separated, you can try it if you want.
About the approach, I think your suggestion is fine. Mose of the big OSS always have some traits like that, named like *PlatformSpecifics (for JS/JVM/Native specific) and *VersionSpecific (for scala 2.12/2.12/3). So I think we can follow the naming convention.
Other than that, I'm fine with your restructure.

@raquo
Copy link
Owner

raquo commented Nov 23, 2024

@HollandDM I made splitMatchOne and splitMatchSeq available without an import (see macro-implicits branch diff here), but I can't figure out how to do the same for the subsequent methods such as handleCase.

In SplitMatchOneMacros, we have both extension methods, and impl methods. The extension methods call into impl methods. What we need is to move or export this:

extension [Self[+_] <: Observable[_], I, O](
  inline matchSplitObservable: SplitMatchOneObservable[Self, I, O]
) { ... }

into object SplitMatchOneObservable. That way, whenever we have a value of type SplitMatchOneObservable, those extension methods will be picked up without any import.

However, I can't figure out how to accomplish that. Naively moving the extension methods and updating impl method references with SplitMatchOneMacros. results in errors like:

[error] -- Error: /Users/raquo/code/scala/airstream/src/main/scala-3/com/raquo/airstream/split/SplitMatchOneObservable.scala:43:6
[error] 43 |      SplitMatchOneMacros.handleCaseImpl('{ matchSplitObservable }, '{ casePf }, '{ handleFn })
[error]    |      ^^^^^^^^^^^^^^^^^^^
[error]    |      Malformed macro parameter
[error]    |
[error]    |      Parameters may only be:
[error]    |       * Quoted parameters or fields
[error]    |       * Literal values of primitive types
[error]    |       * References to `inline val`s

And they're stubborn. I tried various strategies with importing, exporting, extending, but none solved the issue. "Best" I got was when trying to export SplitMatchOneMacros.{*, given} from SplitMatchOneObservable (I was trying to export the extension methods):

[error] 384 |      .handleType[Baz] { (baz, bazSignal) =>
[error]     |                 ^
[error]     |method handleType in object SplitMatchOneObservable does not take parameters

Normally I can sort such things out, but I don't understand macros or their concepts and constraints, so I'm poking at things blindly. Any ideas how to make this work? Would be a bummer if the new feature required additional imports...

@HollandDM
Copy link
Contributor Author

@raquo let's me have a look, will contact back to you later

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