inference: improve `TypeVar`/`Vararg` handling #42583

aviatesk · 2021-10-10T18:40:25Z

During working on the incoming lattice overhaul, I found it's quite
confusing that TypeVar and Vararg can appear in the same context
as valid Type objects as well as extended lattice elements.
Since it usually needs special cases to operate on TypeVar and Vararg
(e.g. they can not be used in subtyping), I believe it would be helpful
to avoid bugs and catch logic errors in the future development if we
separate contexts where they can appear from ones where Type
objects and extended lattice elements are expected.

So this PR:

tries to separate their context, e.g. now TypeVar and Vararg should
not be used in _limit_type_size, which is supposed to return Type,
but they should be handled by its helper function __limit_type_size
makes sure tfuncs don't return TypeVars and TypeVar never spills
into the abstract state
makes sure widenconst are not called on TypeVar and Vararg,
and now widenconst is ensured to return Type always
and other general refactors

With some early-take of #42583.

base/compiler/tfuncs.jl

base/compiler/typeinfer.jl

base/compiler/typeutils.jl

base/essentials.jl

With some early-take of #42583.

aviatesk · 2021-10-13T19:37:41Z

Alright, so I think this is good to go ?
I may have missed some edge cases, but they should apprear as explicit errors and I'll try to be responsible to fix them.

vtjnash · 2021-10-13T20:19:11Z

yeah, just keep an eye on the next nightly pkgeval

KristofferC · 2021-10-13T21:11:08Z

Or run PkgEval first?

aviatesk · 2021-10-14T00:34:46Z

@nanosoldier runtests(ALL, vs = ":master")

After <JuliaLang/julia#42583> is merged, `widenconst` doesn't accept non-valid Julia types (i.e. `TypeVar`/`Vararg`), and we will handle them explicitly.

vtjnash · 2021-10-15T15:14:37Z

test/compiler/inference.jl

+        Type{Bound},            # DataType => Type{Bound} where Bound<:Integer
+        Type{unbound},          # DataType => Type
+        Vector{Some{Bound}},    # DataType => Vector{Some{Bound}} where Bound<:Integer
+        Vector{Some{unbound}},  # DataType => Array


You really went overboard here! Most of these cannot happen at runtime, since this would not declare a valid object (for example). Only wrap_Type (Type{T}) can contain an invalid parameter.

So conceptually we want to replace invalid non-Types with Bottom ? It feels a bit weird to keep inference going when we know something never happens at runtime.

No, it just shouldn't occur, so it is a lot of code and tests to handle a situation that can only be user-constructed as arguments to code_typed. Conceptually it only happens in the runtime for Type{T}::DataType, so the replacement could be just:
has_free_typevars(t) ? (isType(t) ? Type : Any) : t

Okay, I got it.
Since I saw some unbound TypeVars during sysimg creation, I thought inference sometimes seemed to forward unbound TypeVars, but it shouldn't and so it might also be bad entry arguments given at somewhere (maybe return_type ?).
Anyway, I believe this PR is solid now so I'm going to merge this once CI gets passed.

nanosoldier · 2021-10-15T17:32:24Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

During working on the incoming lattice overhaul, I found it's quite confusing that `TypeVar` and `Vararg` can appear in the same context as valid `Type` objects as well as extended lattice elements. Since it usually needs special cases to operate on `TypeVar` and `Vararg` (e.g. they can not be used in subtyping as an obvious example), I believe it would be great avoid bugs and catch logic errors in the future development if we separate contexts where they can appear from ones where `Type` objects and extended lattice elements are expected. So this commit: - tries to separate their context, e.g. now `TypeVar` and `Vararg` should not be used in `_limit_type_size`, which is supposed to return `Type`, but they should be handled its helper function `__limit_type_size` - makes sure `tfunc`s don't return `TypeVar`s and `TypeVar` never spills into the abstract state - makes sure `widenconst` are not called on `TypeVar` and `Vararg`, and now `widenconst` is ensured to return `Type` always - and does other general refactors

This commit eliminates unbound `TypeVar`s from `argtypes` in order to make the analysis much easier down the road. At runtime, only `Type{...}::DataType` can contain invalid type parameters, but we may have arbitrary malformed user-constructed type arguments given at inference entries. So we will replace only the malformed `Type{...}::DataType` with `Type` and simply replace other possibilities with `Any`. The replacement might not be that accurate, but an unbound `TypeVar` is really invalid in anyway. Like the change added on `arrayref_tfunc`, now we may be able to remove some `isa(x, TypeVar)`/`has_free_typevars` predicates used in various places, but it is left as an exercise for the reader.

Follows #42583. The missing case within `const_prop_function_heuristic` was originally reported from RationalAI's test suite. I added the other two cases by some code review.

#42971) Follows #42583. The missing case within `const_prop_function_heuristic` was originally reported from RerationalAI's test suite. I added the other two cases by some code review.

#42971) Follows #42583. The missing case within `const_prop_function_heuristic` was originally reported from RerationalAI's test suite. I added the other two cases by some code review. (cherry picked from commit 71d57d9)

JuliaLang#42971) Follows JuliaLang#42583. The missing case within `const_prop_function_heuristic` was originally reported from RerationalAI's test suite. I added the other two cases by some code review.

… computed by `abstract_apply` (#51393) This commit adds special handling for `Vararg` types that may appear at the end of `argtypes`, as computed by `abstract_apply`. Even though PR #42583 has ensured that `Vararg` and `TypeVar` should never appear within the abstract state, they can still be part of `argtypes`. As a result, this kind of special handling is still necessary. It remains an open question whether we can refactor `abstract_apply` to prevent `Vararg`s from appearing in `argtypes` in the first place.

aviatesk requested review from Keno and vtjnash October 10, 2021 18:40

aviatesk added the compiler:inference Type inference label Oct 10, 2021

aviatesk force-pushed the avi/infrefactor branch from 2c6d5b5 to a7a21c6 Compare October 11, 2021 04:04

aviatesk force-pushed the avi/validwidenconst branch 4 times, most recently from d058fe6 to 777b4b0 Compare October 11, 2021 05:47

aviatesk changed the title ~~inference: make widenconst always return Type~~ inference: improve handlings of TypeVar and Vararg Oct 11, 2021

aviatesk mentioned this pull request Oct 11, 2021

inference: mega lattice implementation overhaul #42596

Draft

34 tasks

Base automatically changed from avi/infrefactor to master October 11, 2021 15:38

aviatesk force-pushed the avi/validwidenconst branch 2 times, most recently from 874dd2d to d850968 Compare October 12, 2021 17:51

aviatesk added a commit that referenced this pull request Oct 12, 2021

lattice overhaul: wrap Vararg in TypeLattice.special::Vararg

2a4d11e

With some early-take of #42583.