avm2: Use return type of methods in optimizer

[Lord-McSweeney]

Closes #17495

We now verify that overrides of methods use the same return type as the method they override, which allows us to make use of the return type of all methods in the optimizer.

Box2d stats:

ConstructSuper -> Pop: 70%

FindPropStrict -> GetScriptGlobals: 82.71%
FindPropStrict -> GetOuterScope: 9%
FindPropStrict -> GetScopeObject: 8.28%

FindProperty -> GetOuterScope: 5.66%
FindProperty -> GetScopeObject: 94.33%

InitProperty -> SetSlot: 6.3% (-2.19%)
InitProperty -> SetSlotNoCoerce: 90.95% (+2.19%)
InitProperty -> CallMethod: 2.73%

SetProperty -> SetSlot: 8.07% (-1.72%)
SetProperty -> SetSlotNoCoerce: 73.88% (+1.72%)
SetProperty -> CallMethod: 2.4%

GetProperty -> GetSlot: 88.89%
GetProperty -> CallMethod: 0.47%

CallProperty -> CallMethod: 83.26%
CallProperty -> CoerceUSwapPop: 5.17%
CallProperty -> CoerceISwapPop: 9.56%

CallPropVoid -> CallMethod: 93.93% (+2.65%)

ConstructProp -> ConstructSlot: 100%

Coerce -> Nop: 73.18% (+5.93%)

CoerceD -> Nop: 98.75% (+8.71%)

CoerceB -> Nop: 97.29% (+70.27%)

CoerceU -> Nop: 64.91% (+2.63%)

CoerceI -> Nop: 52% (+5%)

ReturnValue -> ReturnValueNoCoerce: 96.39% (+12.61%)

[adrian17]

Why not do it during (or after) init_vtable? It already contains the "check trait kind" logic. If we did it during init_vtable, we could directly compare method_table[*disp_id as usize] just before setting the override. If we did it after init_vtable, we could lookup the name in resolved_traits instead. Both feel faster and more correct than quadratic loop with name comparisons.

Actually...

I was just checking what FP does and turns out it does both: the trait kind check early and signature check late - and it can be observed.

The "is it legal to override trait kind A with B" and "overriding methods without override is illegal" checks are done early during initial class loading, preparing the main name->property hashmap; FP does this eagerly, while we do init_vtable late - so in FP the class can fail these even if it's never used.

(see Traits::getOverride and where it's used. It does do a lookup on parent by name just like we do match resolved_traits.get() in init_vtable.)

Meanwhile the overridden methods' signatures are compared late, like in newclass. So if the class exists and has a mismatched signature but is never used/referred to, there won't be any VerifyErrors.

(see TraitsBindings::checkOverride, used in VTable::resolveSignatures(self), which is directly called from MethodEnv::newclass (and a dozen other places). It also doesn't concern itself with names anymore, it just compares vtable.methods[dispid] vs parent_vtable.methods[dispid]).

[adrian17]

I'm not saying we need to do it exactly like FP; simply moving the entire logic during/after init_vtable will just prevent some quadratic loops and should be conceptually simpler overall.

[adrian17]

And even if the verification didn't move and stayed before init_vtable, couldn't we still lookup the superclass's fields by name without a loop, just like init_vtable does: resolved_traits.get(trait_data.name())?

[Lord-McSweeney]

The "is it legal to override trait kind A with B" and "overriding methods without override is illegal" checks are done early during initial class loading, preparing the main name->property hashmap; FP does this eagerly, while we do init_vtable late - so in FP the class can fail these even if it's never used.

(see Traits::getOverride and where it's used. It does do a lookup on parent by name just like we do match resolved_traits.get() in init_vtable.)

Meanwhile the overridden methods' signatures are compared late, like in newclass. So if the class exists and has a mismatched signature but is never used/referred to, there won't be any VerifyErrors.

Currently I believe this PR exactly matches FP in this logic. If the signature comparison was done early (at Class creation time instead of ClassObject creation time) the referenced classes would not yet exist and we would get VerifyErrors.

I don't think it makes sense to move the whole verification to init_vtable, as that is "just" meant to create the vtable from traits. For example, it takes a UC instead of an Activation and doesn't return any errors (at least as it is right now). I'll switch it to use resolved_traits.get instead.