The snippet in #3502 was solved by #4974, my comments below are the ones that still break the compiler and which this PR fixes.

LGTM 👍 ?

@sdogruyol Even though the wiki says one other core team member must approve, I'd like to always get approval from two other core team members.

That makes sense, we should update wiki to "two core members other than the PR owner" then 😄

It also states: "In some cases they may not feel sure to merge even when it looks good to them: in those cases it is totally ok to leave the PR approved and ask for another member to look at it for further assurance.", in this case Ary wants two reviewers and that is totally fine.

There's a tradeoff in this rule between core team consensus and agility: if we require 2 approvers for a PR submitted by a core team member, that means involving 3 members to get one thing done, which might be too much for small things.

Also, there are many parts of Crystal that are really well understood only by 1 or 2 members, so it'd become a bit cumbersome to gather all the approvals. See for example here: crystal-lang/crystal-random#2

Julien sent a PR and konovod approved it. I'm not sure how many other members would be capable of "really" reviewing that PR. I read the code and thought it was fine, but I'd probably wouldn't have merged it without konovod's approval.

Thanks for the great explanation @mverzilli 👍

@asterite

In the "here" line, T shouldn't be the same as the T from A(T)

I think it should be same because it is T.
When we want different type, we should use other parameter like this.

class A(T)
  alias B = Gen(U)
end

Otherwise, in short 0.24.0, it breaks basic generic use case like this.

class SomeElementFolder(T)
  alias List = Array(T)
end

# 0.23.x or less : works
# 0.24.0 or this PR: ` undefined constant T`

I think this problem should be solved before releasing 0.24.0, or many users may be confused.
Best regards,

Sorry, no. Types are nested without type parameters. SomeElementList::List is a valid type on its own, and there's no T there.

And nothing really breaks, just write List(T) instead of List and problem solved.

Actually, maybe this can be "fixed". I don't know how, though.

In this case, do you mean that it should be written like this?

class SomeElementFolder(T)
  alias List = Array
  property list : List(T) # not `List`

Well, I agree that it works with the case of name aliasing.
If so, what would you do to use aliasing for representing complex structures?

class CandidatesProducer(T)
  alias CandidateRules = Hash(String, Hash(T, Array(T)))

  def initialize(@resolver : CandidateRules)
  end
end

Thank you,

The solution is generic aliases:

class CandidatesProducer(T)
  alias CandidateRules(T) = Hash(String, Hash(T, Array(T)))

  def initialize(@resolver : CandidateRules(T))
  end
end

or much less confusingly

class CandidatesProducer(T)
  alias CandidateRules(U) = Hash(String, Hash(U, Array(U)))

  def initialize(@resolver : CandidateRules(T))
  end
end