Consider simplifying package usage

[Clavum]

Issue:
The current PluginBase class requires overriding the getLints method and returning a Stream<Lint> without providing guidance or structure on how all the custom lints are to be provided. Users of this package must design their own way to go from a ResolvedUnitResult to yielding Lints.

To be more clear, when creating a single custom lint, the current implementation works fine (like your final providers lint in the example folder). The issue happens when trying to define multiple lints. How do we make lints easy to add/remove? How do we avoid getLints from becoming a giant method full of different lints? How do we decouple lints but also let them share an AstVisitor to speed up processing?

Essentially:

1. Lints aren't easily sharable because everyone will implement them differently
2. It's hard to get started writing custom lints because you don't just write the lint, you have to connect it with the PluginBase too
3. Related to point 2, there's more code to test as well, as users (who like to test their code, at least) must test their PluginBase too.

Solution:
Expect users to supply lint rules instead of a PluginBase.

I anticipate showing code will be the best way to explain this.

This code would be added to the custom_lint package (it's just an example, doesn't need to be done this way).

import 'dart:async';

import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/error/error.dart';
import 'package:analyzer/error/listener.dart';
import 'package:analyzer/source/line_info.dart';

//ignore: implementation_imports
import 'package:analyzer/src/lint/linter_visitor.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:example_lints/src/rules/example_rule.dart';

final List<ExampleLintRule> rules = [
  ExampleRule(),
];

// In this example, I extend PluginBase, but if you use my idea, the PluginBase can be modified
// itself instead.
//
// This class implements AnalysisErrorListener from the analyzer package so that it's onError
// method will be called when each rule reports a lint.
class ExamplePluginBase extends PluginBase implements AnalysisErrorListener {
  late StreamController<Lint> _streamController;

  @override
  Stream<Lint> getLints(ResolvedUnitResult resolvedUnitResult) {
    _streamController = StreamController<Lint>();

    // For the stream to work, it must work specifically in this order:
    // 1. Be created
    // 2. Be listened to (by being returned from this method)
    // 3. Have lints added
    // 4. Be closed
    //
    // If any of these steps are out of order, the stream will not work
    // correctly and lints will never be reported.
    //
    // Thus, the method which does to processing is done in an unawaited future
    // so that step 2 can happen before 3 or 4.
    unawaited(process(resolvedUnitResult));

    return _streamController.stream;
  }

  Future<void> process(ResolvedUnitResult resolvedUnitResult) async {
    // To speed up analysis, a single visitor is created, which will find each
    // AST node and send it to any registration from the registry.
    final registry = NodeLintRegistry(false);
    final visitor = LinterVisitor(registry);

    // When an ExampleLintRule finds a lint error, this reporter will send it
    // to the onError method below.
    final reporter = ErrorReporter(
      this,
      resolvedUnitResult.libraryElement.source,
      isNonNullableByDefault: true,
    );

    for (final rule in rules) {
      rule.reporter = reporter;
      rule.registerNodeVisitors(registry, resolvedUnitResult);
    }

    // Visit the AST tree.
    resolvedUnitResult.unit.visitChildren(visitor);

    // We have to be sure to close the stream, or the stream will be listened to indefinitely!
    _streamController.close();
  }

  // Any time a rule reports an error, we convert it to a Lint and send to the stream.
  @override
  void onError(AnalysisError error) {
    LineInfo lineInfo = LineInfo.fromContent(error.source.contents.data);

    final startLocation = lineInfo.getLocation(error.offset);
    final endLocation = lineInfo.getLocation(error.offset + error.length);

    _streamController.add(Lint(
      code: error.errorCode.name,
      message: error.message,
      location: LintLocation(
        offset: error.offset,
        length: error.length,
        startLine: startLocation.lineNumber,
        startColumn: startLocation.columnNumber,
        endLine: endLocation.lineNumber,
        endColumn: endLocation.columnNumber,
        filePath: error.source.uri.path,
      ),
    ));
  }
}

// Here we make our own version of a LintRule from the analyzer package.
abstract class ExampleLintRule extends LintRule {
  final LintSeverity severity;
  // I'm not sure if we need the group, or if we can just always make it Group.style.
  final Group lintGroup;
  final String message;
  final String code;
  final String? url;

  ExampleLintRule({
    required this.severity,
    required this.lintGroup,
    required this.message,
    required this.code,
    this.url,
  }) : super(
          name: code,
          group: lintGroup,
          description: message,
          details: 'No details',
        );

  // The original `registerNodeProcessors` method of the LintRule class expects to be given
  // a LinterContext, which we can't provide, so we make this method instead.
  void registerNodeVisitors(
      NodeLintRegistry registry, ResolvedUnitResult resolvedUnitResult);
}

Then for a user to define a lint rule, it's just a matter of defining an ExampleLintRule.

Here's the rewritten version of the final provider rule in the example folder.

bool _isProvider(DartType type) {
  final element = type.element! as ClassElement;
  final source = element.librarySource.uri;

  final isProviderBase = source.scheme == 'package' &&
      source.pathSegments.first == 'riverpod' &&
      element.name == 'ProviderBase';

  return isProviderBase || element.allSupertypes.any(_isProvider);
}

