Allow lints to accept configurations

[a14n]

It would be useful and nice to be able to customize some lints with parameter/configuration.

For instance:

linter:
  rules:
    - file_headers:
        pattern: '// Copyright 2015 The Chromium Authors. All rights reserved.\n// Use of this source code isgoverned by a BSD-style license that can be\n// found in the LICENSE file.'
    - prefer_const_declarations:
        local_variables: false

[a14n]

@bwilkerson @pq wdyt ?

[bwilkerson]

Honestly, I have mixed feelings. On the one hand, this can be really useful and allow a much richer UX. But it's easy to abuse, and to end up with rules that are difficult to configure.

I worked on a similar product at a previous company, and we made the mistake of adding configuration support. We didn't have outside contributions, so we had no one to blame but ourselves, but we ended up with a product that users couldn't use until they had spent multiple hours going through the list of lints to decide whether they wanted to use the lint and if so how to configure it. The task was so daunting that many of our customers never realized the full potential value of the product.

So, yeah, it would be great to be able to specify the file header to a lint so that the quick fix could insert it when it's missing. On the other hand, I'm not convinced that it isn't better, for something like "include local variables in the lint" to just make them two separate lints and allow users to enable the functionality they want by choosing the lints they enable (something they have to do anyway).

[pq]

I always thought we'd do this (see for example this old tracking issue: #57153) but as time has gone on I'm not as keen (for the reasons that Brian cites).

Very open to reconsidering though!

[a14n]

I agree configuration should be an exception. And for a file_header lint I pointed above configuration is definitively mandatory.

[jibiel]

@bwilkerson

But it's easy to abuse, and to end up with rules that are difficult to configure.

So the way to go as I see it is to introduce just a few super basic options to a linter rule which will not allow overconfiguring by design.

A good example is the Ruby linter which introduces a few basic options for each rule: Exclude, Enabled and EnforcedStyle, and maybe 1 or 2 specific (but still basic) options depending on the rule.

Here's how it looks:

Layout/AccessModifierIndentation:
  EnforcedStyle: outdent
  IndentationWidth: 0

Layout/LineLength:
  Exclude:
    - config/**/*
  Max: 80

Lint/EmptyBlock:
  Exclude:
    - spec/factories/**/*

Style/Lambda:
  EnforcedStyle: literal

[pq]

FWIW, when trying to enable implementation_imports in analyzer I found myself really missing this. What I'd like is to be able to say _fe_analyzer is privileged (or friends or similar) since really src imports in this case are expected.

It'd be interesting to explore places that (judicious) configuration would really open up and weigh them against the downsides of the added complexity.

Input welcome!

[bwilkerson]

Is the problem that implementation_imports needs to be configurable because sometimes users validly need to reach into the src directory, or is the problem that _fe_analyzer_shared (and a few other packages in the SDK) isn't following the established conventions, and hence a lint to enforce that convention can't be used? Personally, I think it's the latter and that making the lint configurable would be bad for most of our users.

[pq]

Ah. Excellent point. If _fe_analyzer_shared were structured more for code sharing this would be a non-issue.

/fyi @stereotype441 @johnniwinther

[bwilkerson]

Yes, but it's structured the way it is because we're explicitly not making any guarantees about the API that it provides. Packages outside the SDK should never depend on it or import it.

[pq]

Understood. But isn't that what the _ is for?

In any event, probably more fuss than it's worth to get around a handful of //ignores...

[bwilkerson]

I'm not aware of any enforcement of the convention (which might only exist for this one package), but yes, it's prefixed with an underscore to attempt to communicate that it's private.

[stereotype441]

+1 to what Brian said about _fe_analyzer_shared. It's not so much that it's not structured for code sharing, it's that it's structured for use only by the analyzer and the front end. We really don't want to take on the maintenance burden of having other packages start to depend on it and then file bugs when we change its API.

There are other approaches we could use instead, I suppose. For example, _fe_analyzer_shared could be a subdirectory of the analyzer's lib/src directory, and then the front end could make use of it via import 'package:analyzer/src/_fe_analyzer_shared/.... But that kind of seems worse than what we're doing today.

I agree with Phil that, at least in the case of _fe_analyzer_shared a few // ignore comments are a small price to pay compared to restructuring a bunch of stuff.

[FMorschel]

As pointed in #59211, the behaviour of "avoid_positional_boolean_parameters is only triggered by public methods" is not expected but was designed that way.

I do believe that there could be one more option inside the analyzer or similar where we could define project-wise if some lint will trigger for private members or not. Or even, there could be an ignore-for-file tag or other ignore name that would allow us to set that for a file.

Something similar to what we already have today with:

analyzer:
  plugins:
    - dart_code_metrics
  language:
    strict-casts: true
  exclude:
    - "**.g.dart"
  errors:
    missing_required_param: error
  # Added new option:
  ignore_for_private_members:
    - avoid_positional_boolean_parameters

Or in the ignore something like:

// ignore-for-private-members-in-file: avoid_positional_boolean_parameters

That could also, at least help with #54374 main idea for example.

[FMorschel]

Just pointing out this would be great if made around the same time as #57034, since they are about the same thing.