Allow user-defined macro methods

[Blacksmoke16]

Reopens #8836, fixes #8835.

As it turns out adding next and changing the syntax to macro def is not a trivial endeavor. Because of this, and since these changes are BC, I propose to just merge the current state of this to at least have something.

This is based on the original PR by @asterite. I added an additional commit to add some specs for the new .each and .each_with_index macro methods as well.

We can always iterate on this to improve it in the future. Considering the breaking change when that happens would be simple to fix, and I doubt this'll be used as much as some stdlib features; it's probably fine to handle that at a later time.

[Sija]

As much as I dig the feature, the idea of merging it without the support from its author seems off. It's not only bells and whistles but also, fixing potential bugs, uncovered edge-cases and future maintenance - all of which require will and understanding the code. If @asterite doesn't feel confident about it, then who will?

[asterite]

Oh, I feel confident about the code. I just think we need to think this a bit more. Reusing the macro syntax feels like a hack, and the implementation is a bit hacky. It just needs a proper design phase.

[Sija]

@asterite By it I meant the whole feature (incl. the design), not just the implementation.

[Blacksmoke16]

It depends on how we want to view this feature.

On one hand it can be seen as an MVP that is a pretty small diff, backwards compatible, and useable. On the other it could be viewed as a proof of concept of something that could be refined/improved upon more.

IMO both of these are true however, due to the reason mentioned earlier, it would be more helpful to release this MVP, have people try it out, and let that inform future possible iterations. Otherwise I fear that it'll be stuck in the perpetual design phase.

Given I don't see this being super commonly used and breaking changes are likely to be small; it's probably fine to gather feedback and address any issues/refactors at a later point. Either way, it's a bit out of my control so we'll just see what the other core devs opinions are.

[straight-shoota]

I see this more like a PoC for now. That means it should either be merged after 1.0. Or behind a feature flag. But waiting for 1.0 is certainly the easier option.

[Blacksmoke16]

I would be in favor of a feature guard to at least allow some of my work to move forward. However, requiring all users to build their application with that flag is less than ideal. Also it would be even worse if the feature was later reverted. So I suppose the best course of action would be waiting until after 1.0.0, which I hear is nearing.

This feature would be a game changer in what is possible via macros (for the better). I personally have a vested interest in the feature as it's blocking some refactors/features I wanted to implement within Athena. Because of this, I would be happy to sponsor this issue or something if that would help address some of the design concerns and/or help make it more of a priority/first class citizen.

Either way I guess I'll just keep this open and see how things play out.

[asterite]

@Blacksmoke16 Could you provide a list of things you could do with this that you can't do right now?

[Blacksmoke16]

@asterite Sure. I'll use https://github.com/athena-framework/dependency-injection as an example.

The two big points are:

1. Not being able to share macro code at all
   1.1 If you need to do something more than once you have to duplicate it.
2. Recursion
   2.1 Similar to the first point, but if you need to do something, say parsing an array of values, but then say one of the elements is also an array and you want to do the same logic on each item in that array, you have to duplicate the code and nest it.

To be clear, based on #8835 I'm more interested in a way to work directly with the ASTNodes, i.e. accept one or more, apply logic to them, and return one or more. Or in the case of HashLiteral for example, being able to mutate it by reference. Being able to add your own method to existing types like StringLiteral is neat, but something I don't really have a need for.

In the end because of these two points, you end up with code that is quite gross.

With this PR to address those previous points, I'm now able to refactor things to be much cleaner. The logic is broken out into "methods" so it's more readable, and reusable; and quite minimal considering what it's doing.

Another feature which this PR enables is the ability to alter the service container while it's being compiled, similar to Compiler Passes in Symfony. An example of how this could be used is like:

• Manually registering services with the container without needing those classes to be annotated.
• Changing the value of a specific argument of a specific service.
• In the future maybe something related to env specific parameters defiend at compile time.

AND, since I have things broken out into "methods", users who implement these compiler passes are able to reuse the same logic that used in the core shard.

All this macro code would make you think its doing some hacky stuff or something. However, in the end its just defining some getters/ivars within a type. The complexity comes from auto resolving dependencies of each service, such as services depending on other services etc.

# Use compiler pass the change the name
# that the `Foo` service will use
module ChangeNamePass
  include ADI::CompilerPass

  macro post_process(service_hash, alias_hash)
    service_hash["foo"][:arguments][1] = "bob"
  end
end

# An interface that could have multiple implementations
module SomeInterface
end

@[ADI::Register]
class FakeService
  include SomeInterface
end

# Allow aliasing a specific interface to a given service
@[ADI::Register(name: "custom_fake", alias: SomeInterface)]
class CustomFooFakeService
  include SomeInterface
end

# Auto resolves based on an interface but supply explicit arg
@[ADI::Register(_name: "JIM")]
class Bar
  def initialize(@asdf : SomeInterface, @name : String); end
end

# Auto resolves based on type
@[ADI::Register]
class FooBar
  def initialize(@obj : Foo); end
end

# Scalar arguments
@[ADI::Register(1, "fred", false)]
class Foo
  def initialize(@id : Int32, @name : String, @active : Bool); end
end

# Multiple services based on same class
@[ADI::Register("GOOGLE", "Google", name: "google", tags: ["feed_partner", "partner"])]
@[ADI::Register("FACEBOOK", "Facebook", name: "facebook", tags: ["partner"])]
struct FeedPartner
  getter id : String
  getter name : String

  def initialize(@id : String, @name : String); end
end

# Resolve all services with the given tag
@[ADI::Register("!partner")]
class PartnerManager
  getter partners

  def initialize(@partners : Array(FeedPartner))
  end
end

This code would produce:

struct ServiceContainer
  # Define getters for each service, if the service is public, make the getter public and also define a type based getter

  private getter fake_service : FakeService { FakeService.new }

  private getter custom_fake : CustomFooFakeService { CustomFooFakeService.new }

  private getter bar : Bar { Bar.new(custom_fake, "JIM") }

  private getter foo_bar : FooBar { FooBar.new(foo) }

  private getter foo : Foo { Foo.new(1, "bob", false) }

  private getter partner_manager : PartnerManager { PartnerManager.new([google, facebook] of Union(FeedPartner)) }

  private getter google : FeedPartner { FeedPartner.new("GOOGLE", "Google") }

  private getter facebook : FeedPartner { FeedPartner.new("FACEBOOK", "Facebook") }

  # Define getters for aliased service, if the alias is public, make the getter public and also define a type based getter

  private def some_interface : CustomFooFakeService
    custom_fake
  end

  # Initializes the container.  Auto registering annotated services.
  def initialize
    # Work around for https://github.com/crystal-lang/crystal/issues/7975.
    Athena::DependencyInjection::ServiceContainer

    @fake_service = fake_service
    @custom_fake = custom_fake
    @bar = bar
    @foo_bar = foo_bar
    @foo = foo
    @partner_manager = partner_manager
    @google = google
    @facebook = facebook
  end
end

[asterite]

Wait, how does @type.get_service_hash_value even work? It seems like you are adding methods to TypeNode by adding macros to types... or something like that? I didn't expect that.

post_process

Sorry, that's not being used in the code... it's also a lot of code so it's not easy to follow for me.

Not being able to share macro code at all

If what this PR enables is not having to repeat code, BUT you can still do thing by repeating code, then I think we should take our time to design this properly.

[Blacksmoke16]

Wait, how does @type.get_service_hash_value even work?

It's defined at the top. It just accepts some arguments, and returns a NamedTupleLiteral. In this case @type refers to the type the macro was defined in so is just working like an alias in this case. Athena::DependencyInjection::ServiceContainer.get_service_hash_value service_id, service, ann, alias_hash works as well.

Sorry, that's not being used in the code... it's also a lot of code so it's not easy to follow for me.

Yea sorry. It's used towards the bottom. One of the last thing that happens before creating the getters.

See the bottom of my reply for some example code that shows the input/output.

If what this PR enables is not having to repeat code, BUT you can still do thing by repeating code,

The compiler pass concept is impossible currently. I tried implementing the di-refactor branch without this PR, but following the same design. The macro code gets so gross and complex it's essentially impossible as well. I might try it again, but it's for sure not going to be anywhere near as clean as it is now.

then I think we should take our time to design this properly.

Don't get me wrong. I'm not suggesting rushing to merge this if it's something that could be reverted in the future. But I'd also rather not let this go by the wayside and be forgotten about or take years...

[asterite]

I guess I didn't know this was possible:

class Foo
  macro baz
    1
  end

  macro bar
    {{@type.baz}}
  end
end

p Foo.bar

My idea was that you could do Foo.baz inside macros, but now I realize @type.baz works as well. That's a bit confusing because the method name can clash with existing macro methods of TypeNode.

The compiler pass concept is impossible currently

Maybe that's a good thing. I think I introduced the macro finished early in the language and I'm pretty sure right now it wouldn't be accepted at all. It's not well designed. I'm pretty sure it's used everywhere in the wild, but it didn't have a design phase.

I just wouldn't like to introduce more and more features without proper planning.

[Blacksmoke16]

That's a bit confusing because the method name can clash with existing macro methods of TypeNode.

True. I think we have some options tho:

1. Accept this limitation, as someone using this would be more advanced with macros
2. Detect the collision
3. Disallow @type and require the full namespace name.

However, doesn't this PR also allow you to add methods to existing AST types as well? As it would be possible to do like:

class Crystal::Macros::StringLiteral
  macro upcase
    "foo"
  end
end

macro bar
  {{ "x".upcase }}
end

# Oh, why isn't this "foo"
pp bar # => "X"

If we want to be strict, maybe the best solution would be to do 3 and disallow defining methods within existing types, like StringLiteral.

I just wouldn't like to introduce more and more features without proper planning.

I'm totally fine with this. Where do we start? 🙂

[asterite]

Where do we start?

Once 1.0 is released

[HertzDevil]

These should live in their own PR as they would benefit regular macros too, although clearly there is a long way from implementing the full semantics of normal while loops.

[Blacksmoke16]

Closing in favor of a future implementation resulting from #8835 (comment).