mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-21 08:47:04 +08:00
fix(ruby): a method reached by name alone must be in reach (#2147)
Ruby types nothing, so two guesses produced most of a Ruby repo's wrong
call edges:
- `receiver.m()` through an untyped receiver took the project's one method
named `m`. It now also needs a word shared between the receiver and the
method's owner (`web_push_request.legacy_encrypt` → WebPushRequest);
`self` / `self.class` are exempt.
- a bare `m()` inside a class took any class's `m`. It now reaches only the
class's ancestry — superclasses and include/extend/prepend modules, read
from source and resolved against the lexical nesting. Calls in module
bodies and top-level blocks (specs, DSLs) are not judged.
A/B (edges removed / added): rubocop -4406/+262, mastodon -2642/+540,
jekyll -343/+10, lobsters -243/+4, sinatra -207/+2, devise -112/+3.
Deliberate losses: instance_eval'd DSL blocks inside a class method
(sinatra's own tests' `mock_app { get '/' }`), and helpers on a framework
class the repo reopens past an external superclass (devise setup_mailer).
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -14,6 +14,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
|
||||
### Fixes
|
||||
|
||||
- In Ruby, a call no longer lands on an unrelated class's method just because it's the only method with that name. A call on an untyped receiver now needs the receiver to be named after the method's class, like `web_push_request.legacy_encrypt`. A call with no receiver inside a class now reaches only that class's methods, its superclasses' methods, and those of the modules it includes, extends or prepends. On rubocop, `node.loc` on a syntax node used to land on the project's one `loc` method over 1,200 times, and Kernel's `format(…)` on the language server's `format`. On mastodon, `render` in controllers used to land on an inline renderer, and `authorize` now reaches the authorization concern.
|
||||
- In Kotlin, a call to a top-level or extension function now resolves only to one the file can see: one in its own package, one it imports by name, or one from a package it imports with `*`. On Exposed, tests that imported kotlin.test's or JUnit's `assertEquals`, or the JDBC suite's helper, used to land on the R2DBC suite's helper over 3,000 times. On okhttp, tests that import `mockwebserver3.MockResponse` used to construct the deprecated class of the same name. A function type with a qualified receiver, like `(DatabaseConfig.Builder.() -> Unit)?`, no longer breaks the parse of the class around it. Re-index after upgrading.
|
||||
- In Dart, a call written without a receiver now reaches only a method of the class it's written in, of a class it extends, mixes in or implements, or of an extension on one of those types, and picks the nearest one. On riverpod, package:test's `test(…)` in the test files used to land on a container class's `test` method over 1,100 times, shelf's and getx's `expect(…)` on unrelated classes' `expect`, and bloc's `emit(…)` in a Cubit on the wrong base class. Classes declared as `class A = B with C;` are now indexed under their name, with what they extend and mix in. Re-index after upgrading.
|
||||
- In R, a variable assigned at the top of a test file is no longer linked from the package or from other tests, because testthat runs each test file in its own environment. On ggplot2, a test's `c <- ggplot(…)` used to collect 2,455 of the package's `c(…)` calls. Variables in `helper-*.R` and `setup-*.R`, which testthat loads for every test, are still shared.
|
||||
|
||||
@@ -0,0 +1,137 @@
|
||||
/**
|
||||
* Ruby has nothing that types a receiver, so a method reached by name alone
|
||||
* must earn it:
|
||||
*
|
||||
* - `receiver.m()` through an untyped receiver takes the one project method
|
||||
* named `m` only when the receiver is named after its owner
|
||||
* (`web_push_request.legacy_encrypt` → WebPushRequest). rubocop's
|
||||
* `node.loc` — a rubocop-ast node — went to the project's lone `loc` 1,201
|
||||
* times; lobsters' `value.to_s` to a short-id class's `to_s`.
|
||||
* - a bare `m()` inside a class is a call on self: the class's own methods,
|
||||
* its superclasses', and those of modules mixed into any of them. rubocop's
|
||||
* `format(…)` — Kernel's — went to the LSP runtime's `format` 411 times.
|
||||
*/
|
||||
import { describe, it, expect, afterAll, beforeAll } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { CodeGraph } from '../src';
|
||||
|
||||
let root = '';
|
||||
let cg: CodeGraph;
|
||||
|
||||
beforeAll(async () => {
|
||||
root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ruby-scope-'));
|
||||
const files: Record<string, string> = {
|
||||
'lib/app/ext/regexp.rb': `module App
|
||||
module Ext
|
||||
module Expression
|
||||
def loc
|
||||
@loc
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/lsp/runtime.rb': `module App
|
||||
module LSP
|
||||
class Runtime
|
||||
def format(text)
|
||||
text
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/web_push_request.rb': `class WebPushRequest
|
||||
def legacy_encrypt(payload)
|
||||
payload
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/cop/base.rb': `module App
|
||||
module Cop
|
||||
module RangeHelp
|
||||
def range_between(a, b)
|
||||
[a, b]
|
||||
end
|
||||
end
|
||||
|
||||
class Base
|
||||
def add_offense(node)
|
||||
node
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/cop/style/foo.rb': `module App
|
||||
module Cop
|
||||
module Style
|
||||
class Foo < Base
|
||||
include RangeHelp
|
||||
|
||||
MSG = 'Use %<x>s.'
|
||||
|
||||
def on_send(node, web_push_request)
|
||||
selector = node.loc
|
||||
web_push_request.legacy_encrypt(selector)
|
||||
add_offense(node)
|
||||
range_between(1, 2)
|
||||
format(MSG, x: 1)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/greeting.rb': `module Greeting
|
||||
def greet
|
||||
"Hello, " + display_name()
|
||||
end
|
||||
end
|
||||
`,
|
||||
'lib/app/person.rb': `class Person
|
||||
include Greeting
|
||||
|
||||
def display_name
|
||||
'Ada'
|
||||
end
|
||||
end
|
||||
`,
|
||||
};
|
||||
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);
|
||||
}
|
||||
cg = await CodeGraph.init(root, { index: true });
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
cg?.close();
|
||||
if (root) fs.rmSync(root, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
/** `Owner::method` of every call edge out of a file. */
|
||||
function callsFrom(file: string): string[] {
|
||||
const ids = cg.getNodesInFile(file).map((n) => n.id);
|
||||
return cg
|
||||
.getOutgoingEdgesFrom(ids)
|
||||
.filter((e) => e.kind === 'calls')
|
||||
.map((e) => cg.getNode(e.target)!.qualifiedName)
|
||||
.sort();
|
||||
}
|
||||
|
||||
describe('Ruby calls reached by name alone', () => {
|
||||
it('keep the inherited and mixed-in methods, and the receiver named after its class', () => {
|
||||
expect(callsFrom('lib/app/cop/style/foo.rb')).toEqual([
|
||||
'App::Cop::Base::add_offense',
|
||||
'App::Cop::RangeHelp::range_between',
|
||||
'WebPushRequest::legacy_encrypt',
|
||||
]);
|
||||
});
|
||||
|
||||
it('leave a call in a module body alone: a module runs on whatever includes it', () => {
|
||||
expect(callsFrom('lib/app/greeting.rb')).toEqual(['Person::display_name']);
|
||||
});
|
||||
});
|
||||
@@ -1431,6 +1431,70 @@ function dartHeadOf(decl: Node, context: ResolutionContext): { supers: string[];
|
||||
};
|
||||
}
|
||||
|
||||
const RUBY_ANCESTRY = new WeakMap<ResolutionContext, Map<string, Set<string>>>();
|
||||
|
||||
/**
|
||||
* Whether a bare Ruby call written inside a class body can mean `method`: a
|
||||
* receiver-less call is a call on `self`, so it reaches the class's own
|
||||
* methods, its superclasses', and those of the modules any of them
|
||||
* `include`s, `extend`s or `prepend`s — read from source, resolved against
|
||||
* the lexical nesting (`class Foo < Base` inside `module RuboCop::Cop` is
|
||||
* `RuboCop::Cop::Base`). A call in a module body, a block at the top of a
|
||||
* file (a spec, a DSL) or a script is not judged: a module's methods run on
|
||||
* whatever includes it, and a block may be evaluated on anything. rubocop's
|
||||
* `format(…)` — Kernel's — went to the LSP runtime's `format` 411 times.
|
||||
*/
|
||||
function isRubyMethodInScope(method: Node, ref: UnresolvedRef, context: ResolutionContext): boolean {
|
||||
const cut = method.qualifiedName.lastIndexOf('::');
|
||||
if (cut < 0) return true;
|
||||
const here = context
|
||||
.getNodesInFile(ref.filePath)
|
||||
.filter((n) => (n.kind === 'class' || n.kind === 'module') && n.startLine <= ref.line && n.endLine >= ref.line)
|
||||
.sort((a, b) => (a.endLine - a.startLine) - (b.endLine - b.startLine))[0];
|
||||
if (!here || here.kind !== 'class') return true;
|
||||
return rubyAncestry(here.qualifiedName, context).has(method.qualifiedName.slice(0, cut));
|
||||
}
|
||||
|
||||
/** A Ruby class's qualified name, those of its superclasses, and of every module mixed into any of them. */
|
||||
function rubyAncestry(qn: string, context: ResolutionContext): Set<string> {
|
||||
let memo = RUBY_ANCESTRY.get(context);
|
||||
if (!memo) RUBY_ANCESTRY.set(context, (memo = new Map()));
|
||||
const hit = memo.get(qn);
|
||||
if (hit) return hit;
|
||||
const seen = new Set<string>();
|
||||
const queue = [qn];
|
||||
while (queue.length > 0 && seen.size < 60) {
|
||||
const q = queue.shift()!;
|
||||
if (seen.has(q)) continue;
|
||||
seen.add(q);
|
||||
for (const decl of context.getNodesByQualifiedName(q)) {
|
||||
if (decl.language !== 'ruby' || (decl.kind !== 'class' && decl.kind !== 'module')) continue;
|
||||
const lines = context.getFileLines?.(decl.filePath) ?? context.readFile(decl.filePath)?.split(/\r?\n/) ?? [];
|
||||
const outer = q.includes('::') ? q.slice(0, q.lastIndexOf('::')) : '';
|
||||
const sup = /^\s*class\s+[\w:]+\s*<\s*(::)?([A-Z][\w:]*)/.exec(lines[decl.startLine - 1] ?? '');
|
||||
if (sup) queue.push(rubyConstantQn(sup[2]!, sup[1] ? '' : outer, context));
|
||||
for (const line of lines.slice(decl.startLine, decl.endLine)) {
|
||||
const mix = /^\s*(?:include|extend|prepend)\s+([A-Z:][\w:]*(?:\s*,\s*[A-Z:][\w:]*)*)/.exec(line);
|
||||
if (!mix) continue;
|
||||
for (const name of mix[1]!.split(/\s*,\s*/)) {
|
||||
queue.push(name.startsWith('::') ? rubyConstantQn(name.slice(2), '', context) : rubyConstantQn(name, q, context));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
memo.set(qn, seen);
|
||||
return seen;
|
||||
}
|
||||
|
||||
/** The class or module a constant written inside `scope` names: the nearest enclosing namespace that has it, else the name itself. */
|
||||
function rubyConstantQn(name: string, scope: string, context: ResolutionContext): string {
|
||||
for (let prefix = scope; ; prefix = prefix.includes('::') ? prefix.slice(0, prefix.lastIndexOf('::')) : '') {
|
||||
const qn = prefix ? `${prefix}::${name}` : name;
|
||||
if (context.getNodesByQualifiedName(qn).some((n) => n.language === 'ruby' && (n.kind === 'class' || n.kind === 'module'))) return qn;
|
||||
if (!prefix) return name;
|
||||
}
|
||||
}
|
||||
|
||||
const KOTLIN_FILE_SCOPES = new WeakMap<ResolutionContext, Map<string, { pkg: string; imports: Set<string>; stars: Set<string> }>>();
|
||||
/** Packages every Kotlin file imports without writing it. */
|
||||
const KOTLIN_DEFAULT_IMPORTS: ReadonlySet<string> = new Set([
|
||||
@@ -1911,8 +1975,10 @@ export function matchByExactName(
|
||||
const javaBare = ref.language === 'java' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName);
|
||||
const dartBare = ref.language === 'dart' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName) && isReceiverLessDartCall(ref, context);
|
||||
const kotlinCall = ref.language === 'kotlin' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName);
|
||||
const rubyBare = ref.language === 'ruby' && ref.referenceKind === 'calls' && /^[A-Za-z_]\w*[?!]?$/.test(ref.referenceName);
|
||||
const phpSelf = phpSelfReceiver(ref, context);
|
||||
const filtered = sameName.filter((n) =>
|
||||
!(rubyBare && n.kind === 'method' && !isRubyMethodInScope(n, ref, context)) &&
|
||||
!(javaBare && n.kind === 'method' && !isJavaMethodInScope(n, ref, context)) &&
|
||||
!(kotlinCall && !isKotlinTopLevelVisible(n, ref, context)) &&
|
||||
!(dartBare && isDartMember(n) && !isDartMethodInScope(n, ref, context)) &&
|
||||
@@ -3029,6 +3095,7 @@ export function clearNameMatcherMemos(context: ResolutionContext): void {
|
||||
DART_SUPERS.delete(context);
|
||||
DART_HIERARCHIES.delete(context);
|
||||
KOTLIN_FILE_SCOPES.delete(context);
|
||||
RUBY_ANCESTRY.delete(context);
|
||||
LUA_LOCALS.delete(context);
|
||||
PHP_FILE_SCOPES.delete(context);
|
||||
JAVA_STATIC_IMPORTS.delete(context);
|
||||
@@ -4011,8 +4078,15 @@ export function matchMethodCall(
|
||||
targetMethods = kept;
|
||||
}
|
||||
|
||||
// If only one same-language method with this name exists, use it
|
||||
if (targetMethods.length === 1 && !narrowed && targetMethods[0]!.language === ref.language) {
|
||||
// If only one same-language method with this name exists, use it —
|
||||
// except in Ruby, where nothing types a receiver: there the one method
|
||||
// must also belong to something the receiver is named after
|
||||
// (`web_push_request.legacy_encrypt` → WebPushRequest). rubocop's
|
||||
// `node.loc` on a rubocop-ast node went to the one `loc` in the project
|
||||
// 1,201 times; lobsters' `value.to_s` to a short-id class's.
|
||||
if (targetMethods.length === 1 && !narrowed && targetMethods[0]!.language === ref.language &&
|
||||
!(ref.language === 'ruby' && objectOrClass !== 'self' && objectOrClass !== 'self.class' &&
|
||||
!sharesReceiverWord(objectOrClass!, targetMethods[0]!))) {
|
||||
return {
|
||||
original: ref,
|
||||
targetNodeId: targetMethods[0]!.id,
|
||||
@@ -4814,6 +4888,14 @@ function hasParameterBinding(code: string, escapedName: string): boolean {
|
||||
/**
|
||||
* Split a camelCase or PascalCase string into words.
|
||||
*/
|
||||
/** Whether a receiver's name and the owner of `method` have a word in common, case aside. */
|
||||
function sharesReceiverWord(receiver: string, method: Node): boolean {
|
||||
const cut = method.qualifiedName.lastIndexOf('::');
|
||||
if (cut < 0) return false;
|
||||
const owner = new Set(splitCamelCase(method.qualifiedName.slice(0, cut)).map((w) => w.toLowerCase()));
|
||||
return splitCamelCase(receiver).some((w) => owner.has(w.toLowerCase()));
|
||||
}
|
||||
|
||||
function splitCamelCase(str: string): string[] {
|
||||
return str.replace(/([a-z])([A-Z])/g, '$1 $2')
|
||||
.replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2')
|
||||
@@ -4984,6 +5066,7 @@ export function matchFuzzy(
|
||||
const javaBare = ref.language === 'java' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName);
|
||||
const dartBare = ref.language === 'dart' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName) && isReceiverLessDartCall(ref, context);
|
||||
const kotlinCall = ref.language === 'kotlin' && ref.referenceKind === 'calls' && /^[A-Za-z_$][\w$]*$/.test(ref.referenceName);
|
||||
const rubyBare = ref.language === 'ruby' && ref.referenceKind === 'calls' && /^[A-Za-z_]\w*[?!]?$/.test(ref.referenceName);
|
||||
const phpSelf = phpSelfReceiver(ref, context);
|
||||
// Names are case-sensitive in every language but a handful: Rust's
|
||||
// `Bytes` is not the method `bytes`, Python's builtin `dir(…)` not a class
|
||||
@@ -5000,6 +5083,7 @@ export function matchFuzzy(
|
||||
!(javaBare && n.kind === 'method' && !isJavaMethodInScope(n, ref, context)) &&
|
||||
!(dartBare && isDartMember(n) && !isDartMethodInScope(n, ref, context)) &&
|
||||
!(kotlinCall && !isKotlinTopLevelVisible(n, ref, context)) &&
|
||||
!(rubyBare && n.kind === 'method' && !isRubyMethodInScope(n, ref, context)) &&
|
||||
!(phpSelf && (n.kind !== 'method' || !isPhpMethodInScope(n, ref, phpSelf, context))))
|
||||
.filter((n) => (ref.referenceKind !== 'references' && ref.referenceKind !== 'function_ref') ||
|
||||
sameLanguageFamily(n.language, ref.language));
|
||||
|
||||
Reference in New Issue
Block a user