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

Fix accidental recursion in jl_concrete_subtype #31064

Merged
merged 1 commit into from
Jun 8, 2019
Merged

Fix accidental recursion in jl_concrete_subtype #31064

merged 1 commit into from
Jun 8, 2019

Conversation

Keno
Copy link
Member

@Keno Keno commented Feb 14, 2019

jl_concrete_subtype was recursing into the vararg element of a tuple,
which I don't think is correct (because it could be zero-length and
thus be constructible even if the element type is not). Fix that and
as a bonus get we are now able to get rid of the previous condition
that (accidentally) special cased tuples to have that behavior.

Fixes #31062

@Keno Keno requested a review from JeffBezanson February 14, 2019 00:16
@ararslan ararslan added the bugfix This change fixes an existing bug label Feb 14, 2019
@Keno
Copy link
Member Author

Keno commented Feb 14, 2019

Well, I guess we found out what that condition is for. I suppose it's not possible to create a type that contains itself without either unions (which this returns true for), or having ninitialized < nfields.

@Keno
Copy link
Member Author

Keno commented Feb 14, 2019

Though I guess that only prevents us from instantiating such a type, not from defining it and indeed:

julia> struct Bar
       x::Bar
       end

julia> precompile(identity, (Bar,))
ERROR: StackOverflowError:

I suppose we could cache it during datatype initialization (which essentially gets us the recursion memoization for free by storing the flag in the datatype).

@vtjnash
Copy link
Member

vtjnash commented Feb 14, 2019

Yes, Bar there could make an excellent implementation for a Never type.

I started computing that property in datatype initialization, of whether the type is able to be allocated versus whether the fields could form a cycle, to set isinlinealloc (although as you may guess from the name, it's only currently computed for isbitstypes):

julia> Bar.isinlinealloc
false

@Keno Keno requested a review from vtjnash June 7, 2019 22:15
@vtjnash
Copy link
Member

vtjnash commented Jun 8, 2019

Seems valid, so LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StackOverflowError in jl_has_concrete_subtype
3 participants