Skip to content

The body map keys call sites by start position, so getattr(self, x)(y) loses its dynamic call #215

Description

@rahlk

Problem

emit_l1_body keys a callable's body dict on the call site's start position
(codeanalyzer/schema/l1_body.py:9):

key = f"{cs.start_line}:{cs.start_column}"
c.body[key] = BodyNode(kind="call", ...)

Two nested ast.Call nodes can start at the same position. getattr(self, x)(y) is that shape
exactly: the outer application and the inner getattr both begin at the g of getattr.
_call_sites builds both, because _iter_calls_in_scope yields every ast.Call in scope
(symbol_table_builder.py:762). The dict then keeps one. The later write wins, and it is the inner
getattr. The dynamic invocation is destroyed.

Reproducer:

FTYPES = ['pdf', 'docx']


class A:
    def _index_pdf(self, bin_data):
        return bin_data

    def _index(self, bin_data):
        res = False
        for ftype in FTYPES:
            buf = getattr(self, '_index_%s' % ftype)(bin_data)
            if buf:
                res = buf
        return res

ast.walk finds two calls, at one position:

line 11 col 18  func=Call  src="getattr(self, '_index_%s' % ftype)(bin_data)"
line 11 col 18  func=Name  src="getattr(self, '_index_%s' % ftype)"

SymbolTableBuilder.build_pymodule_from_file keeps both:

call_sites (2):
  11:18  method_name='<unknown>'  callee_signature='builtins.getattr'
  11:18  method_name='getattr'    callee_signature='builtins.getattr'

canpy -a 1 keeps one:

body (1 keys):
  '11:18'  kind='call'  method_name='getattr'  callee=None

IdentityMap.for_function uses the same key format (codeanalyzer/dataflow/identity.py:36), so L3
and L4 inherit the collision. Every cfg, cdg and ddg endpoint at that position is ambiguous.

The Neo4j projection then has nothing left to record. _project_program_graphs iterates c.body
(neo4j/project.py:194), so the line gets one :PyBodyNode, and that node resolves — to
builtins.getattr. Measured on a merged Odoo graph: 193 getattr call sites, all 193 carrying a
PY_RESOLVES_TO edge to the builtin, and no node anywhere for the invocation that follows.
getattr(self, x) and getattr(self, x)(y) are indistinguishable in the graph. One reads an
attribute. The other performs a dynamic call.

A second, smaller defect sits at the same site. _callee_anchor returns node.lineno, node.col_offset when the callee is not an ast.Attribute (symbol_table_builder.py:147). The outer
call's func is an ast.Call, so the anchor lands on getattr, and Jedi infers builtins.getattr.
The outer call is labelled as a call to getattr. It calls whatever getattr returned.

The consequence for a consumer. Odoo's IrAttachment._index dispatches with
getattr(self, '_index_%s' % ftype)(bin_data), and the five _index_* methods have zero inbound
PY_CALLS on the graph. That was verified directly. Recording no edge to _index_pdf is correct
behaviour: resolving it needs string-value analysis over ftype against a module-level list, and a
sound call graph should not guess. Recording that a dynamic call happens there is a different fact,
and it is the one that is lost. A reachability consumer has no site to impute from, so it greps
PyCallable.code for getattr(self instead.

Scope boundary

Makes co-positioned call sites addressable, so the dynamic invocation survives into body, into the
L3 and L4 graphs, and into the Neo4j projection. Also fixes the callee anchor for a call whose func
is itself a call. Does NOT resolve the dynamic target: no string-value analysis, no PY_RESOLVES_TO
edge for the invocation, and no new PY_CALLS edge to _index_pdf. Does NOT change any existing
body key that does not collide. Does NOT carry the unparsed text of a non-Name, non-Constant
argument, so '_index_%s' % ftype still reaches arguments_json as
{"ast_kind": "BinOp", "name": null, "value": null}. That belongs in its own issue.

Goals

  • body keeps one node per ast.Call, including two that share a start position
  • The local key stays line:col for a call site with no co-positioned sibling, so no existing id moves
  • IdentityMap.for_function agrees with the new format, so global_id and _global_ordinal still land on one identity
  • sig_by_pos in _project_program_graphs joins per call site, not per position (project.py:186)
  • _callee_anchor gives a call whose func is an ast.Call a callee_signature of None
  • method_name for that call stays <unknown>, so the site is identifiable
  • The checked-in graph schema snapshot is regenerated if any declared property changes

Caveats and known risks

  • The key format is a published identity, not an internal detail. IdentityMap documents a local
    id as "@entry", "@exit" or "line:col", and _global_ordinal must agree with
    IdentityMap.global_id. python-sdk reads the trailing local key and ranks ties on its column
    (body_key_column), and BodyNode carries no id, though the Neo4j projection mints one for every body node #176 and LocateResult.node_id does not join to the graph's node ids python-sdk#320 both concern this id. A new
    discriminant must keep line:col for the common case, and the same rule must still parse it.
    Adding a suffix only on collision is the smallest change that holds both.
  • A chained call has the same shape. f()()() produces three co-positioned calls, and so can an
    inline decorator factory. The discriminant must be stable across runs on unchanged source, so it
    must not depend on ast.walk order alone. Ranking co-positioned siblings by end_col_offset,
    widest first, is deterministic and puts the outermost call first.
  • Existing graphs do not gain the node. The fix applies to new runs. A consumer reading an older
    graph still sees one node per position, so it must tolerate the absence.
  • The argument text is still missing after this fix. The site will say that a dynamic call
    happens, and it will not say which names the call could reach. arguments_json keeps value only
    for a JSON-safe Constant, and name only for a bare Name (symbol_table_builder.py:786). A
    follow-up should carry ast.unparse(arg) for other shapes. Until then a consumer still needs the
    source text to recover '_index_%s'.
  • Do not resolve the dynamic call as part of this. Binding
    getattr(self, '_index_%s' % ftype) to the five _index_* methods needs string-value analysis,
    and it would publish edges the analyzer cannot justify. The point here is that the site is
    recorded, so a consumer can decide for itself.

Definition of done

  • The reproducer above yields two body nodes on line 11. The test names the exact expected key set.
  • The invocation's node has method_name='<unknown>', callee=None and callee_signature=None, and
    it gets no PY_RESOLVES_TO edge.
  • The getattr node still has method_name='getattr' and callee_signature='builtins.getattr'.
  • Every body key in the existing corpus is unchanged for a callable with no co-positioned calls,
    asserted against the current keys rather than against a count.
  • IdentityMap.global_id and the Neo4j _global_ordinal produce the same string for both new nodes.
  • f()()() yields three distinct keys, and two runs on the same source yield the same three.
  • The existing suite is green, and the schema snapshot matches.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions