fix(ruby): calls to !/? methods and calls on what .new made link their method

CHANGELOG.md

@@ -198,6 +198,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - In C++, a base class like `Message` in `class DynamicMessage final : public Message` now links to the class C++ itself would pick: the one declared in the namespaces around the class, or brought in by the file's `using` declarations and namespace aliases, and the class a `typedef` or `using` alias names. Before, a base class was matched by its name alone, so it could land on a class of the same name in another namespace or another language: in protocolbuffers/protobuf, `DynamicMessage` derived from the JSON parser's internal `Message` class, and in google/leveldb and RocksDB, iterators derived from an unrelated nested `Iterator` class such as the skip list's. The wrong base showed up in `codegraph_explore`'s type hierarchy and in impact, and tied the base's methods to the wrong overrides. A template parameter used as a base no longer links to a class that shares its name, and a base written from the global scope, like `public ::testing::Test`, now links to its class. Re-index C++ projects after upgrading.
 - In C++ and Rust, a call written with a qualifier, like `Status::OK()`, now links to a method of the class it names, never to one of a class whose name only ends the same way. Before, facebook/rocksdb's calls to `Status::OK()`, `Status::InvalidArgument(…)` and `Status::Corruption(…)` linked to `IOStatus`'s methods of those names, a subclass calling its base, like `return FileSystemWrapper::NewWritableFile(…)` or google/leveldb's `return DB::Put(…)`, linked to another subclass or to the calling method itself, and BurntSushi/ripgrep's `Path::new(…)` on the standard library's `Path` linked to its own `PrinterPath::new`. When classes in several languages share the name, the one in the call's own language is taken, so protocolbuffers/protobuf's C++ `DescriptorPool::generated_pool()` calls, which met its Ruby namesake first and linked to nothing, now link to the C++ method. A C++ call through a template parameter, like `CacheShard::ComputeHash(key)` in rocksdb's `template <class CacheShard> class ShardedCache`, no longer links to a class that merely shares words with the parameter's name, unless that class is the parameter's default, like `TcParser` in `template <typename Parser = TcParser>`. A template's value parameter, like `const PrefetchOpts& kOpts`, is no longer mistaken for a type parameter, so references to `PrefetchOpts` inside it now link. Re-index C++ and Rust projects after upgrading.
 - In Ruby, a call on a class written with its namespace, like `Billing::ChargeCard.call(…)` or `::Billing::ChargeCard.call(…)`, now links the caller to that class and to the class method it calls, as `ChargeCard.call(…)` already linked to the class. Before, a class used only through such calls looked like it had no callers. The name is looked up the way Ruby does it, from the namespaces around the call outward, so `Users::Create.call` reaches `Users::Create` and never a `Billing::Create`, a call inside `module Api; module V2` reaches `Api::V2`'s class before another version's, and a library's class like `ActiveRecord::Base` is never taken for a project class of the same last name. Re-index Ruby projects after upgrading. Thanks @james-jang for the report and @drakeo338 for the pull request. (#2503)
+- In Ruby, a call to a method whose name ends in `!` or `?` now links when its receiver's class is known, like `writer.flush!` or `writer.ready?` after `writer = CacheWriter.new`, or `CacheWriter.create!(…)` on the class. A method called straight on a new object, like `CacheWriter.new(key).write_cache` or `Billing::Ledger.new.charge!`, now links to that class's method or to one it inherits. Before, all of these linked nothing. The class is looked up the way Ruby does it, from the namespaces around the call outward, and one from outside your project, like rack's `Request` used inside `Rack::Protection`, links nothing rather than a project class of the same name. Re-index Ruby projects after upgrading. Thanks @ricobl.
 - In PHP, a class written through a namespace alias in a different case than its `use` statement, like `new field\FirstName()` after `use App\Fields as Field;`, now links to that class, since PHP reads namespace names without regard to case. Before, the name was read relative to the file's own namespace and stayed unlinked. Re-index PHP projects after upgrading. Thanks @danusha2345. (#2369)
 - In C++, a call through a class template's specialization written with its arguments, like fmtlib/fmt's `formatter<int>::format(42, ctx)`, no longer links to a member defined for another specialization, like a test's `formatter<incomplete_type>::format`, and when the template declares no such member, as for `formatter<std::tm, Char>::parse(ctx)`, the call is no longer matched to another class's method of that name, like `native_formatter::parse`. A call to the specialization a member is defined for, and to a template's own members, links as before. Re-index C++ projects after upgrading.
 - In C++, a function is now named by its declarator, never by something written in the clauses after its parameters. nlohmann/json's constructor `basic_json(CompatibleType&& val) noexcept(noexcept(JSONSerializer<U>::to_json(…)))` was missing from the index, replaced by a method `JSONSerializer::to_json` that doesn't exist and that other `JSONSerializer<T>::to_json(…)` calls linked to; a method declared with `throw(std::bad_alloc)` was indexed as `std::bad_alloc`, and a function with a trailing `requires std::integral<T>` as `std::integral<T>`. Re-index C++ projects after upgrading.

