diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa4f107b..20c6e999b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/__tests__/ruby-call-scope.test.ts b/__tests__/ruby-call-scope.test.ts new file mode 100644 index 000000000..e9dab7159 --- /dev/null +++ b/__tests__/ruby-call-scope.test.ts @@ -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 = { + '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 %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']); + }); +}); diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index 7a1d5ecca..c8819f160 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -1431,6 +1431,70 @@ function dartHeadOf(decl: Node, context: ResolutionContext): { supers: string[]; }; } +const RUBY_ANCESTRY = new WeakMap>>(); + +/** + * 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 { + 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(); + 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; stars: Set }>>(); /** Packages every Kotlin file imports without writing it. */ const KOTLIN_DEFAULT_IMPORTS: ReadonlySet = 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));