Fix typeassert dropping PartialStruct information #36622

Keno · 2020-07-11T20:47:16Z

There was a complaint on discourse that Generator splats weren't
infering precisely enough [1]. I thought the recent improvement
to abstract iteration precision would have fixed that, but that
turns out not to be the case for a stupid reason. At the moment,
any type assert (even something silly like x::Any), will drop
PartialStruct information. Now, generators have an
::Tuple{Any, Any} typeassert, which prevented the new iteration
logic from working properly. This fixes typasserts, to not drop
this information. The first couple of conditions that just
return the original PartialStruct if the typeassert has no
effect are doing most of the work here. As an add-on, I also
threw in the logic to narrow the PartialStruct if the typeassert
actually introduces additional information. It's not a particularly
common code path, but might as well.

[1] https://discourse.julialang.org/t/tuples-iterators-splats-and-type-stability/42849

martinholters

Generally LGTM.

Without knowing that it's defined in the middle of _tfuncs, the name typeassert_instance might be quite misleading, but I don't want to stall this PR in naming bikeshedding.

martinholters · 2020-07-13T06:20:54Z

base/compiler/tfuncs.jl

-function typeassert_tfunc(@nospecialize(v), @nospecialize(t))
-    t = instanceof_tfunc(t)[1]
-    t === Any && return v
+function typeassert_instance(@nospecialize(v), @nospecialize(t))


I'm not too happy about this name, though I don't really have a better idea. Maybe at least drop a comment that t here is the one meant to be the "instance" (of a Type{...}), not v?

Maybe just make it an inner function inside typeassert_tfunc?

vtjnash · 2020-07-13T18:33:13Z

base/compiler/tfuncs.jl

+        has_free_typevars(t) && return v
+        widev = widenconst(v)
+        newT = typeintersect(widev, t)
+        if newT === widev


Suggested change

if newT === widev

if newT == widev

I know these seem almost the same (since .typ is usually concrete), but the type-system knows that too, and can handle the exceptions better

I figured the case where typeintersect would return something that's == to one of the inputs, but not === to one of the inputs would be pretty rare, and in the case where it doesn't, using === would bypass the useless sometype query that we don't really care about.

vtjnash · 2020-07-13T18:38:44Z

base/compiler/tfuncs.jl

+        newT = typeintersect(widev, t)
+        if newT === widev
+            return v
+        end


Suggested change

newT = typeintersect(widev, t)

if newT === widev

return v

end

if widev <: t

return v

elseif typeintersect(widev, t) === Bottom

return Bottom

end

Calling typeintersect here would force us to call abstract_apply on tuple_tfunc over the result, to recompute widev from newT. That's awkward (sometimes more accurate, sometimes less), so let's avoid that.

I don't understand what you're saying here.

vtjnash · 2020-07-13T18:44:45Z

base/compiler/tfuncs.jl

+        end
+        new_fields = Vector{Any}(undef, length(v.fields))
+        for i = 1:length(new_fields)
+            new_fields[i] = typeassert_instance(v.fields[i], getfield_tfunc(newT, Const(i)))


Suggested change

new_fields[i] = typeassert_instance(v.fields[i], getfield_tfunc(newT, Const(i)))

new_fields[i] = typeassert_instance(v.fields[i], getfield_tfunc(t, Const(i)))

This would be strictly more precise.

vtjnash · 2020-07-13T18:54:58Z

base/compiler/tfuncs.jl

+                return Bottom
+            end
+        end
+        return PartialStruct(newT, new_fields)


Suggested change

return PartialStruct(newT, new_fields)

return tuple_tfunc(new_fields)

This would be strictly more correct.

t is not necessarily a tuple

vtjnash · 2020-07-13T18:55:40Z

base/compiler/tfuncs.jl

+function typeassert_tfunc(@nospecialize(v), @nospecialize(t))
+    t = instanceof_tfunc(t)[1]
+    t === Any && return v
+    typeassert_instance(v, t)


Suggested change

typeassert_instance(v, t)

return typeassert_instance(v, t)

I know a computer doesn't care about the different, but I treat it as a trivial comment for future human readers

There was a complaint on discourse that Generator splats weren't infering precisely enough [1]. I thought the recent improvement to abstract iteration precision would have fixed that, but that turns out not to be the case of a stupid reason. At the moment, any type assert (even something silly like `x::Any`), will drop `PartialStruct` information. Now, generators have an `::Tuple{Any, Any}` typeassert, which prevented the new iteration logic from working properly. This fixes typasserts, to not drop this information. The first couple of conditions that just return the original `PartialStruct` if the typeassert has no effect are doing most of the work here. As an add-on, I also threw in the logic to narrow the PartialStruct if the typeassert actually introduces additional information. It's not a particularly common code path, but might as well. [1] https://discourse.julialang.org/t/tuples-iterators-splats-and-type-stability/42849

Keno · 2020-07-15T23:15:13Z

@vtjnash points out that currently if the type inside a PartialStruct is not a tuple, it must be concrete, so the fields parts of this didn't apply and just re-doing tuple_tfunc would be strictly more precise. Updated to do that.

There was a complaint on discourse that Generator splats weren't infering precisely enough [1]. I thought the recent improvement to abstract iteration precision would have fixed that, but that turns out not to be the case of a stupid reason. At the moment, any type assert (even something silly like `x::Any`), will drop `PartialStruct` information. Now, generators have an `::Tuple{Any, Any}` typeassert, which prevented the new iteration logic from working properly. This fixes typasserts, to not drop this information. The first couple of conditions that just return the original `PartialStruct` if the typeassert has no effect are doing most of the work here. As an add-on, I also threw in the logic to narrow the PartialStruct if the typeassert actually introduces additional information. It's not a particularly common code path, but might as well. [1] https://discourse.julialang.org/t/tuples-iterators-splats-and-type-stability/42849

Keno requested review from JeffBezanson, martinholters and vtjnash July 11, 2020 20:47

Keno added compiler:inference Type inference iteration Involves iteration or the iteration protocol labels Jul 11, 2020

Keno force-pushed the kf/partialstructtypeassert branch from 029bca4 to 4954d05 Compare July 12, 2020 00:45

martinholters approved these changes Jul 13, 2020

View reviewed changes

JeffBezanson approved these changes Jul 13, 2020

View reviewed changes

vtjnash requested changes Jul 13, 2020

View reviewed changes

Keno force-pushed the kf/partialstructtypeassert branch from 4954d05 to b7aad0e Compare July 13, 2020 19:09

Keno force-pushed the kf/partialstructtypeassert branch from b7aad0e to 4533ea4 Compare July 15, 2020 23:14

vtjnash approved these changes Jul 16, 2020

View reviewed changes

Keno merged commit 8a0e0c8 into master Jul 16, 2020

Keno deleted the kf/partialstructtypeassert branch July 16, 2020 20:25

	new_fields[i] = typeassert_instance(v.fields[i], getfield_tfunc(newT, Const(i)))
	new_fields[i] = typeassert_instance(v.fields[i], getfield_tfunc(t, Const(i)))

	return PartialStruct(newT, new_fields)
	return tuple_tfunc(new_fields)

Uh oh!

Fix typeassert dropping PartialStruct information #36622

Fix typeassert dropping PartialStruct information #36622

Uh oh!

Conversation

Keno commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinholters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keno commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Keno commented Jul 11, 2020 •

edited

Loading