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

Constant scope definition #10281

Open
jkthorne opened this issue Jan 22, 2021 · 31 comments
Open

Constant scope definition #10281

jkthorne opened this issue Jan 22, 2021 · 31 comments

Comments

@jkthorne
Copy link
Contributor

I like this pattern in Ruby. Is this something that would be valuable in Crystal?

class Foo
  ALL = [
    BAR = "BAR",
    BAZ = "BAZ",
  ]
end

puts "Foo::ALL #=> #{Foo::ALL}"
puts "Foo::BAR #=> #{Foo::BAR}"
puts "Foo::BAZ #=> #{Foo::BAZ}"

output:

Foo::ALL #=> ["BAR", "BAZ"]
Foo::BAR #=> BAR
Foo::BAZ #=> BAZ

In crystal this does not compile

class Foo
  ALL = [
    BAR = "BAR",
    BAZ = "BAZ",
  ]
end

puts "Foo::ALL #=> #{Foo::ALL}"
puts "Foo::BAR #=> #{Foo::BAR}"
puts "Foo::BAZ #=> #{Foo::BAZ}"
@asterite
Copy link
Member

It works if you declare BAR and BAZ outside of ALL

@jkthorne
Copy link
Contributor Author

it does work if you declare it outside ALL. I consider this an optimization. It is nice when you are maintaining a lot of constants and collection of constants. I would not recommend this in normal setups. But when dealing in dozens or hundreds of constants this is a really nice piece of syntax sugar to have.

@Sija
Copy link
Contributor

Sija commented Jan 23, 2021

@wontruefree It's not nice, it's confusing.

@jkthorne
Copy link
Contributor Author

It might be a style difference. But once you recognize the pattern it is nice to remove duplication and not have to track multiple inserts or ordering in large sets of constants.

I mean it is an edge case but I find it helpful in ruby.

I am porting something from Ruby to Crystal and it is very noticeable to not have it.

@jhass
Copy link
Member

jhass commented Jan 23, 2021

We got enums for this usecase.

@Sija
Copy link
Contributor

Sija commented Jan 23, 2021

@wontruefree All of this looks more like a sloppy coding habit (oh, days of PHP spaghetti code...) than any sensibly useful feature, I vote to close this (non) issue.

@straight-shoota
Copy link
Member

It might be useful in some simple situations, but it's still an edge case. IMO not worth it because it's easy to write differently and chances are that even results in better code.

@jkthorne
Copy link
Contributor Author

@jhass I dont think enums can have strings as values which is super common. Also there are many collections in my industry and I just think enums do not have the right features for this. Arrays are far more preferable.

I realize some people are reacting to how this looks. I do think from engineers onboarding and moving to other teams. When dealing with the problem set we have people generally do not like the look at first but see the value after managing all these values. I guess I just say be a little open minded that people have reached for this feature in ruby because it provides value. but if someone has other ways to manage large sets of constants that have to be in many collections let me know.

@straight-shoota
Copy link
Member

Can you show some real world examples?

@jkthorne
Copy link
Contributor Author

Since a fair amount of my work is on private codebases I cannot share a great example but here is a smaller example on a public codebase. This is in a transitional state due to some refactoring but show the concept.

https://github.com/wontruefree/braintree/blob/master/src/models/sandbox/transaction/card.cr

@straight-shoota
Copy link
Member

straight-shoota commented Jan 23, 2021

Thanks. I can't figure how that would look if you could declare constants within other constant's initializers because most (?) are used in different collections. I suppose the declaration would be on ALL and the other collections just reference the constants?

It certainly works without that features, but I understand a benefit would be that you can't "forget" to add a value to the ALL collection, when all are declared there. That's definitely a plus.

However, I think that this ALL collection itself looks like a workaround and we don't need to enable a workaround just to have a workaround. I'd rather try to solve the underlying problem directly.

As @jhass already mentioned, this looks like a use case for enums. But they can't have string values in Crystal. In Java for example, this would work very well.

The closest you can get to a string based enum in Crystal would be an enum with custom mapping to string values.

enum Foo
  BAR
  BAZ

  def string_value
    case self
    in BAR then "12345"
    in BAZ then "54321"
    end
  end
end