class FinalProviders extends ExampleLintRule {
  FinalProviders()
      : super(
          code: 'riverpod_final_provider',
          severity: LintSeverity.warning,
          message: r'Providers should always be declared as final',
        );

  @override
  void registerNodeVisitors(
      NodeLintRegistry registry, ResolvedUnitResult resolvedUnitResult) {
    var visitor = _Visitor(this);
    registry.addVariableDeclaration(this, visitor);
  }
}

class _Visitor extends SimpleAstVisitor<void> {
  final LintRule rule;

  _Visitor(this.rule);

  @override
  void visitVariableDeclaration(VariableDeclaration node) {
    final element = node.declaredElement2;
    if (element != null && !element.isFinal && _isProvider(element.type)) {
      rule.reportLint(node);
    }
  }
}

The benefits of this are:

1. Rules are sharable as they share a common interface
2. They are easier to write because instead of using resolvedUnitResult.libraryElement.topLevelElements.where(...).where(...), we visit variable declarations and use an if statement
3. It reuses existing code from the analyzer package
4. Relating to point 3, these rules are also written extremely similarly to the rules from the dart linter itself, making them potentially swappable in the future, or at least makes it easier to create a rule based of a dart lint.
5. It speeds up the processing of lint rules (if you have multiple) because we use a NodeLintRegistry so that PluginBase can create a single AstVisitor instead of each rule visiting the tree itself (they use a Visitor, but, it doesn't actually do any visiting...)

My code is a proof of concept and doesn't have to be the route you go, of course. You may use any, none, or all of it.

Conclusion:
The interaction with the custom_lint package can be greatly simplified by allowing users to define rules instead of a PluginBase.

In fact, with this approach, you could modify the startPlugin method to require a list of rules instead of a PluginBase. The startPlugin method would create the PluginBase itself and give it the rules (if a PluginBase is even required anymore).

final rules = [
  FinalProviders(),
  AnotherExampleLintRule(),
  //ARuleThatIsEasilyTemporarilyDisabledByCommentingItOut(),
  // This could allow for people to share their lints by publishing packages with lint rules! Like:
  ARuleFromSomebodyElsesPackage(),
];

void main(List<String> args, SendPort sendPort) {
  startPlugin(sendPort, rules);
}

Also, this could possibly solve #50, or at least get closer.

Depending on the implementation, this doesn't have to be a breaking change.

[rrousselGit]

Hello!

I'm not against such a project. But it likely shouldn't be within custom_lint instead. It probably would be in a separate package (possibly official).
Doing that involves creating a new framework on top of the current mechanism. That's doable, but not within the scope of the current project.

For now, I'd like to focus on nailing the user experience/performance. With the current mechanism, you should be able to kind of do whatever you want.

[Clavum]

Hi.

That was my guess as to what your answer would be, but if I may add some more arguments to the contrary...

My understanding of the goal of the custom_lint package is to make the developer experience for making lints much easier than using the analyzer_plugin. Indeed, the about section of the readme includes the phrases "a better developer experience", and "a simplified project setup" in comparison to analyzer_plugin. It's along these same lines that I believe a project like this would be the responsibility of this package, whatever it's form.

I don't think a framework from a separate package would be adopted remotely as much as if it was from this package. Additionally, it's better to change it sooner than later to reduce user migration efforts.

A package with great performance and functionality misses out on potential if it's not easy to use. After all, this is the problem with analyzer_plugin you are solving.

It's up to you. If you are still in favor of working on other things, wouldn't mind at all if you want to close this issue. I wrote this code for my own use and thought it wouldn't hurt to share.

[rrousselGit]

Hence why I mentioned that the side package could be official.
If the side package is presented and used in custom_lint's doc, it'd likely get a decent usage.

I understand your view. I just don't think that the value is high enough to justify doing this over other things (especially when considering I work on many other packages).

I believe the current getLints is already decently simple. I already gave some care about simplifying that API (such as with exposing the Lint class and utils to compose a LintLocation).

We can most likely go further, yes. But there are some much better things to do, like working on an IDE extension to give superpowers to custom_Lint.
That's something I'm working on right now. It enables things like showing "analyzing" in the bottom bar of VScode, or showing prints & exceptions in the "output" tab, or even supporting an "attach to plugin process" for debugging.

[Clavum]

Alright, makes sense; that's some cool stuff! Thanks for the consideration and for making such awesome packages.

[rrousselGit]

For reference, I personally use some utilities that allow a Visitor<T> to return a List<T>/Stream<T>. Is that something you'd be interested in?

For example I typically do:

class RiverpodLint extends PluginBase {
  @override
  Stream<Lint> getLints(ResolvedUnitResult unit) async* {
    final visitor = RiverpodVisitor(unit);
    yield* unit.unit.accept(visitor) ?? const Stream<Lint>.empty();
  }
}

class RiverpodVisitor extends CombiningRecursiveVisitor<Lint> {
  @override
  Iterable<Lint>? visitExpression(Expression node) sync* {
    if (something) {
      yield Lint(...);
    }
  }
}

[rrousselGit]

The benefit of using streams is that this both:

• waits for asynchronous logic (such that getLints correctly waits for everything to complete)
• handles errors
• supports hot-reload

And in the future, custom_lint could support canceling the previous getLints stream if the file is changed while getLints is running (because streams have a built-in cancel mechanism)

[Clavum]

I don't disagree with the value of a stream. I'm not suggesting to remove it and my code above still has a stream too.

With your CombiningRecursiveVisitor example, how easy is to have more than one lint rule? How about 10, or 50? Would they share a visitor, in which case there's a lot of code in one place? Would you use multiple visitors, which is unoptimized because each one will visit the entire AST? How easy is it to temporarily remove a lint? With my code I shared, none of this is a problem because the plugin explicitly deals with a list of lint rules.

Would you be able to define your rules codes (riverpod_final_providers), their severity, and their message in one place? What if your rule is interested in multiple node types and thus has several yields associated with it? My code has it declared as the parameters of a class so it is accessible dynamically.

If someone else created a lint, how much effort would it take to add it to your own plugin base? With there being no framework to defining lints with this package, that other person may write it substantially different and it's not compatible with your own implementation.

Sure, a CombiningRecursiveVisitor could be one way of doing this. So is my version. There's lots of ways. When a developer uses your package for the first time, do you really want to make them make this decision for themselves before they can start writing lints? Any code they create they would need to test as well, assuming they follow that standard. My code version allows somebody to start writing lints immediately, and their lint rule logic is the only code to test.

Lastly, at the moment, there aren't very many examples of using your package. The final providers in the example folder was good start for the basics, but doesn't show you how to do anything more than check the features of a top level element. My code uses the exact same classes as Dart's lints (LintRule, NodeLintRegistry, SimpleAstVisitor, edit: LinterVisitor too) and they are written the same way too. This means every single dart lint that developers are familiar with can now be an example of writing your own lint. In fact, you can even copy-paste them into your custom lint plugin, assuming they don't import any dart linter internal code.
Dart lint rules code

[rrousselGit]

I'll look more into how the SDK writes its lint. If I like it, forking their approach could be reasonable.

[rrousselGit]

Honestly, I'm not convinced that the SDK approach is inherently faster. Technically having multiple visitors shouldn't be an issue. The amount of iteration done is the same (since the SDK does a loop on all lints on all methods of the visitor)

But I can see a value elsewhere.
Their approach could allow custom_lint to support enabling/disabling rules in the analysis_options.yaml with ease. That in itself is probably worth it

#50 is an added bonus

I'll see what I can do.

[Clavum]

Cool, thanks for looking into it.

Regardless of the implementation of the visitors, each lint rule will indeed run the same amount of code (inspecting an element to see if it breaks the rule). The increase in cost of multiple visitors is in the visiting itself.

Lets say we have 10 lint rules, and our project has a single file with 100 AST nodes within it. If you use a single visitor, and each rule registers its processing with a NodeLintRegistry, then the shared visitor will visit each of the 100 nodes a single time. If each rule creates it's own visitor (which on it's own is accepted), each node will be visited 10 times, for 1000 visits in total.

