Go: Make the models-as-data subtypes column do something more sensible for promoted methods

[owen-mc]

This is feature-complete now, I think. I don't think it needs a change note as models-as-data isn't a feature that we've publicly announced yet.

[owen-mc]

@smowton Do you think the performance of ensureCorrectTypeInfo is okay? Here are the tuple counts on aws/awk-sdk/go (which is very large). On my laptop it takes 15s on 1 thread or 1.2s on 8 threads. (Side note: pretty good parallelisation!) The end result is 1609736 tuples and the largest intermediate tuple count is 241703931, so about 200 times larger. That largest values come in r7, which starts with all values of t2 and calculates all values of field2, which seems backwards. I tried putting some binding pragmas but only made things worse. I tried removing lines 272-278 (which restrict sse and recv) and it almost doubled the time.

[2024-10-23 13:29:15] Found predicate Types::Type.hasQualifiedName/2#dispred#d91da86f_120#join_rhs@532aa68b by cachaca (size: 49920)
[2024-10-23 13:29:30] Evaluated non-recursive predicate FlowSummaryImpl::SourceSinkInterpretationInput::ensureCorrectTypeInfo/2#8b265b65@b4daadkd in 14956ms (size: 1609639).
Evaluated relational algebra for predicate FlowSummaryImpl::SourceSinkInterpretationInput::ensureCorrectTypeInfo/2#8b265b65@b4daadkd with tuple counts:
          1172194    ~0%    {5} r1 = SCAN `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.hasTypeInfo/3#dispred#cb864cc2` OUTPUT In.0, In.1, In.2, In.3, _
                            {4}    | REWRITE WITH NOT [NOT [Tmp.4 := true, TEST InOut.3 = Tmp.4], NOT [Tmp.4 := false, TEST InOut.3 = Tmp.4]] KEEPING 4
          1172194    ~0%    {3}    | SCAN OUTPUT In.1, In.2, In.0
          1173900    ~0%    {2}    | JOIN WITH `Types::Type.hasQualifiedName/2#dispred#d91da86f_120#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.2
                        
          1172194    ~0%    {5} r2 = SCAN `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.hasTypeInfo/3#dispred#cb864cc2` OUTPUT In.0, In.1, In.2, In.3, _
                            {4}    | REWRITE WITH Tmp.4 := true, TEST InOut.3 = Tmp.4 KEEPING 4
           586097    ~0%    {4}    | SCAN OUTPUT In.0, In.1, In.2, _
           586097    ~0%    {4}    | REWRITE WITH Out.3 := true
                        
           586097    ~0%    {3} r3 = JOIN r2 WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.hasTypeInfo/3#dispred#cb864cc2` ON FIRST 4 OUTPUT Lhs.0, Lhs.1, Lhs.2
           586097    ~0%    {4}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.0
           378005    ~0%    {3}    | JOIN WITH `project#Scopes::Method.hasQualifiedName/3#dispred#c331b4b4` ON FIRST 3 OUTPUT Lhs.1, Lhs.2, Lhs.3
           378517    ~0%    {2}    | JOIN WITH `Types::Type.hasQualifiedName/2#dispred#d91da86f_120#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.2
              192    ~5%    {2}    | JOIN WITH `Types::InterfaceType.getAnEmbeddedInterface/0#889bb701_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
              389    ~2%    {2}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
                        
           586097    ~0%    {1} r4 = JOIN r2 WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.hasTypeInfo/3#dispred#cb864cc2` ON FIRST 4 OUTPUT Lhs.0
           586097    ~0%    {2}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62` ON FIRST 1 OUTPUT Rhs.1, Lhs.0
                        
           378005    ~0%    {2} r5 = JOIN r4 WITH `Scopes::Method.getReceiverBaseType/0#dispred#662f8965` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
                        
           208092    ~0%    {2} r6 = JOIN r4 WITH Scopes::Field#2e07e034 ON FIRST 1 OUTPUT Rhs.1, Lhs.1
         24383870    ~2%    {2}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
                        
         24761875    ~3%    {2} r7 = r5 UNION r6
         49259943    ~2%    {2}    | JOIN WITH `Types::lookThroughPointerType/1#ccd91091_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
        241703931    ~0%    {2}    | JOIN WITH objecttypes_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1
                        
          1331796    ~8%    {3} r8 = JOIN r7 WITH `project#Types::StructType.getFieldAtDepth/2#dispred#d127e759#2_20#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.0
          5223335    ~0%    {4}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Lhs.0, Lhs.2, Lhs.1, Rhs.1
          5223335    ~4%    {3}    | JOIN WITH `project#Types::StructType.getFieldAtDepth/2#dispred#d127e759#2_02#join_rhs` ON FIRST 2 OUTPUT Lhs.1, Lhs.2, Lhs.3
          5223335   ~15%    {3}    | JOIN WITH objecttypes ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
          4785701    ~4%    {4}    | JOIN WITH `Types::lookThroughPointerType/1#ccd91091` ON FIRST 1 OUTPUT Lhs.1, _, Lhs.2, Rhs.1
          4785701    ~0%    {4}    | REWRITE WITH Out.1 := true
                        
          1331796    ~0%    {4} r9 = JOIN r7 WITH `project#Types::StructType.getFieldAtDepth/2#dispred#d127e759#2_201#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.0, Rhs.2
          5223335    ~0%    {5}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Lhs.0, Lhs.1, Lhs.2, Lhs.3, Rhs.1
         66692413    ~2%    {6}    | JOIN WITH `project#Types::StructType.getFieldAtDepth/2#dispred#d127e759#2` ON FIRST 1 OUTPUT Rhs.2, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Rhs.1
         66692413   ~99%    {6}    | JOIN WITH objecttypes ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5
         63995947   ~83%    {6}    | JOIN WITH `Types::lookThroughPointerType/1#ccd91091` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5
         63435633  ~103%    {7}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1, Lhs.3, Lhs.4, Lhs.5, Lhs.0
          1722556    ~0%    {8}    | JOIN WITH `project#Types::StructType.getFieldAtDepth/2#dispred#d127e759#2_021#join_rhs` ON FIRST 2 OUTPUT Lhs.2, Lhs.4, Lhs.6, _, _, Rhs.2, Lhs.3, Lhs.5
                            {5}    | REWRITE WITH Tmp.3 := 1, Out.3 := (Tmp.3 + In.5), Out.4 := (In.6 - In.7), TEST Out.4 = Out.3 KEEPING 5
          1710567    ~2%    {4}    | SCAN OUTPUT In.0, _, In.1, In.2
          1710567    ~0%    {4}    | REWRITE WITH Out.1 := true
                        
          6496268    ~0%    {4} r10 = r8 UNION r9
          6484617    ~6%    {5}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.hasTypeInfo/3#dispred#cb864cc2_0312#join_rhs` ON FIRST 2 OUTPUT Lhs.3, Rhs.2, Rhs.3, Lhs.0, Lhs.2
           977821    ~5%    {2}    | JOIN WITH `Types::Type.hasQualifiedName/2#dispred#d91da86f` ON FIRST 3 OUTPUT Lhs.4, Lhs.3
                        
          2152110    ~2%    {2} r11 = r1 UNION r3 UNION r10
          3696824    ~0%    {2}    | JOIN WITH `Types::lookThroughPointerType/1#ccd91091_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
          3678511    ~0%    {3}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1
                        
         30067293    ~0%    {3} r12 = JOIN r11 WITH `project#ControlFlowGraph::Write.writesField/3#dispred#ac3ace1e#2_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
                        
        142223556    ~0%    {3} r13 = JOIN r11 WITH `DataFlowUtil::FieldReadNode.getField/0#dispred#1c1383c2_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
        142223556    ~0%    {3}    | JOIN WITH `DataFlowUtil::ComponentReadNode.getBase/0#dispred#0757f222` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
                        
          3804277    ~1%    {3} r14 = JOIN r11 WITH `DataFlowUtil::CallNode.getTarget/0#dispred#56a22302_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
          3802373    ~0%    {3}    | JOIN WITH `DataFlowUtil::CallNode.getReceiver/0#dispred#fb5c552d#bf` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
                        
        176093222    ~0%    {3} r15 = r12 UNION r13 UNION r14
        176092388    ~2%    {4}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::getSyntacticRecv/1#73ae3efe` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1, Lhs.0
          1609742    ~1%    {2}    | JOIN WITH `DataFlowUtil::Node.getType/0#dispred#e0f50159` ON FIRST 2 OUTPUT Lhs.2, Lhs.3
                            return r15