That's not very nice to write, though. It could probably be improved using a macro. Or maybe even compiler support could be added. This would mean to re-open #2978 and might even be worth a look. Enums could be allowed to have non-integer values associated to them. That wouldn't need to change the representation of an enum, which would still be numeric.
But the compiler could transform a concise definition like the following to something like the above example, where it maps internal int value to an external value which can be any type:

enum Foo : String
  BAR = "12345"
  BAZ = "54321"
end

This could also improve use cases like this one that came up on the forums today: https://forum.crystal-lang.org/t/enum-to-json/2896


An alternative to string-based enums for the discussed use case would be a value type which keeps track of values in the constructor like this:

struct Foo
  BAR = new "12345"
  BAZ = new "54321"
  ALL = [] of self

  def initialize(@value : String)
    ALL << self
  end
end

@jkthorne
Copy link
Contributor Author

I like these ideas and it moves closer to some benefits we see in ruby. I will say even in this example there are overlapping groups that would I think would be hard with enums. for example the ALL, VALID and VISA. but any movement to enable more of these features would be good. Constant management and usability would be a good feature to help sell crystal to my team.

@straight-shoota
Copy link
Member

VISA could really be as simple as Card.values.select(&.to_s.starts_with?("VISA")).

@HertzDevil
Copy link
Contributor

HertzDevil commented Jan 23, 2021

Actually, wouldn't this particular case be eventually supported by #10009? Can't say the same about hash literals though.

EDIT: The expansion as described there doesn't work out of the box:

class Foo
  ALL = ::Array(typeof(BAR = "BAR", BAZ = "BAZ")).unsafe_build(2)
  ALL.to_unsafe[0] = BAR = "BAR"
  ALL.to_unsafe[1] = BAZ = "BAZ"
  ALL
end

First there are still dynamic constants here, and second typeof(BAR = "BAR") triggers an internal compiler error. But the following slightly modified form works:

class Foo
  ALL = ::Array(typeof("BAR", "BAZ")).unsafe_build(2)
  BAR = ALL.to_unsafe[0] = "BAR"
  BAZ = ALL.to_unsafe[1] = "BAZ"
  ALL
end

Foo::ALL # => ["BAR", "BAZ"]
Foo::BAR # => "BAR"
Foo::BAZ # => "BAZ"

And it's relatively easy to destructure the Assign nodes inside the literal expander.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 23, 2021

@HertzDevil Nobody said this would be hard to implement (at least for limited use cases like array literals) ;)

@asterite
Copy link
Member

asterite commented Jan 23, 2021

I don't think we need to change the language for this.

I guess it's boring to translate that structure into Crystal because it's a lot of manual work. This is still manual work but it's much easier to help translating the Ruby code:

macro im_translating_ruby_code(assign)
  {% for const in assign.value %}
    {{ const }}
  {% end %}
  {{ assign.target }} = [
    {% for const in assign.value %}
      {{ const.target }},
    {% end %}
  ]
end

class Foo
  im_translating_ruby_code(
    ALL = [
      BAR = "BAR",
      BAZ = "BAZ",
    ]
  )
end

puts "Foo::ALL #=> #{Foo::ALL}"
puts "Foo::BAR #=> #{Foo::BAR}"
puts "Foo::BAZ #=> #{Foo::BAZ}"

@straight-shoota
Copy link
Member

straight-shoota commented Jan 23, 2021

@asterite That certainly helps for translating, but it's not really an answer. It means you have to keep the Ruby style and translation macro indefinitely. To me that equals to adding yet another workaround to keep an workaround working. Instead of providing a proper solution to the unterlying problem.
What is the plan when you're not translating from Ruby and still want to model something like this in pure Crystal?

@Sija
Copy link
Contributor

Sija commented Jan 23, 2021

[...] Instead of providing a proper solution to the unterlying problem.

@straight-shoota Why do you think that this problem needs to be solved at the language level? Is it a problem at all?

@asterite
Copy link
Member

The plan is to code in Crystal if you are coding in Crystal, not coding in Ruby.

I mean, you can't do that same thing in Go, Java or Python, but nobody complains about that.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 24, 2021

Well, I'm not talking about nested constant assignments. What I described in #10281 (comment) is entirely possible in Java because enum types can have custom complex values.

enum Foo {
  BAR("1234"),
  BAZ("5678"),
  QUX("9012");

  static final Foo[] SOME = {BAR, BAZ};