This is assuming, though, that the visitors are fully recursive. The shared visitor needs to be, since it doesn't know what its rules are interested in. An individual visitor could possibly visit a smaller portion of elements, but in practice most rules can't and don't do this optimization, because they are interested in lower-level nodes that can be anywhere.

It is misleading that each rule (in my example and in the SDK) creates a "_Visitor" class, but this class is never accepted and instead is used merely for the reusability of having a method for each of the ≈200 different node types.

[rrousselGit]

Coming back to this as I'm working on designing the new API right now (to support configuration/disabling rules)

What do you think about an API similar to:

// entrypoint of the plugin

PluginBase createPlugin() => RiverpodAnalysisPlugin();

class RiverpodAnalysisPlugin extends PluginBase {
  List<LintRule> getLintRules() => [
    PreferFinalProvider(),
     ...
  ];

  List<Assist> getAssists() => [
    ConvertToConsumerWidget(),
    ConvertToConsumerStatefulWidget(),
   ...
   ];
}

class PreferFinalProvider extends DartLintRule {
  // Get the lints for a dart file
  Stream<Lint> run(ResolvedUnitResult unit, LintOptions lintOptions) {
    // "run" wouldn't be invoked at all if the lint rule is disabled in the analysis_options.yaml or by ignore_for_file

    // LintOptions would include a JSON map of options resolved from the analysis_options file.

    // TODO return some lints. I have yet to work on potentially changing this part
    // We could fork the ErrorReporter class from the SDK.
    // Same goes for the NodeLintRegistry logic & other visitor utils if we want them.
  }
  
  <possibly move Lint.getFixes to LintRule.getFixes>
}

class ConvertToConsumerWidget extends Assist {
   Stream<Assist> run(ResolvedUnitResult unit, AssistOptions assistOptions, {required int offset, required int length}) {
     // Same thing but for refactorings
   }
}

This seems like a significant organization improvement, on top of natively supporting disabling rules and #50

Would that satisfy your simplification needs?

---

About visitors

It's more of a performance issue than a simplification issue (when this issue is about simplicity) but:
We could alternatively have those run methods return a visitor instead of a Stream<Lint>. Although I'm still not convinced that there's a meaningful performance gain by merging visitors. Afaik merging visitors is equivalent of going from:

for (final a in listA) {
  for (final b in listB) {
    a.doSomething(b);
  }
}

to:

for (final b in listB) {
  for (final a in listA) {
    a.doSomething(b);
  }
}

If it was simple to merge the visitors, I'd do it no questions asked.

But it seems to come with a certain complexity because we now need to register every visitX functions we use.
I can already see folks overriding visitSomething on their visitor but forget to call registerSomethingVisitor(visitor) and wonder why their visitSomething method is never called.
It also makes awaiting any possible asynchronous logic inside the visitor much harder. And the error handling is harder too.

And if someone truly cares about this small performance gain (if it exists), there's an alternate path: not use RecursiveVisitror and only visit the branches that you care about instead of visiting the whole tree.
I originally thought the SDK's NodeLintRegisty did that (as in don't visit some nodes if there's no listener for it), but it doesn't.

Is that fair?

[rrousselGit]

By the way, I want to thank you for the time you spent on this issue.

It's rare that someone spends as much time & effort as you did. So thank you again for that :)

[Clavum]

No problem, I'm happy to contribute!

This seems like a great improvement so far, especially since it lets the plugin know what rules there are ahead of time as you've mentioned.

possibly move Lint.getFixes to LintRule.getFixes

A change here makes sense to me; then you don't have to refer to the same getFixes function from every yield.

I really like the way the SDK does this. Their "Fixes" are generic classes that are agnostic to which rule they are used by. Then they provide a map of each rule and the fixes relevant to them. For example, the AddAsync fix can add async from any place in the code, so that multiple lint rules can use it. I see this being complicated for package users though.

I can already see folks overriding visitSomething on their visitor but forget to...

Yep, agree with you there! Perhaps instead of a single run method, you could make a lint rules class that has an overrideable method for each visitSomeElement type. You could then have the run method provided ahead of time and it would register any provided methods. Not sure about the details there, but my point is that "merging visitors" being complicated is not an issue if it's done in a single place in the package.

It also makes awaiting any possible asynchronous logic inside the visitor much harder. And the error handling is harder too.

I can't say I understand the concern here yet.

there's an alternate path: not use RecursiveVisitror and only visit the branches that you care about instead of visiting the whole tree.

Like I had said before, it's rather rare to be able to avoid full recursion. If you have a lint interested in AdjacentStrings, they can be anywhere in the code and so it needs to be recursive. It is like this for pretty much everything but top level elements.

Again, great progress!

[rrousselGit]

Yep, agree with you there! Perhaps instead of a single run method, you could make a lint rules class that has an overrideable method for each visitSomeElement type. You could then have the run method provided ahead of time and it would register any provided methods. Not sure about the details there, but my point is that "merging visitors" being complicated is not an issue if it's done in a single place in the package.

How would this be any different from having the LintRule class be/return an AstVisitor?

My point is that "merging visitors" being complicated is not an issue if it's done in a single place in the package.

My concern is in not increasing the complexity for users, for what I currently believe to be a minor performance improvement (possibly even debatable)

I can't say I understand the concern here yet.

custom_lint needs to know when a plugin has finished emitting lints. That matters for things like the command line

[Clavum]

How would this be any different from having the LintRule class be/return an AstVisitor?

I suppose you're right; it's the same is if you extended a visitor.

My concern is in not increasing the complexity for users, for what I currently believe to be a minor performance improvement (possibly even debatable)

Maybe I'll try to come up with a more concrete example showing the improvement. In any case, this is a separate issue for the most part and we can ignore it for now.

custom_lint needs to know when a plugin has finished emitting lints. That matters for things like the command line

I don't understand how this could get broken with different code organization. The plugin is done emitting lints when the Stream is closed. That won't and can't change.

[Clavum]

Maybe I'll try to come up with a more concrete example showing the improvement. In any case, this is a separate issue for the most part and we can ignore it for now.

I can try this now...

Firstly, to make sure we're on the same page about what sharing a visitor means...

In either case, we have our predefined lint rule list, which can be any size:

final rules = <LintRule>[
  // lint rule #1
  // lint rule #2
  // ...
];

Sharing a visitor
Sharing a visitor means visitChildren happens once:

// Create a single visitor, which has each rule register in its NodeLintRegistry
final registry = NodeLintRegistry(false);
final visitor = LinterVisitor(registry);
for (final rule in rules) {
  // Registry done here:
  rule.registerNodeVisitors(registry, resolvedUnitResult);
}
// Visit the AST tree. The visitor recurs over every element once. Because it's a [LinterVisitor],
// it calls each registered callback from each rule for each node it finds.
resolvedUnitResult.unit.visitChildren(visitor);

Individual visitors:
Individual visitors means visitChildren happens multiple times:

for (final rule in rules) {
  rule.doYourLintingByYourself(resolvedUnitResult);
}

// With the LintRule class:
void doYourLintingByYourself(ResolvedUnitResult resolvedUnitResult) {
  resolvedUnitResult.unit.visitChildren(someVisitorForThisRule);
}

The first approach adds some overhead from registering callbacks in the registry, and them calling them. The registry works using lists, so callbacks are only attempted to be called if they have been created. This means this overhead has a time complexity of O(2c) where c is the number of registered callbacks. I know, officially, that would be simplified to O(c), but I want to avoid simplifying. This is a very minor overhead since the number of callbacks is pretty small.

No matter the approach, the same amount of time will be spent for each rule to do it's computing (whether a node breaks its rules). We will call the number of rules r. We have a time complexity of O(r).

If we share a visitor, the recursive AST visiting will happen once, so each node is visited once. If the number of nodes in the code is n, we have a time complexity of O(n).

If we use different visitors, each will recursively visit the tree on its own -- it will look at each element and it's children. This is a time complexity of O(rn). Since it looks like the letter m when written together :P I'll clarify that's r (# of rules) times n (# of nodes). This is ignoring possible optimizations of a rule being able to visit a smaller part of the tree, but as I've mentioned before, this optimization is unlikely to happen for most rules.

Summary.
c = number of registered "visitSomeElement" callbacks (small number)
r = number of lint rules (small number)
n = number of AST nodes in the code (very large number)

Shared visitor: O(n + r + 2c)
Individual visitor: O(rn + r)

"r * n" is significantly expensive, adding much more work per lint rule added, especially since n is expected to be a very large number.

This is my first time using big O notation outside of college, so it's possible I used it entirely wrong lol.

[rrousselGit]

Thanks again for the time spent on this!

My concern is that the cost of every visitX method on the single AstVisitor is higher because they now have an added for loop. So while we are involving the visitX method fewer times, its execution is slower

But anyway, considering all the time you took to promote this approach, I figured I might as well write a benchmark.

Shared visitor benchmark:

// Import BenchmarkBase class.
import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/src/lint/linter_visitor.dart';
import 'package:analyzer/src/lint/linter.dart';
import 'package:benchmark_harness/benchmark_harness.dart';

void main() {
  SharedVisitorBenchmark().report();
}

// Create a new benchmark by extending BenchmarkBase
class SharedVisitorBenchmark extends BenchmarkBase {
  SharedVisitorBenchmark() : super('Shared visitors');

  late ParseStringResult result;
  late NodeLintRegistry registry;

  // The benchmark code.
  @override
  void run() {
    result.unit.accept(SharedVisitor(registry));
  }

  // Not measured setup code executed prior to the benchmark runs.
  @override
  void setup() {
    registry = NodeLintRegistry(false);
    registry.addMethodDeclaration(
      Linter1(
        name: 'linter1',
        group: Group('name'),
        description: '',
        details: 'details',
      ),
      Linter1Impl(),
    );
    registry.addExpressionStatement(
      Linter2(
        name: 'linter1',
        group: Group('name'),
        description: '',
        details: 'details',
      ),
      Linter2Impl(),
    );

    result = parseFile(
      path:
          '/Users/remirousselet/dev/invertase/custom_lint/packages/custom_lint/lib/custom_lint.dart',
      featureSet: FeatureSet.latestLanguageVersion(),
    );
  }
}

class SharedVisitor extends LinterVisitor {
  SharedVisitor(super.registry);
}

class Linter1 extends LintRule {
  Linter1({
    required super.name,
    required super.group,
    required super.description,
    required super.details,
  });
}

class Linter1Impl extends SimpleAstVisitor<void> {
  @override
  void visitMethodDeclaration(MethodDeclaration node) {
    doSomething(node);
  }
}

class Linter2 extends LintRule {
  Linter2({
    required super.name,
    required super.group,
    required super.description,
    required super.details,
  });
}

class Linter2Impl extends SimpleAstVisitor<void> {
  @override
  void visitExpressionStatement(ExpressionStatement node) {
    doSomething2(node);
    super.visitExpressionStatement(node);
  }
}

void doSomething(MethodDeclaration node) {}

void doSomething2(ExpressionStatement node) {}

One visitor per lint benchmark:

// Import BenchmarkBase class.
import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/src/lint/linter_visitor.dart';
import 'package:benchmark_harness/benchmark_harness.dart';

void main() {
  MultipleVisitorBenchmark().report();
}

// Create a new benchmark by extending BenchmarkBase
class MultipleVisitorBenchmark extends BenchmarkBase {
  MultipleVisitorBenchmark() : super('Multiple visitors');

  late ParseStringResult result;

  // The benchmark code.
  @override
  void run() {
    result.unit.accept(Visitor1());
    result.unit.accept(Visitor2());
  }

  // Not measured setup code executed prior to the benchmark runs.
  @override
  void setup() {
    result = parseFile(
      path:
          '/Users/remirousselet/dev/invertase/custom_lint/packages/custom_lint/lib/custom_lint.dart',
      featureSet: FeatureSet.latestLanguageVersion(),
    );
  }
}

class Visitor1 extends RecursiveAstVisitor<void> {
  @override
  void visitMethodDeclaration(MethodDeclaration node) {
    doSomething(node);
    super.visitMethodDeclaration(node);
  }
}

class Visitor2 extends RecursiveAstVisitor<void> {
  @override
  void visitExpressionStatement(ExpressionStatement node) {
    doSomething2(node);
    super.visitExpressionStatement(node);
  }
}

void doSomething(MethodDeclaration node) {}

void doSomething2(ExpressionStatement node) {}

From my tests, the shared visitor is on average 10% slower than having one visitor per lint as I thought
Of course, I'm not exactly used to writing benchmarks, so I could've missed something. Please review them carefully :)

[Clavum]

I ran your tests, and I do see a ≈7 microsecond increase with the shared visitor. I believe this is the O(2c) overhead that I mentioned before.

The problem with your perf test is you only have two rules, which doesn't allow the O(rn) complexity of the multiple visitors to show itself. I ran your same tests, but with more lint rules:

Perf test code

// Import BenchmarkBase class.
import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/src/lint/linter.dart';
import 'package:analyzer/src/lint/linter_visitor.dart';
import 'package:benchmark_harness/benchmark_harness.dart';

const ruleCount = 10; // Change to any number

void main() {
  SharedVisitorBenchmark().report();
  MultipleVisitorBenchmark().report();
}

// Create a new benchmark by extending BenchmarkBase
class SharedVisitorBenchmark extends BenchmarkBase {
  SharedVisitorBenchmark() : super('Shared visitors');

  late ParseStringResult result;
  late NodeLintRegistry registry;

  // The benchmark code.
  @override
  void run() {
    result.unit.visitChildren(SharedVisitor(registry));
  }

  // Not measured setup code executed prior to the benchmark runs.
  @override
  void setup() {
    registry = NodeLintRegistry(false);
    for (var i = 0; i < ruleCount; i ++) {
      registry.addAdjacentStrings(
        MyLinter(name: 'linter'),
        LinterImpl(),
      );
    }

    result = parseFile(
      path:
          '/Users/H010675/StudioProjects/personal/projects/framework_example/lib/main.dart',
      featureSet: FeatureSet.latestLanguageVersion(),
    );
  }
}

class SharedVisitor extends LinterVisitor {
  SharedVisitor(super.registry);
}

class MyLinter extends LintRule {
  MyLinter({
    required super.name,
  }) : super(
          group: Group.style,
          description: name,
          details: 'details',
        );
}

class LinterImpl extends SimpleAstVisitor<void> {
  @override
  void visitAdjacentStrings(AdjacentStrings node) {
    doSomething(node);
  }
}

// Create a new benchmark by extending BenchmarkBase
class MultipleVisitorBenchmark extends BenchmarkBase {
  MultipleVisitorBenchmark() : super('Multiple visitors');

  late ParseStringResult result;

  // The benchmark code.
  @override
  void run() {
    for (var i = 0; i < ruleCount; i ++) {
      result.unit.visitChildren(IndividualVisitor());
    }
  }

  // Not measured setup code executed prior to the benchmark runs.
  @override
  void setup() {
    result = parseFile(
      path:
          '/Users/H010675/StudioProjects/personal/projects/framework_example/lib/main.dart',
      featureSet: FeatureSet.latestLanguageVersion(),
    );
  }
}

class IndividualVisitor extends RecursiveAstVisitor<void> {
  @override
  void visitAdjacentStrings(AdjacentStrings node) {
    doSomething(node);
    super.visitAdjacentStrings(node);
  }
}

void doSomething(AstNode node) {}

With 2 rules
Shared visitors(RunTime): 38.91495106311171 us.
Multiple visitors(RunTime): 29.818449089305464 us.

With 10 rules
Shared visitors(RunTime): 40.316888920925535 us.
Multiple visitors(RunTime): 150.44662596931198 us.

With 100 rules
Shared visitors(RunTime): 61.3511756603272 us.
Multiple visitors(RunTime): 1579.019 us.

As you can see, adding more lint rules significantly increases run time with individual visitors due to the O(rn) complexity. This is why the Dart SDK shares a visitor. It makes it so adding more rules doesn't increasingly pollute the time it takes to compute lints. I'd say a 7 microsecond increase when you have a few lints is worth it running 25 times faster with 100 lints?

[Clavum]

I really like the way the SDK does this. Their "Fixes" are generic classes that are agnostic to which rule they are used by. Then they provide a map of each rule and the fixes relevant to them. For example, the AddAsync fix can add async from any place in the code, so that multiple lint rules can use it. I see this being complicated for package users though.

Before I forget, here's the links for what I was talking about when I said this.

Here's all the "context agnostic" code corrections:
https://github.com/dart-lang/sdk/tree/main/pkg/analysis_server/lib/src/services/correction/dart

And here's where the lints are mapped to the corrections they can use:
https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/services/correction/fix_internal.dart

It's a cool pattern worth considering, but could be complicated for package users because then they have to design their corrections to work in any context instead of just for the lint. It would be really awesome, though, if we could reuse these Dart corrections instead of making our own, but they are in src so they might not be not client friendly.

[rrousselGit]

I see, thanks for the counter benchmark.

Stil I don't like the added complexity, and with such an approach, there's no way for custom_lint to await the execution of the lint logic.

[Clavum]

If you're referring to knowing when all lints are done, then that is after resolvedUnitResult.unit.visitChildren(visitor); is completed.

If you're referring to knowing when a single lint is done, then that indeed seems impossible since all lints are mixed together into the same visitor. But why would you need to know this?

[rrousselGit]

then that is after resolvedUnitResult.unit.visitChildren(visitor); is completed.

No, because some lints might want to be async.
A lint can have a visitor to be async, and visitChildren wouldn't know about it.

[Clavum]

Not a single dart lint is asynchronous. Why would our custom lints need to be? They can do everything synchronously.

[rrousselGit]