[smowton]

Code changes otherwise lgtm if performance is good. Haven't reviewed test changes.

[owen-mc]

I ran QA on 5213 go repos. There were two lost alerts. One is due to a model which has subtypes set to false - I will fix this in a follow-up PR setting subtypes = true for all models. The second is due to a bug in our logic about promoted fields and methods of struct types. The underlying problem is that we only consider promoted fields and methods if their embedded parent has a name which doesn't clash with a direct field of the struct type. So if struct type A embeds struct type pkg1.B, and that embeds struct type pkg2.B, and that has a field f and a method m, then we won't realise that f and m should be promoted to A. I have made a separate issue to fix this bug, which I don't think should stop this PR.

However, there are some performances issues that I think need to be dealt with.

[owen-mc]

@smowton Any ideas for how to deal with this bad join order on bramp/antlr4-grammars?

[2024-10-30 10:57:49] Evaluated non-recursive predicate FlowSummaryImpl::SourceSinkInterpretationInput::InterpretNode.getCallTarget/0#dispred#15bc0978@1005c0ib in 439339ms (size: 1354420).
Evaluated relational algebra for predicate FlowSummaryImpl::SourceSinkInterpretationInput::InterpretNode.getCallTarget/0#dispred#15bc0978@1005c0ib with tuple counts:
            561544   ~4%    {2} r1 = SCAN `FlowSummaryImpl::SourceSinkInterpretationInput::InterpretNode.asCall/0#dispred#524b1bb5` OUTPUT In.1, In.0
            561544   ~0%    {2}    | JOIN WITH DataFlowPrivate::DataFlowCall#6d7fbd18 ON FIRST 1 OUTPUT Rhs.1, Lhs.1
                        
            542126   ~0%    {2} r2 = JOIN r1 WITH `DataFlowUtil::CallNode.getTarget/0#dispred#56a22302` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
        1347916366   ~0%    {3}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62_10#join_rhs` ON FIRST 1 OUTPUT Lhs.0, Lhs.1, Rhs.1
                            {3}    | AND NOT Scopes::Method#e27c2e67_0#antijoin_rhs(FIRST 1)
            120502   ~0%    {2}    | SCAN OUTPUT In.1, In.2
                        
            542126   ~0%    {3} r3 = JOIN r1 WITH `DataFlowUtil::CallNode.getTarget/0#dispred#56a22302` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.0
        1347916366   ~4%    {3}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62_10#join_rhs` ON FIRST 1 OUTPUT Lhs.2, Lhs.1, Rhs.1
        1347795864   ~0%    {3}    | JOIN WITH `DataFlowUtil::CallNode.getReceiver/0#dispred#fb5c552d#bf` ON FIRST 1 OUTPUT Lhs.2, Lhs.1, Rhs.1
        1347795864   ~0%    {6}    | JOIN WITH num#FlowSummaryImpl::SourceSinkInterpretationInput::TMethodOrFieldEntityElement#934df8c5_40123#join_rhs ON FIRST 1 OUTPUT Lhs.1, Lhs.0, Lhs.2, Rhs.2, Rhs.3, Rhs.4
                        
        1347795864   ~0%    {7} r4 = SCAN r3 OUTPUT In.0, In.1, In.2, In.3, In.4, In.5, _
                            {6}    | REWRITE WITH NOT [NOT [Tmp.6 := true, TEST InOut.5 = Tmp.6], NOT [Tmp.6 := false, TEST InOut.5 = Tmp.6]] KEEPING 6
        1347795864   ~1%    {6}    | SCAN OUTPUT In.3, In.4, In.0, In.1, In.2, In.5
        1347795864   ~0%    {7}    | JOIN WITH `Types::Type.hasQualifiedName/2#dispred#d91da86f_120#join_rhs` ON FIRST 2 OUTPUT Lhs.0, Lhs.1, Lhs.5, Lhs.3, Lhs.2, Lhs.4, Rhs.2
        1347795864   ~0%    {4}    | JOIN WITH num#FlowSummaryImpl::SourceSinkInterpretationInput::TMethodOrFieldEntityElement#934df8c5_1234#join_rhs ON FIRST 4 OUTPUT Lhs.5, Lhs.6, Lhs.4, Lhs.3
                        
        1347795864   ~0%    {7} r5 = SCAN r3 OUTPUT In.0, In.1, In.2, In.3, In.4, In.5, _
                            {6}    | REWRITE WITH Tmp.6 := true, TEST InOut.5 = Tmp.6 KEEPING 6
         673897932   ~1%    {5}    | SCAN OUTPUT In.3, In.4, In.0, In.1, In.2
         673897932   ~0%    {7}    | JOIN WITH `Types::Type.hasQualifiedName/2#dispred#d91da86f_120#join_rhs` ON FIRST 2 OUTPUT Lhs.0, Lhs.1, _, Lhs.3, Lhs.2, Lhs.4, Rhs.2
         673897932   ~0%    {7}    | REWRITE WITH Out.2 := true
                        
         673897932   ~3%    {4} r6 = JOIN r5 WITH num#FlowSummaryImpl::SourceSinkInterpretationInput::TMethodOrFieldEntityElement#934df8c5_1234#join_rhs ON FIRST 4 OUTPUT Lhs.6, Lhs.4, Lhs.3, Lhs.5
          75341590   ~0%    {4}    | JOIN WITH `Types::InterfaceType.getAnEmbeddedInterface/0#889bb701_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3
         170484585   ~0%    {4}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Lhs.3, Rhs.1, Lhs.1, Lhs.2
                        
         673897932   ~1%    {4} r7 = JOIN r5 WITH num#FlowSummaryImpl::SourceSinkInterpretationInput::TMethodOrFieldEntityElement#934df8c5_1234#join_rhs ON FIRST 4 OUTPUT Lhs.3, Lhs.4, Lhs.5, Lhs.6
         673897932   ~1%    {5}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::SourceOrSinkElement.asEntity/0#dispred#f809bd62` ON FIRST 1 OUTPUT Rhs.1, Lhs.3, Lhs.1, Lhs.0, Lhs.2
          38845887   ~0%    {4}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::getIntermediateEmbeddedType/2#0e951d33_021#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.2, Lhs.3, Lhs.4
         693382676   ~3%    {4}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a_10#join_rhs` ON FIRST 1 OUTPUT Lhs.3, Rhs.1, Lhs.1, Lhs.2
                        
        2211663125   ~3%    {4} r8 = r4 UNION r6 UNION r7
           1233918   ~0%    {2}    | JOIN WITH `FlowSummaryImpl::SourceSinkInterpretationInput::getSyntacticQualifierBaseType/1#7696a4a6` ON FIRST 2 OUTPUT Lhs.2, Lhs.3
                        
           1354420   ~0%    {2} r9 = r2 UNION r8
                            return r9

