Crystal: JSON.mapping cannot have `properties` key

[jgaskins]

Given the following code:

require "json"

class Foo
  JSON.mapping(
    properties: Hash(String, String),
  )
end

p Foo.from_json({
  properties: { "foo" => "bar" },
}.to_json)

This doesn't work, but if you change properties to something else, it does. properties doesn't appear to be a language keyword, so I don't think it's the same problem as #284, but I figure it might be caused by properties being used in the JSON.mapping macro.

I'm currently making it work with props: { type: JSON::Any, key: "properties" }, but if properties isn't already a method on a JSON-mapped class, it makes sense to be able to use it.

[straight-shoota]

Yes, in this case, the call to JSON.mapping is interpreted as assigning the named argument properties.
You can solve this easily by wrapping the named tuple in curly braces:

  JSON.mapping({
    properties: Hash(String, String),
  })

[bew]

You can use it like this:

require "json"

class Foo
  JSON.mapping({
    properties: Hash(String, String),
  })
end

p Foo.from_json({ properties: { "foo" => "bar" }, }.to_json)

https://carc.in/#/r/27ny

Note that I'm passing a NamedTuple to JSON.mapping, not directly the fields as args

[straight-shoota]

Considering that the notation without braces is encouraged by the example in the API docs and properties is a very likely name used in a JSON mapping, it would be great to rename the argument to something less likely to conflict. It should usually not be required to explicitly address this as a named parameter.

[Sija]

Isn't this solved by #236?
Could you see does it still happens on the master branch?

[straight-shoota]

No, this is not related.

The signature of JSON.mapping is
``crystal macro def mapping(properties, strict = false) ```` When you call it like in the above example, the compiler assumes,properties:` refers to the named argument of the macro call while instead is it intended to be a field in the named tuple that is the value of that argument.

[drosehn]

So this issue could be avoided for the specific value of properties by changing the macro declaration to be:

macro def mapping(mapping_properties, strict = false)

I realize that doesn't really solve anything, it just changes where the collision occurs.

[straight-shoota]

... and thus makes a collision less likely.

[asterite]

Whatever name we choose, there will always be the possibility of a collision. There's also the strict parameter. If named arguments give you a conflict, pass a hash literal as mentioned. I don't think this is easily solvable.

We can maybe remove the named arguments overload.

[straight-shoota]

strict is not really an issue. A TupleLiteral with strict: will be matched as value for the required argument properties: https://carc.in/#/r/27v8

We can maybe remove the named arguments overload.

Is it currently possible to remove an argument from name-matching?
If this can be done, it would be great. Otherwise, changing the (external) name to something with lower risk of name collision would be a reasonable improvement.

[bcardiff]

@straight-shoota I think @asterite mean that as a change in the compiler.

Syntax-wise I think we could allow _ as an external name that will disallow to make the call with named argument for that, hence making a positional arg only.

But is definitely simpler for now to change that name for now. Adding that kind of syntax seems nice and neat, but there are thing to check and disallow when mixing many _ and named arguments and mandatory named arguments.

[asterite]

Oh, yes, I think we can just rename properties to _properties or even _properties_ to make the collision less probable.

[bew]

As #5180 has been merged, can we close this issue?