I've made several asynchronous lints before. It's not impossible that I missed some util that would've enabled me to make it synchronously instead of async.

But I know the average user will try to make an async lint at some point. And they will probably take a long time to realize that their async logic has broken the custom_lint CLI

Being able to handle asynchronous lints, even if optional, would add some safeguard.

[Clavum]

A couple options come to mind.

1. Make a custom NodeLintRegistry which has callbacks that are FutureOr<void>. I don't think this solution is very ideal though, as that's a lot of code to maintain.
2. Have asynchronous work be done before AST analysis.
   a. The SDK does this by gathering all possible information a lint would need to know into a LintContext model, and then provides this to the lints so that all data is available synchronously. However, with custom lints, this means the plugin could be spending time gathering information that no lints ever use. However, this has the benefit that an asynchronous task will only be done once.
   b. As another option, we could have a FutureOr<void> setUp() method in the LintRule class, where any asynchronous work
   can be done ahead of time and the results are stored within fields of the LintRule that can then be used within any of the
   visit methods. However, this could mean that two lints that need the same asynchronous info compute it twice instead of sharing.

their async logic has broken the custom_lint CLI

If no methods of the LintRule may be a Future, it is impossible for a rule to have anything asynchronous. I suppose they could call an asynchronous method within a synchronous method and it would not be awaited, but that should be a red flag for anyone.

[rrousselGit]

Yeah I'm experimenting with those right now. I'll see what I can come-up with.

[rrousselGit]

Do we really need to register an AstVisitor for the shared Visitor btw?

I was thinking that we could do:

register(registry) {
  registry.onVisitVarialeDeclaration((variableDeclaration) {
    reporter.reportError(...);
  });
}

This removes the need for making a separate AstVisitor (which is apparently always a SimpleAstVisitor in the SDK), removing the need for registering each node listener.

In terms of organization, it's a bit worse. But it's probably more intuitive for the average Dart dev. Otherwise, I'm 100% sure many will forget the "register" step. Even I am sure I'll make the mistake at some point.

[Clavum]

Yeah, I agree the registering needs to be more intuitive. It's extremely easy to forget to keep it up to date, and then when you miss something, there's no indication that's the problem.

Only way I could think of doing it more automatically is something like this that detects if a visit method has been overridden:

Classes

class LintRuleBase extends LintRule {
  LintRuleBase({
    required super.name,
    required super.group,
    required super.description,
    required super.details,
  });
}

abstract class ExampleLintRule extends SimpleAstVisitor<void> {
  final String message;
  final String code;
  final String? url;

  ExampleLintRule({
    required this.message,
    required this.code,
    this.url,
  });

  late LintRule rule = LintRuleBase(
    description: '',
    details: message,
    group: Group.style,
    name: code,
  );


  void registerNodeVisitors(NodeLintRegistry registry) {
    if (visitClassDeclarations != null) {
      registry.addClassDeclaration(rule, this);
    }
    // About 150 more if statements :(
  }

  Function(ClassDeclaration node)? get visitClassDeclarations => null;
  // 150 more getters :'(
  // Had to add an "s" to not conflict with the name of the actual method below.

  @override
  void visitClassDeclaration(ClassDeclaration node) {
    visitClassDeclarations?.call(node);
  }
  // 150 more visit methods D:
}

Then a developer uses it like this:

Usage

class FinalProviders extends ExampleLintRule {
  FinalProviders() : super(code: 'some_lint_code', message: 'Fix this');

  @override
  Function(ClassDeclaration node) get visitClassDeclarations => (ClassDeclaration node) {
    print('Hello world');
  };
}

But the syntax of overriding a getter that returns a method is strange, not to mention the lint rule class would be massive.

Your example may just be the way to go, for lack of a better option.

[Clavum]

Although, wait, the NodeLintRegistry's addSomethingDeclaration methods expect to be given a LintRule and a Visitor. How will you get those?

[rrousselGit]

I wouldn't use the SDK's NodeLintRegistry.
It's not public anyway.

I'd make a custom implementation with a different prototype.

[Clavum]

That could work then

[rrousselGit]

For reference, I ended up with the following. That's going to be the new example (with a lint + fix + assist)

I've used the shared visitor logic for assists/fixes too.

import 'package:analyzer/error/error.dart';
import 'package:analyzer/error/listener.dart';
import 'package:analyzer/source/source_range.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';

const _providerBaseChecker =
    TypeChecker.fromName('ProviderBase', packageName: 'riverpod');

PluginBase createPlugin() => _RiverpodLint();

class _RiverpodLint extends PluginBase {
  @override
  List<LintRule> getLintRules(CustomLintConfigs configs) => [
        PreferFinalProviders(),
      ];

  @override
  List<Assist> getAssists() => [_ConvertToStreamProvider()];
}

class PreferFinalProviders extends DartLintRule {
  PreferFinalProviders() : super(code: _code);

  static const _code = LintCode(
    name: 'riverpod_final_provider',
    problemMessage: 'Providers should be declared using the `final` keyword.',
  );

  void run(
    CustomLintResolver resolver,
    ErrorReporter reporter,
    LintContext context,
  ) {
    context.registry.addVariableDeclaration((node) {
      final element = node.declaredElement;
      if (element == null ||
          element.isFinal ||
          !_providerBaseChecker.isAssignableFromType(element.type)) {
        return;
      }

      reporter.reportErrorForElement(PreferFinalProviders._code, element);
    });
  }