[smowton]

It's the result.asEntity() = callTarget that causes the blowup in both cases. That implies for a given call target, there are lots of nodes with that entity (I suppose, implying lots of interface embedding going on).

How about we try that call-target -> result mapping happening last? Currently you have

  bindingset[sse, qual]
  pragma[inline_late]
  private predicate elementAppliesToQualifier(SourceOrSinkElement sse, DataFlow::Node qual) {

which implies that we want to constrain both sse and qual, and only then set about checking if elementAppliesToQualifier(sse, qual). If this was just bindingset[qual] then we would bind qual (via the non-problematic cn = this.asCall().getNode() and callTarget = cn.getTarget() that already goes first, then roll through elementAppliesToQualifier using the path qual -> syntacticQualBaseType -> targetType, subtypes -> pkg, typename, subtypes -> sse, which looks promising, then only at the very end would we constrain by result.asEntity() = callTarget.

Note that messing with bindingset will likely do the trick for the elementAppliesToQualifier arm of the disjunction in getCallTarget, but might not also nudge the order of constraints for the not callTarget instanceof Method case. For that one it might be necessary to kick result.asEntity() = callTarget out into a little predicate with bindingset[calltarget] in order to get the not callTarget instanceof Method antijoin to go first.

[owen-mc]

@smowton After much work I managed to get the join order you recommended. Repos like bramp/antlr4-grammars which have a huge amount of embedding were causing performance problems before, e.g. taking 30-50s to evaluate getCallTarget. My latest refactor reduced the time to evaluate that predicate to 77ms, which was very satisfying. The tuple counts for that predicate are all much smaller now, and there are no slow predicates which seem to be linked to this PR, and the time to run the tainted path query (which has no results) is the same. I will run QA again to be sure.

[owen-mc]

I have run QA and the alert changes are fine and there are no noticeable performance changes. This PR is ready to be reviewed.

[smowton]

Also wants a change note. Otherwise LGTM.

[smowton]

I think I was wrong about this -- I think it's only Methods that can have multiple qnames like this, and field embedding is represented more explicitly

Suggested change

	* `Method`s and `Field`s may have multiple qualified names due to embedding.
	* `Method`s may have multiple qualified names due to embedding.

[owen-mc]

I looked into it and actually it is true for fields as well. In our code this is clear because Field.hasQualifiedName calls Type.getField, whose qldoc says "This includes fields promoted from an embedded field."

[owen-mc]

I had to rebase because #17941 was merged. The only merge conflicts were removing the workaround for the bug that that PR fixed (which was to mention *T for every named type T).