-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
tfuncs: Be more robust in the face of uninhabited types #37945
Conversation
base/compiler/tfuncs.jl
Outdated
@@ -853,6 +853,14 @@ function getfield_tfunc(@nospecialize(s00), @nospecialize(name)) | |||
if isa(name, Conditional) | |||
return Bottom # can't index fields with Bool | |||
end | |||
for _ft in ftypes | |||
if (!isa(_ft, Type) && !isa(_ft, TypeVar)) || _ft === Bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a type like this:
struct Foo
x
y::Union{}
Foo(x) = new(x)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or similarly this, but this case should possibly be disallowed on construction:
struct Bar{T}
x
y::T
Bar{T}(x) where {T} = new(x)
end
Bar{false}(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that legal? I guess there's nothing technically wrong with it, but boy. I guess I'll add a check to only bail if it's one of the initialized fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first one is definitely legal. The second one is currently allowed but probably shouldn't be, since a constructable type needs a layout, and if a field type is a non-type it's impossible to say what its layout is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also s.has_concrete_subtype
which has already checked for conditions such as that?
It's not impossible to describe the layout, but impossible for that field to have a value. We could also treat it similar to apply_type failures, which returns the uninhabited type for the fieldtype and expand the check there (in inst_ftypes
) instead of here:
julia> struct Bar{T}
x
y::Complex{T}
Bar{T}(x) where {T} = new(x)
end
julia> fieldtypes(Bar{false})
(Any, Union{})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure I understand what you're saying. Are you saying we should do this check when we construct the datatype and set any bad type to Union{}
, in which case the existing tfunc would go through fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's no particular necessity in saying the field has type "false", when it's not even valid as a type
Updated as discussed. |
Fixes #37943