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

Check Type has parameters field with an element #493

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

Zentrik
Copy link
Collaborator

@Zentrik Zentrik commented Sep 1, 2023

Fixes #491, #492, #494

@Zentrik Zentrik closed this Sep 3, 2023
Fixes type Union has no field parameters JuliaDebug#494
@Zentrik Zentrik reopened this Sep 3, 2023
@Zentrik
Copy link
Collaborator Author

Zentrik commented Sep 3, 2023

Unfortunately it seems that extract_inner_type(::Type{Type{T}}) specializes on each new type it sees, I don't know how problematic that is. Also not sure why we were taking the first element of T.parameters before but this implementation seems to work fine.

@codecov-commenter
Copy link

Codecov Report

Merging #493 (30ec350) into master (a017a4d) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #493   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           9       9           
  Lines        1511    1511           
======================================
  Misses       1511    1511           

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I doubt it will be measurable (and so we can merge this), but presumably extract_inner_type will be specialized for every type. The old code tried to avoid that but it may not be readily fixable. So I'm fine with merging this, thanks for tackling it!

EDIT: Oh, just saw you commented on the same issue.

function type_annotation_mode(node, @nospecialize(T); type_annotations::Bool, hide_type_stable::Bool)
kind(node) == K"return" && return false, "", "", ""
type_annotate = is_show_annotation(T; type_annotations, hide_type_stable)
pre = pre2 = post = ""
if type_annotate
if T isa Type && T <: Type
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess this should have been T <: Type{S} where S or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Union{} <: Type{S} where S is true so I don't think that would have fixed it either.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be T isa DataType or Base.isType(T)

@timholy
Copy link
Member

timholy commented Sep 3, 2023

Also not sure why we were taking the first element of T.parameters before but this implementation seems to work fine.

julia> TT = Type{Float64}
Type{Float64}

julia> TT.parameters[1]
Float64

Basically we were extracting the T in the same way as your extract_inner_type, without forcing dispatch specialization. I think the problem with the old implementation is that T <: Type was too weak of a condition. Perhaps T <: Type{S} where S might fix it?

@Zentrik
Copy link
Collaborator Author

Zentrik commented Sep 3, 2023

I thought extract_inner_type(@nospecialize(x::Type{<:Type})) = x.parameters[1] might work, but unfortunately it fails with Core.TypeofBottom. Given I don't know when a type actually has a non empty parameters svec might be easiest to just use this implementation.

@simeonschaub
Copy link
Collaborator

I believe it should be enough to just change the check to T isa DataType && T <: Type

@Zentrik
Copy link
Collaborator Author

Zentrik commented Sep 3, 2023

Core.TypeofBottom satisfies that but has an empty parameters svec, maybe checking if it's non empty as well will work.

@Zentrik
Copy link
Collaborator Author

Zentrik commented Sep 5, 2023

Given we don't think the current implementation will be particularly slow I'll merge it for now, and we can revisit this if it is slow.

@Zentrik Zentrik merged commit 8350220 into JuliaDebug:master Sep 5, 2023
22 of 28 checks passed
Zentrik added a commit to Zentrik/Cthulhu.jl that referenced this pull request Sep 25, 2023
Fixes JuliaDebug#506 which was due to calling `extract_inner_type` with `Type{AbstractArray{var"#s91"<:Real, 1}}`. Uses suggestions from JuliaDebug#493, this also removes the specialisation compared to the current implementation.
vchuravy pushed a commit that referenced this pull request Oct 12, 2023
Fixes #506 which was due to calling `extract_inner_type` with `Type{AbstractArray{var"#s91"<:Real, 1}}`. Uses suggestions from #493, this also removes the specialisation compared to the current implementation.
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

Successfully merging this pull request may close these issues.

Some Types don't have field parameters error when checking if annotation is of form "String::Type{String}"
6 participants