__tests__/kernel-ruby-parity.test.ts

@@ -148,6 +148,51 @@ describe.skipIf(!kernelBuilt)('kernel Ruby extraction parity', () => {
     ]);
   });
 
+  // A call on what `.new` made (`CacheWriter.new(key).write_cache`, #684) is
+  // recorded as `new CacheWriter().write_cache` on both backends: the class as
+  // written, without the arguments the receiver's text carried. A `new` on a
+  // value (`klass.new.run`, `self.class.new`) or a later link of a chain keeps
+  // the receiver's text, as before.
+  it('calls on a constructed receiver', () => {
+    const src = [
+      'class Callers',
+      '  def a(key, klass)',
+      '    CacheWriter.new.write_cache(key)',
+      '    CacheWriter.new(key, ttl: 5).flush!',
+      '    Billing::Ledger.new { |l| l }.post',
+      '    ::CacheWriter.new.ready?',
+      '    CacheWriter.new&.write_cache(key)',
+      '    klass.new.run',
+      '    self.class.new(key).run',
+      '    Widget.create(key).save',
+      '    CacheWriter.new.first.second',
+      '  end',
+      'end',
+      '',
+    ].join('\n');
+    assertParity('src/callers.rb', src);
+    process.env.CODEGRAPH_KERNEL = '0';
+    const calls = extractFromSource('src/callers.rb', src, 'ruby').unresolvedReferences
+      .filter((r) => r.referenceKind === 'calls' && /[.(]/.test(r.referenceName))
+      .map((r) => `${r.referenceName} ${r.line}`);
+    delete process.env.CODEGRAPH_KERNEL;
+    expect(calls.sort()).toEqual([
+      'CacheWriter.new.first.second 11',
+      'Widget.create 10',
+      'Widget.create(key).save 10',
+      'klass.new 8',
+      'klass.new.run 8',
+      'new ::CacheWriter().ready? 6',
+      'new Billing::Ledger().post 5',
+      'new CacheWriter().first 11',
+      'new CacheWriter().flush! 4',
+      'new CacheWriter().write_cache 3',
+      'new CacheWriter().write_cache 7',
+      'self.class.new 9',
+      'self.class.new(key).run 9',
+    ]);
+  });
+
   // A class method (`def self.x`, `def Widget.x`, a `def` in `class << self`)
   // is static on both backends, every other def is not: they share a
   // qualified name with any instance method of the name.

__tests__/ruby-constructed-receivers.test.ts

@@ -0,0 +1,294 @@
+/**
+ * Ruby calls on a typed receiver whose method name ends in `!` or `?`, and
+ * calls on what `.new` made (#684).
+ *
+ * `writer.flush!` and `writer.ready?` after `writer = CacheWriter.new` linked
+ * nothing, while `writer.write_cache` linked: the method-call pattern took no
+ * `!` or `?`. `CacheWriter.create!` linked nothing where `CacheWriter.create`
+ * did, for the same reason.
+ *
+ * `CacheWriter.new(key).write_cache` was recorded with the receiver's text,
+ * arguments and all, so it never linked. It is now `new CacheWriter()
+ * .write_cache`, and links an instance method of the class Ruby's constant
+ * lookup finds for `CacheWriter` where the call is written, its own or one
+ * its ancestry declares. A class the project doesn't declare links nothing:
+ * rack-protection's `Request.new(env)` is rack's `Rack::Request`, not the
+ * project's `Sinatra::Request`.
+ *
+ * Runs on the native kernel (skipped when none is built) and the wasm
+ * extractor.
+ */
+import { describe, it, expect, afterEach } from 'vitest';
+import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+import { CodeGraph } from '../src';
+import { getKernel } from '../src/extraction/kernel/loader';
+
+const FILES: Record<string, string> = {
+  'app/writer.rb': `class CacheWriter
+  def self.create(key)
+    new
+  end
+
+  def self.create!(key)
+    new
+  end
+
+  def write_cache(key)
+    key
+  end
+
+  def flush!
+    nil
+  end
+
+  def ready?
+    true
+  end
+end
+
+class OtherWriter
+  def write_cache(key)
+    key
+  end
+
+  def flush!
+    nil
+  end
+
+  def ready?
+    false
+  end
+end
+`,
+  'app/billing.rb': `module Billing
+  class Card
+    def charge!(amount)
+      amount
+    end
+  end
+
+  class Ledger < Card
+    def post
+      Card.new.charge!(1)
+    end
+  end
+end
+
+module Admin
+  class Card
+    def charge!(amount)
+      amount
+    end
+  end
+end
+`,
+  'app/counter.rb': `class Counter
+  def bang
+    writer = CacheWriter.new
+    writer.flush!
+  end
+
+  def question
+    writer = CacheWriter.new
+    writer.ready?
+  end
+
+  def chained(key)
+    CacheWriter.new.write_cache(key)
+  end
+
+  def chained_args(key)
+    CacheWriter.new(key).write_cache(key)
+  end
+
+  def chained_bang
+    CacheWriter.new(1).flush!
+  end
+
+  def namespaced
+    Billing::Ledger.new.charge!(2)
+  end
+
+  def top_level
+    ::CacheWriter.new.ready?
+  end
+
+  def class_bang
+    CacheWriter.create!(1)
+  end
+
+  def outside(env)
+    Request.new(env).preferred_type
+  end
+end
+`,
+  'lib/rack/show_exceptions.rb': `module Rack
+  module Protection
+    class ShowExceptions
+      def call(env)
+        Request.new(env).preferred_type("text/html")
+      end
+    end
+  end
+end
+`,
+  'lib/sinatra/request.rb': `module Sinatra
+  class Request
+    def preferred_type(*types)
+      types.first
+    end
+  end
+end
+`,
+  // A module a call returns is no mixin of the constant the call starts at:
+  // UrlForHelper doesn't get RouteSet's methods (actionpack's url_for tests).
+  'app/url_helper.rb': `class RouteSet
+  def url_for(options)
+    options
+  end
+
+  def url_helpers
+    Module.new
+  end
+end
+
+class UrlForHelper
+  include RouteSet.new.url_helpers
+end
+
+class UrlForTest
+  def test_url
+    UrlForHelper.new.url_for(1)
+  end
+end
+`,
+  // Two project methods named `key?`: a `?` call on an untyped receiver is not
+  // guessed between them by the receiver's words.
+  'lib/options.rb': `class InheritableOptions
+  def key?(key)
+    true
+  end
+end
+
+class FlashHash
+  def key?(name)
+    false
+  end
+end
+
+class Store
+  def merged(options)
+    options.key?(:namespace)
+  end
+end
+`,
+};
+
+let root = '';
+let cg: CodeGraph | undefined;
+let savedKernel: string | undefined;
+
+afterEach(() => {
+  cg?.close();
+  cg = undefined;
+  if (root) fs.rmSync(root, { recursive: true, force: true });
+  root = '';
+  if (savedKernel === undefined) delete process.env.CODEGRAPH_KERNEL;
+  else process.env.CODEGRAPH_KERNEL = savedKernel;
+});
+
+function write(files: Record<string, string>): void {
+  for (const [rel, content] of Object.entries(files)) {
+    fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true });
+    fs.writeFileSync(path.join(root, rel), content);
+  }
+}
+
+async function index(backend: string, files: Record<string, string> = FILES): Promise<CodeGraph> {
+  savedKernel = process.env.CODEGRAPH_KERNEL;
+  if (backend === 'wasm') process.env.CODEGRAPH_KERNEL = '0';
+  else delete process.env.CODEGRAPH_KERNEL;
+  root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ruby-made-'));
+  write(files);
+  cg = await CodeGraph.init(root, { index: true });
+  return cg;
+}
+
+/** `<qualified name>:<line>` of every `calls` edge written on `line` of `file`. */
+function callsAt(graph: CodeGraph, file: string, line: number): string[] {
+  const ids = graph.getNodesInFile(file).map((n) => n.id);
+  return graph
+    .getOutgoingEdgesFrom(ids)
+    .filter((e) => e.kind === 'calls' && e.line === line)
+    .map((e) => {
+      const t = graph.getNode(e.target)!;
+      return `${t.qualifiedName}:${t.startLine}`;
+    })
+    .sort();
+}
+
+const backends = [
+  ['kernel', getKernel() === null],
+  ['wasm', false],
+] as const;
+
+describe.each(backends)('Ruby bang and question methods, and calls on .new (%s, #684)', (backend, skip) => {
+  it.skipIf(skip)('links a `!` or `?` method called on a typed local', async () => {
+    const graph = await index(backend);
+    expect(callsAt(graph, 'app/counter.rb', 4)).toEqual(['CacheWriter::flush!:14']);
+    expect(callsAt(graph, 'app/counter.rb', 9)).toEqual(['CacheWriter::ready?:18']);
+  });
+
+  it.skipIf(skip)('links a `!` class method', async () => {
+    const graph = await index(backend);
+    expect(callsAt(graph, 'app/counter.rb', 33)).toEqual(['CacheWriter::create!:6']);
+  });
+
+  it.skipIf(skip)('links a call on what `.new` made, with or without arguments', async () => {
+    const graph = await index(backend);
+    expect(callsAt(graph, 'app/counter.rb', 13)).toEqual(['CacheWriter::write_cache:10']);
+    expect(callsAt(graph, 'app/counter.rb', 17)).toEqual(['CacheWriter::write_cache:10']);
+    expect(callsAt(graph, 'app/counter.rb', 21)).toEqual(['CacheWriter::flush!:14']);
+    expect(callsAt(graph, 'app/counter.rb', 29)).toEqual(['CacheWriter::ready?:18']);
+  });
+
+  it.skipIf(skip)('finds the class by Ruby constant lookup, and the method in its ancestry', async () => {
+    const graph = await index(backend);
+    // `Card` inside `Billing::Ledger` is Billing::Card, not Admin::Card.
+    expect(callsAt(graph, 'app/billing.rb', 10)).toEqual(['Billing::Card::charge!:3']);
+    // Billing::Ledger has no `charge!`; its superclass Billing::Card does.
+    expect(callsAt(graph, 'app/counter.rb', 25)).toEqual(['Billing::Card::charge!:3']);
+  });
+
+  it.skipIf(skip)('links nothing for a class the project does not declare', async () => {
+    const graph = await index(backend);
+    expect(callsAt(graph, 'app/counter.rb', 37)).toEqual([]);
+    expect(callsAt(graph, 'lib/rack/show_exceptions.rb', 5)).toEqual([]);
+  });
+
+  it.skipIf(skip)("finds no method in a module a call returns, which the class's ancestry doesn't name", async () => {
+    const graph = await index(backend);
+    expect(callsAt(graph, 'app/url_helper.rb', 17)).toEqual([]);
+  });
+
+  it.skipIf(skip)('guesses no `?` method by the receiver word among several of the name', async () => {
+    const graph = await index(backend);
+    // `options` shares a word with InheritableOptions, but a Hash answers `key?` too.
+    expect(callsAt(graph, 'lib/options.rb', 15)).toEqual([]);
+  });
+});
+
+describe('Ruby call on .new, after a sync (#684)', () => {
+  it('links the call once a sync adds the class it constructs', async () => {
+    const { 'app/writer.rb': writer, ...rest } = FILES;
+    const graph = await index('default', rest);
+    expect(callsAt(graph, 'app/counter.rb', 17)).toEqual([]);
+
+    write({ 'app/writer.rb': writer! });
+    expect((await graph.sync()).filesAdded).toBe(1);
+    // counter.rb did not change: only the retry of its parked refs links it.
+    expect(callsAt(graph, 'app/counter.rb', 17)).toEqual(['CacheWriter::write_cache:10']);
+  }, 60_000);
+});

