Linter

[rrousselGit]

Let's add custom lint rules for helping to catch mistakes

It can include things such as:

• "Avoid using ref.read inside build"
• "Do not dynamically create providers"
• "Avoid passing providers as parameter to objects"
• "Avoid ref.watch outside of build"
• check for extra/missing dependencies
• avoid ref.read(autoDispose)
• use container.listen + sub.read instead of container.read on autoDispose providers
• async ref.watch(autoDispose) (typically inside FutureProviders)
• equivalent of use_build_context_synchronously for WidgetRef instead of BuildContext (proposal: extend use_build_context_synchronously to consider Riverpod's WidgetRef dart-lang/sdk#58744)
• ref.watch(family([])) (or other non-constant object instantiation with no == override)
• check circular dependencies
• don't mutate other providers inside build
• avoid global ProviderContainer
• Don't pass WidgetRef as parameter outside of widgets
• Make sure lints work with codegen
• Improve performance of linter (see custom_lint) issues
• avoid overrides on non-root ProviderScope/ProviderContainer
• no ProviderScope detected on runApp

and refactoring options such as:

• "wrap in a consumer"
• "extract to consumer widget"
• "convert to hookconsumerwidget"
• "container.listen + sub.read instead of container.read on autoDispose providers"
• ...

[nt4f04uNd]

"Avoid ref.watch outside of build"

What about inside a Provider, isn't it also a valid usage?

[rrousselGit]

I consider the body of a provider as a build method too. It's just that instead of taking a context and returning a Widget, it takes a ref and return your state.

[pattobrien]

It'd be interesting if lints were to be used to highlight and offer quick fixes for performance optimizations, such as:

// dart-lang/sdk#57147 AnalysisErrorType.info when provider could be watched further down the widget tree,
// at the lowest common denominator of all Widgets consuming counterProvider
  Widget build(BuildContext context, WidgetRef ref) {
    final counterValue = ref.watch(counterProvider); // throws lint error (info level)
    return Scaffold(
      body: Center(
        child: Padding(
          padding: const EdgeInsets.all(10.0),
          child: Column(
            children: <Widget>[
              const Text(
                "You have pushed the button this many times:",
              ),
             // quick fix wraps Text widget in a ConsumerWidget
              Text(
                counterValue.toString(),
              ),
            ],
          ),
        ),
      ),
    );

// dart-lang/sdk#57148 AnalysisErrorType.info when select() method can be used instead
 Widget build(BuildContext context, WidgetRef ref) {
    // throws lint error (info level)
    final counterValue = ref.watch(asyncCounterProvider);
    // quick fix replacement:
    // final counterValue = ref.watch(asyncCounterProvider.select((value) => value.valueOrNull));
    return Scaffold(
      body: Column(
        children: <Widget>[
          const Text(
            "You have pushed the button this many times:",
          ),
          Text(
            counterValue.value.toString(),
            // quick fix replacement:
            // counterValue.toString(),
          ),
        ],
      ),
    );
  }

I'm already comfortable with the analyzer APIs / custom_lint APIs from my own analyzer plugin development, so I could take this on myself if it seems worthwhile. What do you think @rrousselGit ?

[TimWhiting]

@pattobrien
I'm currently working on the linter plugin. There is a PR on this repo with the current state of things. Happy to have some help, as long as it doesn't end in too many merge conflicts. So we should probably coordinate which lint's we are working on. @rrousselGit Maybe we can merge the linter branch and then iterate from the current state?

[pattobrien]

Maybe we can merge the linter branch and then iterate from the current state?

I just took a look at your PR / fork (looks top notch by the way), and see what you mean regarding potential conflicts. I don't want to unnecessarily derail any of the 2.0 plans, but would be more than happy to help in whatever timeline and capacity you see fit.

[rrousselGit]

@TimWhiting I'm not working on any lint. So as long as your PR is ready to merge, I'll do so.

[rrousselGit]

It'd be interesting if lints were to be used to highlight and offer quick fixes for performance optimizations, such as:

There would be too many false positives. We don't always need a select

I think what's more appropriate is a refactoring command, like "refactor to use select".

[pattobrien]

There would be too many false positives. We don't always need a select

That's true if used strictly as a lint, but in a CI/CD environment or an environment where this enhancement were to be supported, such optimization analyses could be reserved for pre-launch or refactoring phases, rather than being enabled at all times in the IDE.

Dart code metrics has some thought provoking use cases in this area, e.g. calculating total technical debt project-wide, # of independent paths in a method to determine code cleanliness. Such warnings would likely be annoying to deal with at all times, but for certain points in a development cycle the benefits are obvious.

[TimWhiting]

@pattobrien
We now have the linter branch merged, I'm continuing to work on lints on this branch #1584. Do you want to work on the refactorings? TBH, most of these lints are pretty simple, I've already worked on a few of the harder ones which require analyzing non-local parts of the AST.

[pattobrien]

@TimWhiting awesome! today I familiarized myself with structure of the package (AsyncRecursiveVisitor, ProviderDeclaration, RiverpodVisitor etc).. I feel caught up to speed.

But regarding the refactorings, are you talking about AnalysisErrorFixes or actual refactoring like Extract Widget for Flutter? Because the latter AFAIK doesn't have support from the analyzer_plugin or custom_lint packages, or at least I thought... I think we only have support for Lints and Quick Fixes at the moment.

[rrousselGit]

Refactorings would need a PR to custom_lint to expose the associated API on the Plugin subclass.

There's one, in analyzer_plugin. It's just not exposed by custom_lint yet

[pattobrien]

@rrousselGit in an analyzer plugin I've been working on myself, the methods you're talking about handleEditGetAvailableRefactorings and handleEditGetRefactoring didn't seem to receive / send any events to the server. Only Quick Fixes and Errors were working.

I think there's a missing implementation on the analysis_server and/or analyzer_plugin package. I can confirm this tomorrow by trying again with custom_lint (which is in better shape than my own plugin) and file a dart sdk issue if refactoring isnt working.

[rrousselGit]

I'm pretty sure they work. It's been a while, but I believe I used them before.

[pattobrien]

Okay cool! Then I'll try it out tomorrow and open up a PR against custom_lint 👍🏼

[pattobrien]

I tried to implement refactorings by overriding the two handleEditGetRefactoring and handleEditGetAvailableRefactorings functions from the analyzer plugin package, and just as I had experienced with my own plugin, the functionality is not available from the plugin.

I came across this sdk issue, as well as a recent groups discussion about the limitation of the analyzer plugin API.

However, we can most likely use the assists API, as mentioned here. I'll explore implementing this into the custom_lint API.

[rrousselGit]

Ah yes, I thought refactorings were quick assists

If you're talking about things like "rename a property" which requires user input, there's no such thing.
Fortunately, most use-cases would fit quick assists. Like "Convert to ConsumerWidget" & co

[pattobrien]

Right - for reference, anything requiring user input seems to be blocked by this LSP issue.

Of the original 3 'Refactorings' and my proposed 2, only "extract to consumer widget" would make use of user input, so I'll plan to focus on all others first.

[rrousselGit]

There are a few more "convert to x" cases for the new code generator

[rrousselGit]

Riverpod_lint is out, so I'll close this :)

riverpod_lint has an internal list of TODOs for tracking implemented lints.