Add Oref analyzer plugin, lint rules, fixes, and assists#40
Conversation
📝 WalkthroughWalkthroughAdds a new Analyzer plugin for oref: introduces a plugin entrypoint, many analyzer rules, fixes, assists, AST utilities (hook detection, scopes, custom-hook discovery, type/package helpers), test harness and tests, CI wiring, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,240,255,0.5)
actor Dev as Developer
participant IDE as IDE/Editor
participant Plugin as AnalyzerPlugin
participant Registry as PluginRegistry
participant Analyzer as Analysis Server
participant Rules as Lint Rules
participant Utils as Analysis Utils
end
Dev->>IDE: Open Dart file / request analysis
IDE->>Plugin: Load/initialize plugin
Plugin->>Registry: registerRules(), registerFixes(), registerAssists()
IDE->>Analyzer: Parse file -> produce AST
Analyzer->>Rules: Invoke node visitors
Rules->>Utils: Query hook detection, scopes, types, custom hooks
Utils-->>Rules: Return HookCall / HookScope / type info
alt Violation detected
Rules->>IDE: Emit diagnostic + suggested fixes/assists
IDE->>Plugin: Request fix/assist edits
Plugin->>IDE: Provide edits from Fix/Assist providers
Dev->>IDE: Apply fix
else No violations
Rules-->>IDE: No diagnostics
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/src/analyzer/utils/custom_hooks.dart`:
- Around line 350-399: Both _librarySignature and
_computeCustomHookNamesForLibrary currently use direct File stat/read calls
which bypass analyzer overlays; replace those disk accesses with overlay-aware
AnalysisSession APIs: use the session (e.g., AnalysisSession.getParsedUnit or
getParsedUnit2) for each fragment's path to obtain the ParsedUnitResult.unit and
file timestamp/contents instead of File(...).statSync()/readAsStringSync(),
ensure the analysis context uses an OverlayResourceProvider (or equivalent) so
getParsedUnit reflects unsaved overlays, and hook into overlay change
notifications to invalidate whatever cache uses
_librarySignature/_computeCustomHookNamesForLibrary (referencing the
_hookImportInfoForLibrary call sites) so cached signatures/names are recomputed
when overlays change.
In `@lib/src/analyzer/utils/hook_calls.dart`:
- Around line 77-113: The matchHookConstructor function may dereference
enclosing.library without checking for null; update matchHookConstructor to
defensively handle a null enclosing.library (from the enclosing Element
retrieved from node.constructorName.element) by returning null if
enclosing.library is null before calling _isOrefLibrary—this ensures the check
using _isOrefLibrary(library.uri) is only performed when library is non-null and
avoids a potential null dereference.
🧹 Nitpick comments (4)
lib/src/analyzer/utils/package_utils.dart (1)
30-50: Synchronous file I/O may block the analysis server.
readAsLinesSync()performs blocking I/O on the analysis server thread. While the caching mitigates repeated reads, the initial read could cause latency. Consider whether asynchronous reading is feasible given the analyzer plugin API constraints.Additionally, a minor optimization: the
RegExp(r'\s')on line 42 is recreated on each invocation. Consider extracting it as a top-level constant.♻️ Suggested optimization
+final _whitespaceRegex = RegExp(r'\s'); + String? _readPackageName(String rootPath) { final pubspec = File('$rootPath${Platform.pathSeparator}pubspec.yaml'); if (!pubspec.existsSync()) { return null; } try { for (final line in pubspec.readAsLinesSync()) { final trimmed = line.trimLeft(); if (trimmed.startsWith('name:')) { return trimmed .substring('name:'.length) .trim() - .split(RegExp(r'\s')) + .split(_whitespaceRegex) .first; } } } on FileSystemException { return null; } return null; }lib/src/analyzer/utils/callback_utils.dart (1)
148-162: Consider documenting the fallback behavior.The fallback on line 161 (
return parent is Expression ? parent : null) is permissive and could return Expression nodes that aren't direct argument containers. While callers perform additional validation, this implicit behavior could be surprising to maintainers.♻️ Consider adding a clarifying comment
if (parent is ParenthesizedExpression && parent.parent is ArgumentList && parent.expression == node) { return parent; } + // Fallback for other expression wrappers; callers validate the full chain return parent is Expression ? parent : null; }lib/src/analyzer/utils/hook_scopes.dart (1)
181-201: Name-based heuristic may produce false positives.The function returns
truefor any identifier namedcontextbefore verifying its type. A variable namedcontextthat isn't aBuildContextwould incorrectly match. The type check on line 200 is the authoritative verification.Consider reordering to prioritize the type check, or document this as intentional for performance optimization.
♻️ Alternative: prioritize type check
bool isBuildContextExpression(Expression? expression) { if (expression == null) { return false; } + // Prefer type-based check when available + if (isBuildContextType(expression.staticType)) { + return true; + } + // Fallback to name heuristic for unresolved types if (expression is SimpleIdentifier) { if (expression.name == 'context') { return true; } } // ... rest unchanged - return isBuildContextType(expression.staticType); + return false; }lib/src/analyzer/rules/avoid_hooks_in_control_flow_rule.dart (1)
50-106: Consider extracting shared hook‑reporting logicThis visitor largely mirrors the nested‑function rule (hook detection + reporting). A small shared helper/base visitor would reduce duplication and drift as rules evolve.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/src/analyzer/utils/custom_hooks.dart`:
- Around line 294-298: Update _normalizeCustomHookElement to use the new
nullable accessor: when element is a PropertyAccessorElement, return
element.variable2 instead of the deprecated element.variable; ensure the
function still returns Element? to accept the nullable variable2 result and keep
the existing fallback return element for non-accessor cases.
In `@lib/src/analyzer/utils/hook_calls.dart`:
- Around line 149-156: The function isOrefFunctionInvocation checks
node.methodName.element but assumes element.library is non-null; add a defensive
null check so that if element is an ExecutableElement but element.library is
null you return false instead of dereferencing it. Update
isOrefFunctionInvocation to verify element.library != null (or obtain uri via a
null-safe check) before calling _isOrefLibrary, keeping the existing
early-return logic and references to node.methodName.element and _isOrefLibrary.
🧹 Nitpick comments (2)
lib/src/analyzer/utils/hook_scopes.dart (1)
175-194: Consider documenting the name-based fallback heuristic.The fallback to checking identifier names (
context) whenstaticTypeis null is pragmatic for unresolved types, but could produce false positives if a variable namedcontexthas a different type.Consider adding a brief doc comment explaining when the fallback is triggered and its limitations:
+/// Returns true if [expression] is a BuildContext expression. +/// +/// Falls back to name heuristics when static type information is unavailable, +/// which may produce false positives for unrelated `context` identifiers. bool isBuildContextExpression(Expression? expression) {lib/src/analyzer/utils/custom_hooks.dart (1)
3-3: Consider bounded cache or periodic cleanup for long-running sessions.The global
_customHookLibraryCachegrows monotonically as libraries are analyzed. While signature-based invalidation prevents stale data, entries for removed/renamed libraries are never evicted. For typical project sizes this is acceptable, but for very long-running analyzer sessions or large monorepos, memory could accumulate.If this becomes an issue in practice, consider an LRU eviction policy or periodic cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@analysis_tests/test/avoid_hooks_in_nested_functions_test.dart`:
- Line 32: The test returns an abstract Widget via the expression "return
Widget();" which fails analysis; replace that instantiation with a concrete
widget (e.g., return SizedBox.shrink(); or return Container(); or return
Text('')); update the return expression in the test (the line with "return
Widget();") to use one of those concrete widget constructors so the test
compiles and analysis passes.
- Line 49: The test currently attempts to instantiate an abstract Flutter type
with "return Widget();", which is invalid; locate the occurrence of the
expression "return Widget();" in
analysis_tests/test/avoid_hooks_in_nested_functions_test.dart and replace it
with a concrete widget instance (for example return const SizedBox.shrink() or
return Container()) so the test returns a concrete Widget implementation instead
of trying to construct the abstract Widget type.
🧹 Nitpick comments (1)
analysis_tests/test/avoid_hooks_in_control_flow_test.dart (1)
1-53: LGTM!The tests correctly verify the rule's behavior for hooks inside conditionals vs. top-level hook calls. This enforces consistent hook ordering, which is essential for reactive state management.
Consider adding tests for other control flow constructs (
for,while,switch,try-catch) to ensure comprehensive coverage of the rule.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/analyzer/utils/hook_scopes.dart`:
- Around line 237-252: hookScopeLabel currently only detects custom hooks when
customHooks is non-null, causing inconsistent labels vs enclosingHookScope;
update hookScopeLabel to apply the same custom‑hook predicate regardless of
registry presence by removing the customHooks null guard and using the registry
methods when customHooks is provided otherwise fall back to the existing
heuristic used by enclosingHookScope (i.e., test parent via
isCustomHookFunctionDeclaration and builderFunction via
isCustomHookFunctionExpression equivalently). Ensure you reference
hookScopeLabel, builderFunction, CustomHookRegistry,
isCustomHookFunctionDeclaration and isCustomHookFunctionExpression when making
the change.
🧹 Nitpick comments (1)
analysis_tests/test/avoid_hooks_in_control_flow_test.dart (1)
20-133: Good test coverage for common control-flow constructs.The test suite covers the primary control-flow cases (if, for, while, switch, try) and includes a positive test for valid top-level usage. The offset calculation via
code.indexOf('signal(')and lint span via'signal'.lengthare consistent across tests.Consider adding tests for additional control-flow edge cases.
For completeness, you may want to add tests for:
do-whileloopselse/catch/finallybranches- Nested control flow (e.g., hook inside
ifinsidefor)These would strengthen confidence that the rule handles all control-flow scenarios.
Example additional test for do-while
void test_reports_hook_in_do_while() async { const code = r''' import 'package:flutter/widgets.dart'; import 'package:oref/oref.dart'; class MyWidget extends StatelessWidget { `@override` Widget build(BuildContext context) { do { signal(context, 0); } while (false); return Widget(); } } '''; final offset = code.indexOf('signal('); await assertDiagnostics(code, [lint(offset, 'signal'.length)]); }
1 participant
Resolves #38
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.