Improve inference for ismutable with missing sparam #46693
Conversation
Keno
force-pushed
the
kf/ismutablerefvalue
branch
from
c889a0c to
e52e3f9
Compare
We could already infer `ismutable(RefValue{T})` if we knew what
`T` was at inference time. However, the mutable does of course
not change depending on what `T` is, so fix that up by adding
an appropriate special case in `_getfield_tfunc`.
Keno
force-pushed
the
kf/ismutablerefvalue
branch
from
e52e3f9 to
9352745
Compare
| @@ -823,7 +824,11 @@ function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck:: | |||
| if isa(s, Union) | |||
| return getfield_nothrow(rewrap_unionall(s.a, s00), name, boundscheck) && | |||
| getfield_nothrow(rewrap_unionall(s.b, s00), name, boundscheck) | |||
| elseif isa(s, DataType) | |||
| elseif isType(s) | |||
There was a problem hiding this comment.
(otherwise, it might be say s = UnionAll (isa DataType) here, but at runtime turn into s = Union or s = DataType)
There was a problem hiding this comment.
Yes, since we put in the extra checks below, that probably needs to be reflected here.
| if isa(sv, DataType) && isa(name, Const) && (!isType(sv) && sv !== Core.TypeofBottom) | ||
| nv = _getfield_fieldindex(DataType, name) | ||
| if nv == DATATYPE_NAME_FIELDINDEX | ||
| # N.B. This doesn't work in general, because |
There was a problem hiding this comment.
Ah, whoops not sure how that line got dropped. Should have said because the other fields are not guaranteed to not depend on type parameters.
| end | ||
| s = typeof(sv) | ||
| else | ||
| sv = s.parameters[1] |
There was a problem hiding this comment.
No, this is intended to catch the cases where we are guaranteed to find a DataType at runtime. @JeffBezanson and I thought the below was sufficient to catch that, but looking at it with fresh eyes, we probably also need to exclude Tuples, since they distribute over Union (so could be a Union at runtime). Are there any other cases that you can think of that we're missing?
There was a problem hiding this comment.
Okay, I think that is right, since this should be a concrete DataType at runtime. Or one of the uncommon over-wrapped types at runtime (such as found in the .sig field of a method with unbound vars), but I think we currently assume it must be the former.
| s = typeof(sv) | ||
| else | ||
| sv = s.parameters[1] | ||
| if isa(sv, DataType) && isa(name, Const) && (!isType(sv) && sv !== Core.TypeofBottom) |
There was a problem hiding this comment.
Since you already tested above with isconstType to discover that the typeof identity of sv is unknown, this seems incorrect now to assume that the typeof identity is DataType, when the previous conditional branch just returned that it might be instead a Union or UnionAll at runtime
|
Can we try to avoid self-merging PRs for new features within a couple hours of opening them, particularly late on a Friday night? It makes review discussions rather awkward to track, since they get split between the old PR / new issue / and new PR. This also should probably have had a nanosoldier run, so we can check if we expect anything to be affected by it. It is unlikely, but the performance test suite is usually pretty quick to run. |
The check in #46693 was attempting to guarantee that the runtime value was a `DataType`, but was missing at least the case where a tuple of unions could have been re-distributed. Fix that up and refactor while we're at it.
* Follow-up #46693 - Also exclude `Type{Tuple{Union{...}, ...}}` The check in #46693 was attempting to guarantee that the runtime value was a `DataType`, but was missing at least the case where a tuple of unions could have been re-distributed. Fix that up and refactor while we're at it. * Apply suggestions from code review Co-authored-by: Jameson Nash <[email protected]> Co-authored-by: Jameson Nash <[email protected]>
I agree. Also confirmed this PR doesn't seem to have introduced regressions: d4f6fd2#commitcomment-83798816 |
Similar to #46693, but for :new, rather than getfield. Unfortunately, this is somewhat limited as we don't really have the ability to encode type equality constraints in the lattice. In particular, it would be nice to analyze: ``` struct Foo{T} x::T Foo(x::T) where {T} = new{T}(x) end ``` and be able to prove this nothrow. If it's really important, we could probably pattern match it, but for the moment, this is not easy to do. Nevertheless, we can do something about the similar, but simpler pattern ``` struct Foo{T} x Foo(x::T) where {T} = new{T}(x) end ``` which is what this PR does.
Similar to #46693, but for :new, rather than getfield. Unfortunately, this is somewhat limited as we don't really have the ability to encode type equality constraints in the lattice. In particular, it would be nice to analyze: ``` struct Foo{T} x::T Foo(x::T) where {T} = new{T}(x) end ``` and be able to prove this nothrow. If it's really important, we could probably pattern match it, but for the moment, this is not easy to do. Nevertheless, we can do something about the similar, but simpler pattern ``` struct Foo{T} x Foo(x::T) where {T} = new{T}(x) end ``` which is what this PR does.
We could already infer
ismutable(RefValue{T})if we knew whatTwas at inference time. However, the mutable does of course not change depending on whatTis, so fix that up by adding an appropriate special case in_getfield_tfunc.