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

Semantic: don't find type parameter in alias #4996

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Semantic: don't find type parameter in alias #4996

merged 1 commit into from
Sep 21, 2017

Conversation

asterite
Copy link
Member

Fixes #3502

Similar to #4974

When we have code like:

class Gen(T)
end

class A(T)
  alias B = Gen(T) # here
end

In the "here" line, T shouldn't be the same as the T from A(T). Types in Crystal simply don't work like that. An alias must be solved from the current type (A(T) in this case) but without considering type parameters. alias is not generic. And nested types don't know about type parameters of parent types.

@asterite
Copy link
Member Author

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

@sdogruyol
Copy link
Member

LGTM 👍 ?

@asterite
Copy link
Member Author

@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.

@sdogruyol
Copy link
Member

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

@mverzilli
Copy link

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.

@sdogruyol
Copy link
Member

Thanks for the great explanation @mverzilli 👍

@asterite asterite merged commit f23a126 into crystal-lang:master Sep 21, 2017
@asterite asterite deleted the bug/3502-type-parameter-in-alias branch September 22, 2017 11:17
@mverzilli mverzilli added this to the Next milestone Sep 22, 2017
@maiha
Copy link
Contributor

maiha commented Oct 28, 2017

@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,

@asterite
Copy link
Member Author

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

@asterite
Copy link
Member Author

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

@asterite
Copy link
Member Author

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

@maiha
Copy link
Contributor

maiha commented Oct 28, 2017

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,

@RX14
Copy link
Contributor

RX14 commented Oct 28, 2017

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

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

Successfully merging this pull request may close these issues.

None yet

5 participants