Implementation of parameter parser#234
Implementation of parameter parser#234Dariaa14 wants to merge 12 commits intoanalysis_server_migrationfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the AnalysisOptionsLoader and LintOptions classes to parse and manage lint configurations from analysis_options.yaml. The review identifies critical issues with the current caching strategy, noting that a global cache will cause incorrect behavior in monorepos and lacks a mechanism for invalidation when configuration files change. Further recommendations include renaming variables for clarity and consistently using the analyzer's ResourceProvider API instead of dart:io to ensure better compatibility and testability.
| if (_rulesCache.isNotEmpty) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The global _rulesCache and the early return when it is not empty will cause incorrect behavior in multi-package workspaces (monorepos). If different packages have different analysis_options.yaml configurations, the plugin will incorrectly apply the configuration of the first package it analyzes to all subsequent packages. The cache should be keyed by the configuration file path or the context root to ensure each package uses its own settings.
There was a problem hiding this comment.
@Dariaa14, I think this one might be relevant as well.
|
|
||
| /// Loads and parses analysis options from a Dart project's YAML file. | ||
| class AnalysisOptionsLoader { | ||
| Map<String, LintOptions> _rulesCache = {}; |
There was a problem hiding this comment.
The current caching mechanism does not support cache invalidation. In a long-running Analysis Server session (e.g., in an IDE), changes to analysis_options.yaml will not be detected, and the plugin will continue using stale configuration until the server is restarted. Consider checking the file's modification timestamp or implementing a file watcher to refresh the cache when the configuration changes.
There was a problem hiding this comment.
@Dariaa14, have you investigated the problem described here? Looks pretty relevant overall, but I'm not sure if changing analysis_options.yaml would restart the analysis server and reload the configuration.
| final directory = context.allUnits.first.file.path; | ||
| _loadRules(directory); |
There was a problem hiding this comment.
The variable name directory is misleading as it holds the path to a file, not a directory. Renaming it to filePath would improve code clarity.
| final directory = context.allUnits.first.file.path; | |
| _loadRules(directory); | |
| final filePath = context.allUnits.first.file.path; | |
| _loadRules(filePath); |
| final startFile = io.File(filePath); | ||
| io.Directory dir = startFile.parent; | ||
|
|
||
| while (true) { | ||
| final candidate = PhysicalResourceProvider.INSTANCE | ||
| .getFile('${dir.path}${Platform.pathSeparator}$fileName'); | ||
|
|
||
| if (candidate.exists) { | ||
| return candidate.path; | ||
| } | ||
|
|
||
| final parent = dir.parent; | ||
|
|
||
| if (parent.path == dir.path) { | ||
| return null; | ||
| } | ||
|
|
||
| dir = parent; | ||
| } |
There was a problem hiding this comment.
Avoid mixing dart:io classes with the analyzer's ResourceProvider abstraction. Using the ResourceProvider API consistently ensures the plugin works correctly in all environments, including tests with MemoryResourceProvider. Additionally, use pathContext.join instead of manual string interpolation for platform-independent path construction.
final pathContext = PhysicalResourceProvider.INSTANCE.pathContext;
var currentDir = pathContext.dirname(filePath);
while (true) {
final candidatePath = pathContext.join(currentDir, fileName);
if (PhysicalResourceProvider.INSTANCE.getFile(candidatePath).exists) {
return candidatePath;
}
final parentDir = pathContext.dirname(currentDir);
if (parentDir == currentDir) {
return null;
}
currentDir = parentDir;
}
solid-vovabeloded
left a comment
solid-vovabeloded
left a comment
There was a problem hiding this comment.
Great start, @Dariaa14! Please, take a look at the following suggestions and let me know what you think.
|
|
||
| /// Loads and parses analysis options from a Dart project's YAML file. | ||
| class AnalysisOptionsLoader { | ||
| Map<String, LintOptions> _rulesCache = {}; |
There was a problem hiding this comment.
@Dariaa14, have you investigated the problem described here? Looks pretty relevant overall, but I'm not sure if changing analysis_options.yaml would restart the analysis server and reload the configuration.
| /// A global instance of [AnalysisOptionsLoader] for use across the plugin. | ||
| final analysisLoader = AnalysisOptionsLoader(); |
There was a problem hiding this comment.
Please, avoid using top-level variables. I believe it would be better to instantiate this class during plugin creation and then inject it wherever we need.
| if (_rulesCache.isNotEmpty) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@Dariaa14, I think this one might be relevant as well.
| final startFile = io.File(filePath); | ||
| io.Directory dir = startFile.parent; | ||
|
|
||
| while (true) { | ||
| final candidate = PhysicalResourceProvider.INSTANCE | ||
| .getFile('${dir.path}${Platform.pathSeparator}$fileName'); | ||
|
|
||
| if (candidate.exists) { | ||
| return candidate.path; | ||
| } | ||
|
|
||
| final parent = dir.parent; | ||
|
|
||
| if (parent.path == dir.path) { | ||
| return null; | ||
| } | ||
|
|
||
| dir = parent; | ||
| } |
| _rulesCache = rules; | ||
| } | ||
|
|
||
| String? _findNearestYamlUpwards( |
There was a problem hiding this comment.
| String? _findNearestYamlUpwards( | |
| String? _findNearestFileUpwards( |
I believe it may be used for searching other files as well.
| final startFile = io.File(filePath); | ||
| io.Directory dir = startFile.parent; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
| while (true) { | |
| while (dir.path != dir.parent.path) { |
Does that make sense?
| } | ||
| } | ||
|
|
||
| return UnmodifiableMapView(rules); |
There was a problem hiding this comment.
If we are using UnmodifiableMapView, then we probably should have _rulesCache of UnmodifiableMapView type.
| Map<String, LintOptions> _rulesCache = {}; | ||
|
|
||
| /// Retrieves the currently loaded lint rules. | ||
| Map<String, LintOptions> get rules => _rulesCache; |
There was a problem hiding this comment.
Would it be great to have a method to get the options for the exact rule name?
| /// Whether the configuration enables/disables the lint rule. | ||
| final bool enabled; | ||
|
|
||
| /// Extra configurations for a [AnalysisRule]. | ||
| final Map<String, Object?> json; |
There was a problem hiding this comment.
Please, declare fields and getters before the constructor.
2 participants
Implementation:
Since the analysis_server_plugin currently lacks a direct way to retrieve the context root (noted by the TODO in the official source code), I explored two strategies for resolving analysis_options.yaml:
Plugin Register: I found this too restrictive, as it only functions correctly when 'dart analyze' is executed from the project root.
Rule Registration with Caching (Chosen): By resolving the configuration at the rule level, we can leverage the RuleContext to identify the file path currently under analysis. To prevent the performance overhead of re-parsing for every file, I implemented a caching layer. The YAML is only parsed once per 'dart analyze'; subsequent files retrieve the configuration from the cache.
Research of other packages:
I researched the approach of others:
custom_lint and dart_code_metrics both utilize a custom server architecture built on the legacy analyzer_plugin package.
I found no existing public implementations for dynamic parameter injection for analysis_server_plugin users.
Configuration Structure
Regarding our previous discussion on YAML structure: I tested the older formats used by other analyzers, but they were not recognized by the current server plugin. I have reverted to the structure specified in the official Dart documentation, which I have verified as functional in this implementation.