diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c97600..8a55a98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Schema-aware diagnostics were missing in files open at startup.** Every + document open when VS Code starts is linted before the first workspace scan + finishes, and nothing re-linted it afterwards, so `DOL008` (misspelled + lookup) and the schema gates of `DOL007` stayed silent until the file was + edited. The same gap meant a `models.py` change never reached diagnostics in + views that were already open. Open documents are now re-linted whenever the + index is replaced. + +- **`DOL007` skipped loops over common querysets.** The loop-head pattern only + accepted flat argument lists and no call after the first, so + `for o in Order.objects.filter(created__lte=timezone.now()):` and + `for o in Order.objects.filter(...).order_by("-id"):` were never inspected. + The head is now read with balanced parentheses (string literals skipped) + across the whole call chain, and a `select_related(...)` anywhere in that + chain still silences the finding. + ## [0.19.0] - 2026-09-21 ### Added diff --git a/src/codeActionProvider.ts b/src/codeActionProvider.ts index 6f25167..7307df9 100644 --- a/src/codeActionProvider.ts +++ b/src/codeActionProvider.ts @@ -182,6 +182,11 @@ export function registerCodeFixes( // lens providers take it: the index is replaced wholesale on every scan, and // a captured value would go stale the first time a models.py is saved. getIndex?: () => WorkspaceIndex, + // Schema-aware rules (DOL007's gates, DOL008) are silent without an index, + // and documents open at startup are linted before the first scan finishes. + // Without a re-lint here they stay silent until the user types in them, and + // a models.py edit never reaches diagnostics in the views already open. + onIndexChanged?: vscode.Event, ): vscode.Disposable[] { const collection = vscode.languages.createDiagnosticCollection('djangoOrmLens'); const provider = new DjangoCodeFixesProvider(collection, getIndex); @@ -217,9 +222,13 @@ export function registerCodeFixes( }), ]; - for (const doc of vscode.workspace.textDocuments) { - refreshDiagnosticsForDoc(doc, collection, getIndex); - } + const refreshAll = (): void => { + for (const doc of vscode.workspace.textDocuments) { + refreshDiagnosticsForDoc(doc, collection, getIndex); + } + }; + if (onIndexChanged) disposables.push(onIndexChanged(refreshAll)); + refreshAll(); return disposables; } diff --git a/src/extension.ts b/src/extension.ts index 3cae244..33adeee 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -27,6 +27,7 @@ import { completionsAt, lookupPathLength } from './ormCompletions'; const LOOKBACK_LINES = 200; let currentIndex: WorkspaceIndex = { apps: [], scannedAt: 0 }; +const indexChanged = new vscode.EventEmitter(); let treeProvider: DjangoTreeProvider; let codeLensProvider: DjangoCodeLensProvider; let statusItem: vscode.StatusBarItem; @@ -58,6 +59,7 @@ async function doScan(): Promise { currentIndex = next; treeProvider.setIndex(currentIndex); codeLensProvider?.refresh(); + indexChanged.fire(); const total = currentIndex.apps.reduce((n, a) => n + a.models.length, 0); const took = Date.now() - before; statusItem.text = `$(database) ${total} model${total === 1 ? '' : 's'}`; @@ -323,7 +325,8 @@ export async function activate(context: vscode.ExtensionContext) { // `.count() > 0` -> `.exists()`, `.first() is None` -> `not .exists()`, // and N+1 attribute-access-in-loop diagnostics. Feature-flagged via // djangoOrmLens.codeFixes.enabled (default true). - for (const d of registerCodeFixes(context, () => currentIndex)) { + context.subscriptions.push(indexChanged); + for (const d of registerCodeFixes(context, () => currentIndex, indexChanged.event)) { context.subscriptions.push(d); } diff --git a/src/rules/queryset.ts b/src/rules/queryset.ts index c360969..305ba28 100644 --- a/src/rules/queryset.ts +++ b/src/rules/queryset.ts @@ -33,8 +33,7 @@ const RE_FILTER_EXCLUDE_CHAIN = /(\b[A-Za-z_][\w.]*(?:\([^()]*\))*)\.filter\(([^()]*)\)\s*\.exclude\(([^()]*)\)/g; const RE_LIST_OF_QS_IN_FOR = /^\s*for\s+[A-Za-z_]\w*\s+in\s+list\(([A-Za-z_][\w.]*(?:\([^()]*\))*)\)\s*:/; -const RE_FOR_LOOP_HEAD = - /^\s*for\s+([A-Za-z_]\w*)\s+in\s+([A-Za-z_][\w.]*(?:\([^()]*\))*(?:\.all\(\))?)\s*:/; +const RE_FOR_LOOP_START = /^\s*for\s+([A-Za-z_]\w*)\s+in\s+/; const LOOP_VAR_ATTR_RE = /\b([A-Za-z_]\w*)\.([A-Za-z_]\w*)\b/g; const MAX_LOOP_LINES = 15; @@ -90,6 +89,52 @@ function isCommentLine(text: string): boolean { * says nothing about what comes back and a line-oriented rule cannot * follow a callee's return. */ +/** Index of the `)` closing the `(` at `open`, skipping string literals; -1 if unclosed. */ +function closingParen(text: string, open: number): number { + let depth = 0; + let quote: string | null = null; + for (let i = open; i < text.length; i++) { + const c = text[i]; + if (quote) { + if (c === '\\') i++; + else if (c === quote) quote = null; + } else if (c === '"' || c === "'") { + quote = c; + } else if (c === '(') { + depth++; + } else if (c === ')' && --depth === 0) { + return i; + } + } + return -1; +} + +/** + * `for in :` where `` is a dotted chain whose calls may + * nest parentheses and follow one another: + * `Order.objects.filter(created__lte=now()).order_by("-id")`. + */ +function parseLoopHead(line: string): { loopVar: string; source: string } | null { + const head = RE_FOR_LOOP_START.exec(line); + if (!head) return null; + const start = head[0].length; + let i = start; + for (;;) { + const name = /^[A-Za-z_]\w*/.exec(line.slice(i)); + if (!name) return null; + i += name[0].length; + while (line[i] === '(') { + const end = closingParen(line, i); + if (end < 0) return null; + i = end + 1; + } + if (line[i] !== '.') break; + i++; + } + if (!/^\s*:/.test(line.slice(i))) return null; + return { loopVar: head[1], source: line.slice(start, i) }; +} + function mayIterateQuerySet(expr: string): boolean { const callAt = expr.indexOf('('); if (callAt === -1) return true; @@ -360,10 +405,10 @@ const DOL007: Rule = { check(ctx: RuleContext): Finding[] { const out: Finding[] = []; for (let i = 0; i < ctx.lineCount; i++) { - const head = RE_FOR_LOOP_HEAD.exec(ctx.lineAt(i)); + const head = parseLoopHead(ctx.lineAt(i)); if (!head) continue; - const loopVar = head[1]; - const qsExpr = head[2]; + const loopVar = head.loopVar; + const qsExpr = head.source; if (!mayIterateQuerySet(qsExpr)) continue; // The chain that produced the loop's source is usually on an earlier // line (`codes = list(Model.objects.select_related("x"))`), so both the diff --git a/test/rules/n-plus-one.test.js b/test/rules/n-plus-one.test.js index 265cfae..b85b44c 100644 --- a/test/rules/n-plus-one.test.js +++ b/test/rules/n-plus-one.test.js @@ -243,3 +243,40 @@ test('DOL007 does not follow a binding across a scope boundary', () => { assert.equal(findings.length, 1); assert.equal(findings[0].fixHint, 'user'); }); + +test('DOL007 reads a loop head whose filter nests a call', () => { + // `created__lte=timezone.now()` put a paren pair inside the call, and the + // loop-head pattern only allowed flat argument lists, so the most common + // shape of a date-bounded report loop was never looked at. + const rule = ruleByCode('DOL007'); + const src = [ + 'def report():', + ' for m in Membership.objects.filter(created__lte=timezone.now()):', + ' print(m.roles, m.user.email)', + ].join('\n'); + const findings = rule.check(makeCtx(src, REPORTER_INDEX)); + assert.equal(findings.length, 1); + assert.equal(findings[0].fixHint, 'user'); +}); + +test('DOL007 reads a loop head that chains calls after the first one', () => { + const rule = ruleByCode('DOL007'); + const src = [ + 'def report():', + ' for m in Membership.objects.filter(roles="admin").order_by("-id"):', + ' print(m.user)', + ].join('\n'); + const findings = rule.check(makeCtx(src, REPORTER_INDEX)); + assert.equal(findings.length, 1); + assert.equal(findings[0].fixHint, 'user'); +}); + +test('DOL007 honours select_related later in a chained loop head', () => { + const rule = ruleByCode('DOL007'); + const src = [ + 'def report():', + ' for m in Membership.objects.filter(roles="a)b").select_related("user"):', + ' print(m.user)', + ].join('\n'); + assert.equal(rule.check(makeCtx(src, REPORTER_INDEX)).length, 0); +});