codegraph-kernel/src/ruby.rs

@@ -212,6 +212,21 @@ impl<'t> Walker<'t> {
     fn text(&self, node: Node) -> &'t str {
         &self.src[node.byte_range()]
     }
+    /// rubyConstructedClass (tree-sitter.ts): the class a call receiver
+    /// constructs, as written — `Foo` for `Foo.new(key)`, `Billing::Card` for
+    /// `Billing::Card.new`, `::Foo` for `::Foo.new`; None for any other
+    /// receiver, a `new` on a value (`klass.new`) among them.
+    fn constructed_class(&self, receiver: Node) -> Option<&'t str> {
+        if receiver.kind() != "call" {
+            return None;
+        }
+        let method = receiver.child_by_field_name("method")?;
+        let made = receiver.child_by_field_name("receiver")?;
+        if self.text(method) != "new" || !is_constant_path(made) {
+            return None;
+        }
+        Some(self.text(made))
+    }
     fn line_of(&self, node: Node) -> u32 {
         node.start_position().row as u32 + 1
     }
@@ -826,6 +841,10 @@ impl<'t> Walker<'t> {
         let skip = matches!(receiver_name, "self" | "super");
         let callee = if skip {
             method_name.to_string()
+        } else if let Some(class_name) = self.constructed_class(receiver) {
+            // A call on what `Foo.new(…)` made: `new Foo().m`, not the
+            // receiver's text with its arguments in it (#684).
+            format!("new {class_name}().{method_name}")
         } else {
             format!("{receiver_name}.{method_name}")
         };