Skip to content

fix(apex): match qualified and generic method return types (#3217) - #3276

Open
Soumyadip2003-AI wants to merge 2 commits into
Graphify-Labs:v8from
Soumyadip2003-AI:fix/apex-qualified-generic-return-types
Open

fix(apex): match qualified and generic method return types (#3217)#3276
Soumyadip2003-AI wants to merge 2 commits into
Graphify-Labs:v8from
Soumyadip2003-AI:fix/apex-qualified-generic-return-types

Conversation

@Soumyadip2003-AI

Copy link
Copy Markdown

Fixes #3217.

The bug

method_re in graphify/extractors/apex.py matched a method's return type with a single character class:

rf"^{_ANNOTATION}\s*{_ACCESS}{_MOD}\s*(?:static\s+)?[\w<>\[\]]+\s+(\w+)\s*\([^)]*\)..."

[\w<>\[\]]+ admits word characters and angle/square brackets only. Any signature whose return type carries a dot, a comma, or a space therefore never matches, and the method is dropped from the graph entirely — no node, no method edge, no warning.

That covers the two most common shapes in real Apex:

  • Database.QueryLocator — every Database.Batchable implementation's start()
  • Map<String, Object> — the @AuraEnabled controller convention
  • List<Map<String, Id>> — nested generics

Repro

public with sharing class Repro {
    public static String simpleReturn() { return ''; }
    public static Map<String, Object> commaGeneric() { return null; }
    public Database.QueryLocator dottedReturn(Database.BatchableContext bc) { return null; }
    private static List<Map<String, Id>> nestedGeneric() { return null; }
    global Set<Id> setReturn() { return null; }
}
before: ['.execute()', '.setReturn()', '.simpleReturn()']
after:  ['.commaGeneric()', '.dottedReturn()', '.execute()', '.nestedGeneric()', '.setReturn()', '.simpleReturn()']

The fix

One _TYPE sub-pattern for an Apex type expression, admitting a namespace-qualified name and generic arguments:

_TYPE = r"[\w.<>\[\]]+(?:\s*,\s*[\w.<>\[\]]+)*"

The important constraint is that whitespace is admitted only adjacent to a comma. A blanket \s in the type class would let ordinary statements be read as declarations — insert new Account(Name = 'x'); would match with return type insert and method name new... Account. Restricting whitespace to comma-adjacent positions keeps every such statement unmatchable, because the type and the method name still have to be separated by whitespace that the type itself cannot swallow.

Verification

  • Extraction of the existing fixtures (tests/fixtures/sample.cls, sample.trigger) is byte-identical before and after — no new nodes, no lost ones.
  • Two tests added in tests/test_languages.py:
    • test_apex_method_qualified_and_generic_return_types — the five signature shapes above.
    • test_apex_statements_are_not_read_as_methods — a negative guard asserting that insert new Account(...), System.assertEquals(1, ids.size()), Map<String, Object> m = new Map<String, Object>();, results.put('a', compute(x));, this.helper(1, 2); and Integer a = 1, b = compute(); still produce exactly one method node.
  • Full suite: 5299 passed, 12 skipped. The one failure, tests/test_labeling.py::test_label_communities_batches_when_over_batch_size, reproduces identically on a clean checkout of v8 and is unrelated to this change.

Out of scope

While building the repro I hit a second, separate bug in the same file that this PR deliberately does not touch: the class heritage clause ((?:\s+extends\s+(\w+))?(?:\s+implements\s+([\w,\s]+))?) also stops at a dot, so class Batch implements Database.Batchable<sObject>, Schedulable fabricates a node labelled Database and loses Schedulable entirely. Filed separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UEfivKVYzq1e5E6hJHiuKi

…Labs#3217)

`method_re` matched the return type with `[\w<>\[\]]+`, so any signature whose
return type carried a dot, a comma, or a space never matched and the method was
dropped with no node, no edges, and no warning. That covers the two most common
shapes in real Apex: `Database.QueryLocator` (every `Database.Batchable.start()`)
and `Map<String, Object>` (the `@AuraEnabled` controller convention), plus
nested generics like `List<Map<String, Id>>`.

Widen the return type to a single `_TYPE` pattern that admits a
namespace-qualified name and generic arguments. Whitespace is admitted only
adjacent to a comma, so a statement such as `insert new Account(...)` or
`Integer a = 1, b = compute();` still cannot be read as a declaration -
extraction of the existing fixtures is byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UEfivKVYzq1e5E6hJHiuKi

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Widens the Apex method-signature regex to accept namespace-qualified and generic return types (Database.QueryLocator, Map<String, Object>, List<Map<String, Id>>), so those methods are now extracted instead of dropped. The new _TYPE pattern only allows whitespace adjacent to a comma, keeping statements like insert new Account(...) from being misread as declarations (#3217). Adds tests covering the qualified/generic return types and confirming ordinary statement bodies yield no spurious method nodes.

Worth a look

  • Generic return types with legal whitespace around angle brackets are skippedgraphify/extractors/apex.py:81 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 560 functions depend on the 560 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract_apex() — 17 callers, 4 callees

Verification — 560 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 560 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify extract\_apex.

The verifier did not have enough to check extract\_apex, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 1 more finding(s) on lines outside this diff (see the check run).

… type

Review follow-up. The first pass admitted whitespace only next to a comma, so a
return type spaced around the angle brackets themselves - all legal Apex - was
still dropped:

    public Map <String, Object> spaceBeforeAngle()   # dropped
    public List< Account > spacesInside()            # dropped
    public List < Map< String, Id > > roomy()        # dropped

Read the type as segments joined by the type punctuators `<`, `>` and `,`, with
whitespace allowed only adjacent to one of them and never between two bare
words. That separator rule is what still bars an ordinary statement from being
read as a declaration, so a bare `<`/`>` used as a comparison
(`if (a > b) { doIt(x); }`, `while (i < list.size()) { next(); }`) fabricates
nothing; the negative test grew those cases plus a generic cast, and extraction
of the existing fixtures is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UEfivKVYzq1e5E6hJHiuKi
@Soumyadip2003-AI

Copy link
Copy Markdown
Author

Thanks — the advisory finding is correct, and I reproduced it. Pushed a follow-up commit.

_TYPE admitted whitespace only next to a comma, so a return type spaced around the angle brackets themselves — all legal Apex — was still dropped:

public Map<String, Object>  tight()             { return null; }  // matched
public Map<String , Object> spaceBeforeComma()  { return null; }  // matched (\s*,\s*)
public Map <String, Object> spaceBeforeAngle()  { return null; }  // DROPPED
public List< Account >      spacesInside()      { return null; }  // DROPPED
public List < Map< String, Id > > roomy()       { return null; }  // DROPPED

Fix

Read the type as segments joined by the type punctuators <, > and ,, with whitespace admitted only adjacent to one of them and never between two bare words:

_TYPE = r"[\w.\[\]]+(?:\s*[<>,]\s*[\w.\[\]]*)*"

That separator rule is what still bars an ordinary statement from being read as a declaration: the return type and the method name have to be separated by whitespace the type itself cannot swallow, and _TYPE can only cross whitespace by consuming a <, > or ,.

On the widened pattern not re-opening the false-positive risk

The obvious worry is that admitting < and > as separators lets a comparison operator glue a statement into a fake declaration. It does not, and the negative test grew to cover it — if (a > b) { doIt(x); }, while (i < list.size()) { next(); }, and a generic cast String s = (Map<String, Object>) JSON.deserializeUntyped(raw); all still produce zero method nodes, alongside the existing insert new Account(...) / System.assertEquals(...) / Integer a = 1, b = compute(); cases.

Measured over the two spellings, positives found / statements misread as declarations:

pattern generic + qualified returns matched statements misread
before 6 / 9 0 / 13
after 9 / 9 0 / 13

Extraction of tests/fixtures/sample.cls and sample.trigger is still unchanged (15 nodes / 18 edges, 3 / 2). Full suite 5299 passed, 12 skipped; the lone tests/test_labeling.py::test_label_communities_batches_when_over_batch_size failure reproduces on a clean v8 checkout and is unrelated.


On the other two sections of the review, for the record: the formal verification row is an honest abstention (path is annotated Path, outside the synthesizable set) rather than a signal, and the health line — extract_apex(), 17 callers — is the pre-existing fan-in of the extractor dispatch table, not coupling this change introduces; the diff adds no call edge.

One thing I could not act on: the review ends with "1 more finding(s) on lines outside this diff (see the check run)", but that finding does not appear in the check-run output or in the PR's review-comment payload, so I have no way to read it. If it can be surfaced I am happy to address it. While testing I did notice one pre-existing false positive that may be it — return foo(x); yields a spurious .foo() node, because _CONTROL_FLOW is checked against the method name but never against the return type. It reproduces identically on v8 (33362d9), so it is not introduced here; say the word and I will file it separately rather than widen this PR.

🤖 Generated with Claude Code

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Tightens the Apex method-declaration regex so return types are matched as </>/,-punctuated segments rather than a single [\w<>\[\]]+ token, correctly recognizing namespace-qualified (Database.QueryLocator), generic (Map<String, Object>), nested-generic, array, and whitespace-loose (Map <String, Id>) returns. Because whitespace is only allowed adjacent to a type punctuator, statements like insert new Account(...) and bare comparisons (if (a > b)) no longer register as declarations. Adds tests covering both the newly-recognized return-type forms and the statements that must not be read as methods.

No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 560 functions depend on the 560 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract_apex() — 17 callers, 4 callees

Verification — 560 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 560 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify extract\_apex.

The verifier did not have enough to check extract\_apex, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 1 more finding(s) on lines outside this diff (see the check run).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apex: method_re drops methods with commas/dots in return type (Map<String, Object>, Database.QueryLocator)

1 participant