Routes with an optional path param causes compilation error

[jwoertink]

From Discord:

This code

class Hierarchy::Companies < UserClub::BrowserAction
  get "/:language/companies/?:letter" do
    letter = params.get?(:letter)

    t = Tabulator.new(
     letter: letter,
     query: CompanyQuery.new.belongs_to(ctx.site).preload_url_path,
     field: :name,
     path: "/#{language}/companies/"
    )

    html Page::Hierarchy::Companies, html_tabs: t.html_tabs, selected: t.selected
  end
end

throws this error:

GET /en-US/products/
 ▸ Handled by Hierarchy::Products
 ▸  Exception 

     can't execute `__temp_971 = if false
       __temp_970
     else
     begin
         Tabulator
         params.get?(:letter)
         ProductQuery.new.belongs_to(ctx.site)
       endend` at expanded macro: setup_call_method:7:7

    Backtrace 

     src/actions/hierarchy/products.cr:2:3 in 'call'
     lib/lucky/src/lucky/renderable.cr:139:16 in 'perform_action'
     lib/lucky/src/lucky/route_handler.cr:10:7 in 'call'
     /usr/share/crystal/src/http/server/handler.cr:28:7 in 'call_next'
     lib/lucky/src/lucky/remote_ip_handler.cr:15:5 in 'call'
     /usr/share/crystal/src/http/server/handler.cr:28:7 in 'call_next'
     lib/lucky/src/lucky/error_handler.cr:14:5 in 'call'
     /usr/share/crystal/src/http/server/handler.cr:28:7 in 'call_next'
     lib/lucky/src/lucky/log_handler.cr:28:5 in 'call'
     /usr/share/crystal/src/http/server/handler.cr:28:7 in 'call_next'
     lib/lucky/src/lucky/http_method_override_handler.cr:11:5 in 'call'
     /usr/share/crystal/src/http/server/handler.cr:28:7 in 'call_next'
     lib/lucky/src/lucky/force_ssl_handler.cr:37:7 in 'call'
     /usr/share/crystal/src/http/server/request_processor.cr:51:11 in 'process'
     /usr/share/crystal/src/http/server.cr:515:5 in 'handle_client'
     /usr/share/crystal/src/http/server.cr:468:13 in '->'
     /usr/share/crystal/src/primitives.cr:266:3 in 'run'
     /usr/share/crystal/src/fiber.cr:92:34 in '->'
     ???

Removing the optional flag ? from the param fixes it... Could it be this line specifically? letter = params.get?(:letter)??

[matthewmcgarvey]

@jwoertink The action is unrelated to the error. Notice the Handled by Hierarchy::Products isn't the same action as the one pasted in. Looks like it's just a compilation error in the listed action

[jwoertink]

oh, good catch on that.

@bruceperens do you have the error from this action that uses the optional path param?

[BrucePerens]

It's the same code as the one pasted. Only the query is changed. All three (each has a different query) failed at runtime. Also, this is not a compilation error, it happens at runtime. I haven't figured out what throws a plain Exception with "can't execute" as its message. But the backtick might be a clue.

[jwoertink]

Ok, thanks. I'll try to dig in more when I get a moment.

[BrucePerens]

It's a runtime complaint from the compiler about an untyped expression in a macro, with the unfortunately still-too-frequent singularly unhelpful message which doesn't say untyped expression in a macro: ./src/compiler/crystal/semantic/cleanup_transformer.cr: str << "can't execute" << node << "at " << node.location

Also, the compiler is just printing the intermediately generated AST node rather than the source filename or line number. So, it doesn't really help you find where your macro expression generates something untyped.

Sorry, the previous location I posted was wrong.

[BrucePerens]

It's the macro code at https://github.com/luckyframework/lucky/blob/b6f68bdc78b0b6831e801cac3ba0c97f83159e81/src/lucky/routable.cr#L96 What it's doing is a bit over my head.

[jwoertink]

That bit of code just ensures that each before / after pipe either returns continue or some response like redirect, etc... Do you have any before/after pipes that are running around these actions?

[BrucePerens]

At least two things that I think are @akadusei code from Shield: check_authorization require_logged_in

[BrucePerens]

Pardon if this sounds too much like a gripe. But does this really need to be in a macro? My sense is that Lucky is a bit macro-happy, and the error messages suffer because of that and programmer acceptance is hurt by the incomprehensibility of Crystal error messages for macro code. Not that you have time for a big rewrite.

[jwoertink]

does this really need to be in a macro?

Yup. Keep in mind that Lucky's first priority goal is compile-time catching of bugs for development. The only way we can achieve this is by the heavy use of macros. Granted, the error messages aren't as nice as we wish they were, but several issues have been opened in Crystal to help address that.

I know it can be pretty frustrating sometimes, but these sorts of issues help us to improve so after we hit 1.0, newcomers don't go through these nearly as often. As always, I appreciate the feedback!

[jwoertink]

I haven't been able to recreate the original issue. I've added a spec to Lucky to ensure that at least the next release ensures it works. I'm gonna close this out, but if anyone finds an easy way to reproduce this, we can open back up.