  @override
  List<Fix> getFixes() => [_MakeProviderFinalFix()];
}

class _MakeProviderFinalFix extends DartFix {
  @override
  void run(
    CustomLintResolver resolver,
    ChangeReporter reporter,
    LintContext context,
    AnalysisError analysisError,
    List<AnalysisError> others,
  ) {
    final errorRange = SourceRange(analysisError.offset, analysisError.length);

    context.registry.addVariableDeclarationList((node) {
      final nodeRange = SourceRange(node.offset, node.length);
      if (!errorRange.intersects(nodeRange)) return;

      final changeBuilder = reporter.createChangeBuilder(
        priority: 1,
        message: 'Make provider final',
      );
      changeBuilder.addDartFileEdit((builder) {
        final nodeKeyword = node.keyword;
        final nodeType = node.type;
        if (nodeKeyword != null) {
          // var x = ... => final x = ...
          builder.addSimpleReplacement(
            SourceRange(nodeKeyword.offset, nodeKeyword.length),
            'final',
          );
        } else if (nodeType != null) {
          // Type x = ... => final Type x = ...
          builder.addSimpleInsertion(nodeType.offset, 'final ');
        }
      });
    });
  }
}

class _ConvertToStreamProvider extends DartAssist {
  @override
  void run(
    CustomLintResolver resolver,
    ChangeReporter reporter,
    LintContext context,
    SourceRange target,
  ) {
    context.registry.addVariableDeclaration((node) {
      // Check that the visited node is under the cursor
      if (!target.intersects(SourceRange(node.offset, node.length))) {
        return;
      }

      // verify that the visited node is a provider, to only show the assist on providers
      final element = node.declaredElement;
      if (element == null ||
          element.isFinal ||
          !_providerBaseChecker.isAssignableFromType(element.type)) {
        return;
      }

      final changeBuilder = reporter.createChangeBuilder(
        priority: 1,
        message: 'Convert to StreamProvider',
      );
      // TODO implement change
    });
  }
}

[rrousselGit]

Now we can do:

# analysis_options.yaml

custom_lint:
  rules:
    - riverpod_final_provider: false

[Clavum]

Awesome, this is really great!

reporter.reportErrorForElement(PreferFinalProviders._code, element);

Could the reporter be given the code from within the package instead of users passing it on every report? If not, could this at least be this.code instead of PreferFinalProviders._code, since code is a field of super?

// Check that the visited node is under the cursor
if (!target.intersects(SourceRange(node.offset, node.length))) {
return;
}

Can/should this be done from within the package?

custom_lint:
rules:
- riverpod_final_provider: false

I'm assuming all rules will be enabled by default unless disabled, right? Hopefully this would be clear to users that it's the opposite from official dart lints.

[rrousselGit]

Could the reporter be given the code from within the package instead of users passing it on every report? If not, could this at least be this.code instead of PreferFinalProviders._code, since code is a field of super?

The ErrorReporter class is from the SDK. I didn't write this one.
We could have a wrapper around it, but not sure that's truly necessary.

Using the static variable is not necessary. Technically there's a code property on the object. I just used the static one here because of a previous iteration where the visitor and the fix were separate (and therefore you had to pass the code to the visitor).

In this variant, using this.code should be fine

Can/should this be done from within the package?

I thought about it, but I'm skeptical. I figured someone may want to visit things that are outside of the cursor too.
If it's a problem, this can be revised later. But for now, I figured doing this manually was decent enough

I'm assuming all rules will be enabled by default unless disabled, right? Hopefully this would be clear to users that it's the opposite from official dart lints.

All lints are enabled by default, and there's a flag for changing the default. So you can do:

custom_lint:
  enable_all_lint_rules: false
  rules:
  - my_rule

[rrousselGit]

Can/should this be done from within the package?

Continuing on this, one thought I had was to keep doing the if, but simplify the condition a bit by having some extensions on AstNode to obtain a SourceRange

So we could do:

      // Check that the visited node is under the cursor
      if (!target.intersects(node.sourceRange)) {
        return;
      }

The gain is minor though.

[rrousselGit]

I'm also probably going to implement a custom_lint list command, for listing all the lints (for #50)

And I'm considering implementing auto-completion in the analysis_options.yaml, to auto-complete lint names.

[Clavum]

I think you could use this:
https://github.com/dart-lang/sdk/blob/main/pkg/analyzer_plugin/lib/utilities/range_factory.dart

It would then be
if (!target.intersects(range.node(node)) {

[rrousselGit]

Fair enough, didn't know it was a thing.

[rrousselGit]

I'll still include an extension to do "node.sourceRange"
It's more intuitive as it shows up in the autocomplete.

[Clavum]

Yep sounds good

[rrousselGit]

It's all working on #63

I was playing around with analyzing the non-dart file (which now works fine). I thought I'd find a similar visitor pattern in the SDK for Yaml/Pubspec/AnalysisOptions visitors, but apparently not.

I guess I'll add a "PubspecLintRule" & co at a later date.

[Clavum]

Thanks a lot for doing this!

[rrousselGit]

Thanks for your involvement in this issue too :)