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

Undefined constant with record and improper properties #13343

Closed
jwoertink opened this issue Apr 18, 2023 · 3 comments · Fixed by #13367
Closed

Undefined constant with record and improper properties #13343

jwoertink opened this issue Apr 18, 2023 · 3 comments · Fixed by #13367
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:macros

Comments

@jwoertink
Copy link
Contributor

This is more of a bad error message than a bug.

record Item, amount: Int32
Item.new(1)

In this code, the record is setup wrong by missing a space after the amount, but the error message is:

In foo.cr:1:8

 1 | record Item, amount: Int32
            ^---
Error: undefined constant Item

Where the correct code would be

record Item, amount : Int32
Item.new(1)
@jwoertink jwoertink added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Apr 18, 2023
@a-alhusaini
Copy link
Contributor

I'm looking into this issue. Hopefully I'll have a PR for this in a few hours or tomorrow. Depending on how many obstacles I run into (My crystal is rusty)

@a-alhusaini
Copy link
Contributor

Update: After looking into this more closely and discussing it on the community discord it seems that the error is caused by Crystal thinking that this is a named argument.

I initially expected that the error could somehow be detected and handled within the record macro itself. That would have been easy to fix.

But with this new information we now know that we'll need to get a hold of the compiler code that parses named arguments and add a conditional block to it that verifies that a named argument is actually a named argument. And if it isn't then it should print a more helpful error.

I've digged around in the compiler code but I haven't been able to find the chunk of code that needs to be changed. This should be relatively easy to fix if you know where to look. I don't.

I'll loook into this further tomorrow and see if I can locate the code that needs changing.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 20, 2023

Perhaps a trivial solution would be to just add a **kwargs parameter to the record macro to capture such a call signature and raise explicitly in the macro.

Basic implementation:

macro record(name, *properties, **kwargs)
  {% raise "unknown argument to macro `record`: #{kwargs}" unless kwargs.empty? %}

  # here follows the existing macro body
end

record Bar, foo: String # Error: unknown argument to macro `record`: {foo: String}

The error message can be improved of course. And maybe even suggest the correct syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:macros
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants