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

Remove instance variable error for abstract classes #2883

Closed
wants to merge 1 commit into from
Closed

Remove instance variable error for abstract classes #2883

wants to merge 1 commit into from

Conversation

opes
Copy link

@opes opes commented Jun 20, 2016

Remove instance variable error for abstract classes

Fixes #2827

Test Plan:

  • Compile the following code:
abstract class Base
  @a : Int32
end

class Sub < Base
  @b : Int32
  def initialize(@a, @b)
  end
end
  • No error messages should be thrown

…act classes

Fixes #2827

Test Plan:
* Compile the following code:

```crystal
abstract class Base
  @A : Int32
end

class Sub < Base
  @b : Int32
  def initialize(@A, @b)
  end
end
```

* No error messages should be thrown
@oprypin
Copy link
Member

oprypin commented Jun 20, 2016

I'm wondering about some edge cases that shouldn't be allowed but might pass through with this change. Please try out this code (I can't right now) and check that it still gives an error.

class A
  @a : Int32
  def initialize
    @a = 5
  end
end

abstract class B < A
  @b : Int32
  def initialize(@b)
  end
end

class C < B
end

Or maybe this is simpler but still checks the same thing (should give an error)

abstract class A
  @b : Int32
end

class B < A
end

(Feel free to use this code, I release it as public domain)

@asterite
Copy link
Member

Thank you!

Two notes. First, with this change this compiles fine but shouldn't:

abstract class Base
  @a : Int32
end

class Sub < Base
end

p Sub.new

Second, if an abstract class defines non-nilable instance variables, in my opinion it should provide a constructor for those variables and make subclasses call super. That makes it more clear that this variable needs to be initialized. So I'm not sure this should be fixed (I don't consider this a bug, but a feature).

@opes
Copy link
Author

opes commented Jun 21, 2016

Thanks, @asterite and @BlaXpirit! It seems like the intended behavior may still be developing, but if this PR is indeed wanted eventually, I will gladly make the changes needed to make sure the edge cases are covered.

@asterite
Copy link
Member

Closing because the solution is incorrect.

@asterite asterite closed this Sep 29, 2017
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

4 participants