  private String value;

  Foo(String value) {
    this.value = value;
  }

  public String getValue() {
    return value;
  }
}

This is better than the workaround used in Ruby: It add's type safety. The enum type is essentially a value type and values can't be confused with arbitrary strings. There's no need for tracking all values manually with ALL: you can get all enum members from Foo.values(). So while there'd be a little bit overhead for defining the enum type, it ultimately results in more consice and better code.

I think this could be a great feature to explore for Crystal.

@asterite
Copy link
Member

But the Ruby code can be translated to Crystal by just moving the constant declaration outside of the array.

Any other idea is unrelated to this issue/request. I think it would be nice to have enums with string values, but I'm not sure how to implement that.

@straight-shoota
Copy link
Member

It is related in the way that it offers a different, better solution to model such data than in the Ruby code. Hence it would not be useful at all to have nested constant assignment in Crystal when you can use a superior alternative.

@asterite
Copy link
Member

This is actually possible in Crystal. Define an enum and provide a method that translates it to the string representation. Enums with string values are not needed after all.

@straight-shoota
Copy link
Member

Yes, it's possible. But writing such code is not very nice. There's a lot of duplication and member declarations are separated from associated values. I doubt even a sophisticated macro can make this really nice to use.

@asterite
Copy link
Member

Here you go:

macro string_enum(name, &block)
  enum {{name}}
    {% for exp in block.body.expressions %}
      {{exp.target}}
    {% end %}

    def to_s
      case self
        {% for exp in block.body.expressions %}
        in {{exp.target}}
          {{exp.value}}
        {% end %}
      end
    end
  end
end

string_enum Numbers do
  One   = "1"
  Two   = "2"
  Three = "3"
end

puts Numbers::One
puts Numbers::Two
puts Numbers::Three

@straight-shoota
Copy link
Member

Sure, but it gets more complicated when you want to define methods.

@asterite
Copy link
Member

Not really:

macro string_enum(name, &block)
  enum {{name}}
    {% for exp in block.body.expressions %}
      {% if exp.is_a?(Assign) %}
        {{exp.target}}
      {% else %}
        {{exp}}
      {% end %}
    {% end %}

    def to_s
      case self
        {% for exp in block.body.expressions %}
          {% if exp.is_a?(Assign) %}
            in {{exp.target}}
              {{exp.value}}
          {% end %}
        {% end %}
      end
    end
  end
end

string_enum Numbers do
  One   = "1"
  Two   = "2"
  Three = "3"

  def hello
    "Hello!"
  end
end

puts Numbers::One
puts Numbers::Two
puts Numbers::Three

puts Numbers::One.hello

But that's not the point. The original complaint was that this doesn't compile in Crystal:

class Foo
  ALL = [
    BAR = "BAR",
    BAZ = "BAZ",
  ]
end

puts "Foo::ALL #=> #{Foo::ALL}"
puts "Foo::BAR #=> #{Foo::BAR}"
puts "Foo::BAZ #=> #{Foo::BAZ}"

But that's not quite true. Yet, it doesn't compile exactly like that. Also empty array literals don't compile in Crystal. That doesn't mean we need to change the language to match Ruby. And the "workaround" here is to define the values outside of the array definition. If you don't want to do all that manual work you can use a macro to make that easier. If you don't like that approach and you'd like to use enum values that map to strings, you can do it! Yes, we could keep adding and changing things to the language, but if we are going to do that for every tiny request then the language will become a mess.

In my opinion this issue is solved. And it should have been a question in the forums.

@asterite asterite reopened this Jan 24, 2021
@jkthorne
Copy link
Contributor Author

I would love to have string enums and the ability to have constants in multiple collections. In general it is something we manage a lot at work. It would be nice to have solutions for this.

@asterite
Copy link
Member

I already posted a solution for String enums.

What does it mean to have constants in multiple collections?

@jkthorne
Copy link
Contributor Author

I would like the same constant which is in an enum in your example and have it in ALL, VALID and VISA. I think string enums are a good move forward for this. since credit card numbers are actual number I kind of want to try this enums with the current code. I just wanted to point out it would be nice to make this kind of stuff easier.

@straight-shoota
Copy link
Member

Enums can't have constants other then their members. So you would have to use class variables instead. But that should really work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants