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

Jump to bottom

Detect recursive generic structs #5099

Merged
merged 2 commits into from Oct 11, 2017
Merged

Detect recursive generic structs #5099

merged 2 commits into from Oct 11, 2017

Conversation

iSarCasm
Copy link
Contributor

@iSarCasm iSarCasm commented Oct 10, 2017 •
edited

Hi, this PR is referencing the issue #4720

You can see that compiler currently doesn't detect recursive structs when generics are involved.

  1. https://carc.in/#/r/2vu8
module Bar
end

struct Foo(T)
  include Bar
  def initialize(@base : Bar?)
  end
end

x = Foo(Int32).new(nil)
z = Foo(Int32).new(x)
  1. https://carc.in/#/r/2vu9
module Bar(T)
end

struct Foo
  include Bar(Int32)
  def initialize(@base : Bar(Int32)?)
  end
end

x = Foo.new(nil)
z = Foo.new(x)

Note: my first PR

bcardiff
bcardiff reviewed Oct 10, 2017
View reviewed changes
src/compiler/crystal/semantic/recursive_struct_checker.cr
@@ -140,6 +140,6 @@ class Crystal::RecursiveStructChecker
end

def struct?(type)
type.struct? && type.is_a?(InstanceVarContainer) && !type.is_a?(PrimitiveType) && !type.is_a?(ProcInstanceType) && !type.is_a?(GenericClassType) && !type.abstract?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asterite the GenericClassType are used to represent generic structs also? Because I don't find right away why there was a !type.is_a?(GenericClassType) otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the GenericClassType are used to represent generic structs also?

Yes

Because I don't find right away why there was a !type.is_a?(GenericClassType) otherwise.

I can't remember why that condition exists either. But if tests pass then I guess it's OK.

@bcardiff
Copy link
Member

@iSarCasm congrats for the first PR. It's amazing that you dig right into the compiler.

@RX14 RX14 added this to the Next milestone Oct 11, 2017
@RX14 RX14 merged commit 4d872f9 into crystal-lang:master Oct 11, 2017
@sdogruyol
Copy link
Member

Wow, such an interesting first PR, congrats @iSarCasm 🎉

@sdogruyol sdogruyol mentioned this pull request Oct 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asterite asterite asterite left review comments

@bcardiff bcardiff bcardiff approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:bug topic:compiler
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@iSarCasm @bcardiff @sdogruyol @asterite @RX14