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

Can't inherit from generic of nested class #5031

Closed
cfrederrrr opened this issue Sep 24, 2017 · 5 comments
Closed

Can't inherit from generic of nested class #5031

cfrederrrr opened this issue Sep 24, 2017 · 5 comments

Comments

@cfrederrrr
Copy link

cfrederrrr commented Sep 24, 2017

The code

# a book has pages. it is essentially an array of pages
class Book < Array(Book::Page)

  class Page
    @content : String = ""

    def initialize(@content); end
  end

  @bookmark : Int32 = 0

  def initialize(@bookmark); end
end

# a book is different from a newspaper, which also has pages, but in some ways, the same
class NewsPaper < Array(NewsPaper::Page)

  class Page
    class Article
      @content : String = ""
    end

    @articles : Array(Article)

    def initialize(@number, @articles); end
  end

end

The compile:

$ crystal build src/book.cr 
Error in src/book.cr:2: undefined constant Book::Page (did you mean 'Bool')

class Book < Array(Book::Page)
                   ^~~~~~~~~~
$crystal -v
Crystal 0.23.1 [e2a1389e8] (2017-07-13) LLVM 3.8.1

Why this is a problem:

As you can see Book::Page and NewsPaper::Page are wholly different types but they are still "pages," with respect to their outer types, and their names should reflect this, but they belong in different subtypes.

Writing another class just called Page outside of Book and Newspaper, does not make sense, as they have different attributes, and thus different behavior, despite still both being "pages"

Writing it as generic Page(T) (Page(Book) or Page(NewsPaper)) is both misleading and tedious, and still preempts the flexibility offered by namespaces.

It is possible to simply write NewsPaperPage and BookPage to get around this, but in my opinion that makes organizing the project messy. Additionally, their definition is less expressive as you refer them in the same namespace as the class which is supposed to contain them.

This isn't a huge deal as i can easily just make a wrapper class which delegates to the array...

class Book
  class Page
    property content : String = ""
  end

  property pages : Array(Page) = [] of Page
  property bookmark : Int32 = 0

  def turn
    @bookmark += 1
  end

  def read_page
    @pages[@bookmark].content
  end
  # etc.
end

...but this would mean re-writing every method (or at least the ones i foresee using) from Array(T) and using it on @pages which isn't great either. I guess the solution is Book.new.pages[0] or something.

Questions:

  • Do classes truly need to have such concrete definition when being used to specify generics such as Array(T) at the time of declaring inheritance? Of course, they must get defined before the end of input, but is it critical for that definition of T to be fully and completely fleshed out as of this line class Something < Generic(T)?
  • How difficult would it be to instruct the compiler to keep T of Array(T) abstract throughout class definition, and then:
    • if T doesn't get any definition later, raise an error?
    • if T does get defined later, close the loop on inheritance?
      It does seem like it would be more than just an insertion of some conditional reasoning and might require some in depth thought and possibly a large rework of how the compiler assess type values to support this.
  • Should i just devise a workaround?

Comments

I checked #2665 and was surprised I didn't find any issues were like this one. It seems like someone would have encountered this before me, so I apologize if this is a duplicate... but I didn't find any.

@konovod
Copy link
Contributor

konovod commented Sep 24, 2017

Not sure if this is a best solution, but you can use delegate_to macro for the

but this would mean re-writing every method (or at least the ones i foresee using) from Array(T)

https://carc.in/#/r/2sg9
Or even forward_missing_to to delegate all methods:
https://carc.in/#/r/2sga

@ghost
Copy link

ghost commented Sep 24, 2017

is the property pages : Array(Page) = [] of Page actually redundant (Array(Page) + of Page) or does this have a certain benefit?

(just curious because I am new)

@cfrederrrr
Copy link
Author

@monouser7dig i guess i could just say property pages = [] of Page but i was less concerned with that - i didn't test that block, i was just putting up some code to propose an alternative to the issue i'm having.

@konovod that could work. i'm ok with composition over inheritance, but i didn't realize we had that macro. it does more or less solve the problem, but i still feel that class A < Generic(A::B) should work as long as A::B gets some definition.

@asterite asterite changed the title Can't inherit from generic of subclass Can't inherit from generic of nested class Sep 25, 2017
@asterite
Copy link
Member

Answers from my point of view:

  1. Inheriting Array is a code smell
  2. Prefer composition over inhertiance
  3. Lazily resolving the type parameter is both hard and confusing. The language is tending more towards not lazily resolving types (see for example Wrong overload called instead of BigFloat.initialize(BigRational) #4897 (comment) )

So I'd close this issue. The problem can be solved without requiring language changes.

@cfrederrrr
Copy link
Author

cfrederrrr commented Sep 28, 2017

@asterite yea, after submitting the issue and thinking about it more for the past few days i have arrived at more or less the same conclusion as your first and second points, especially after what @konovod said. i did expect your third point would be true... and probably would require cascading language changes.
i almost didn't open the issue because i know accommodating this change would probably make it really hard to release 1.0 on time, but i figured you could always just say no 😄

so i agree and i will close it.

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

No branches or pull requests

3 participants