Error Suppression FTW.

Error Suppression FTW.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/2f9c4af993df48cdd7180dcb79d36dbe6aa462f0

[pquitslund, 2015-10-27 16:06:00]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[pquitslund, 2015-10-27 16:06:00]

Introduces a new analysis option that allows developers to suppress
errors/warnings/hints/lints at the project (e.g., context)- level by specifying
error codes in an `.analysis_options` file.  For example:

analyzer:
  suppress:
    - invalid_assigmment

associates an error filter with the context that filters errors with the
`invalid_assigmment` (case insensitive) error code name.  This support will
allow us to largely do away with the need for error-scraping post-processors
(like skyanalyzer) and will align the IDE with them to boot.

A few obvious concerns (and counters):

1. But what about stable identifiers?  We’re not done arguing about them yet! 

:)

If error codes change or another consensus is reached about stable identifiers
it will be easy to guide people to the preferred path.  In a follow-up CL I’ll
add fixes (for typos) and completion for discovery.  If we change the scheme,
these mechanisms can help guide the user back onto the rails.

2.  What about scoping?  Is this a global namespace?

Similar to above my thinking is that we can evolve this scheme pretty easily. 
If it turns out we need to qualify identifiers, I think it will be easy to
migrate to that kind of scheme.  For example, 

invalid_assigmment

might become

sdk/invalid_assigmment

or 

static_warning/invalid_assigmment

More background and motivation can be found in the following.

skyanalyzer:
https://github.com/flutter/engine/blob/master/sky/tools/skyanalyzer

Please allow a package author to ship configurations for analyzer:
https://github.com/dart-lang/sdk/issues/24733

Allow a user to configure which hints are "fatal”:
https://github.com/dart-lang/sdk/issues/24452

Stable identifiers for analysis errors/warnings/hints/lints:
https://github.com/dart-lang/sdk/issues/24522

Configure analyzer to enable/disable certain lints:
https://github.com/dart-lang/sdk/issues/24680

[sethladd, 2015-10-27 16:24:32]

> But what about stable identifiers?

Did we consider adding a prefix to the identifiers? e.g. "X_invalid_assignment"
or "TEMP_invalid_assignment"

[pquitslund, 2015-10-27 16:38:34]

On 2015/10/27 16:24:32, sethladd wrote:
> > But what about stable identifiers?
> 
> Did we consider adding a prefix to the identifiers? e.g.
"X_invalid_assignment"
> or "TEMP_invalid_assignment"

If that's what it takes, sure.  My preference is to keep it cleaner with the
understanding that we can tool our way towards any coming consensus.  Plus, what
if it's not temporary?

[Brian Wilkerson, 2015-10-27 19:18:59]

I think we'd be better off making the format more general so that we can solve
more problems with the same mechanism. The current format only allows users to
suppress errors, but we also have requests to allow changing the severity of
errors. How about something like:

analyzer:
  errors:
    - invalid_assigmment: disable
    - unused_variable: error

Also, the names of the error codes might not be sufficient. We have hint codes
with the same name as static warning codes, so using the name without
qualification will mean that they cannot be differentiated.

https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:19: const List<ErrorFilter> _noFilters
= const <ErrorFilter>[];
I'd prefer to have this inlined, given that it's only referenced in one place.

[pquitslund, 2015-10-27 20:41:57]

Thanks Brian!

On 2015/10/27 19:18:59, Brian Wilkerson wrote:
> I think we'd be better off making the format more general so that we can solve
> more problems with the same mechanism. The current format only allows users to
> suppress errors, but we also have requests to allow changing the severity of
> errors. How about something like:
> 
> analyzer:
>   errors:
>     - invalid_assigmment: disable
>     - unused_variable: error

Love it!

As per our conversation, I'll update this scheme to support these more
open-ended mappings.  To start, I'll stick to simple disablement but leave the
door open for things like escalation.

> Also, the names of the error codes might not be sufficient. We have hint codes
> with the same name as static warning codes, so using the name without
> qualification will mean that they cannot be differentiated.

Ah, great point.  I didn't realize that the names weren't uniquely identifying.

As discussed, we can see if this hits us in practice. If it does, we can cook up
a disambiguation scheme.  (Aside: if/when we transition to stable identifiers
this should go away altogether.  The race is on! :))

> 
>
https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
> File pkg/analyzer/lib/src/generated/error.dart (right):
> 
>
https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/error.dart:19: const List<ErrorFilter>
_noFilters
> = const <ErrorFilter>[];
> I'd prefer to have this inlined, given that it's only referenced in one place.

Sure thing.  I'll update.

[pquitslund, 2015-10-27 22:33:42]

Updated as per conversations.

The new format supports key/values with 'false' and 'ignore' used to specify
that a given error code should be filtered out.  For example:

  analyzer:
    errors:
      unused_local_variable: ignore

and

  analyzer:
    errors:
      unused_local_variable: false

are equivalent.

Down the road we might add other verbs.  For example:

  analyzer:
    errors:
      unused_local_variable: error

could promote this hint to an error.

Comments welcome!

https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1420363005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:19: const List<ErrorFilter> _noFilters
= const <ErrorFilter>[];
On 2015/10/27 19:18:59, Brian Wilkerson wrote:
> I'd prefer to have this inlined, given that it's only referenced in one place.

Done.

[scheglov, 2015-10-27 23:41:45]

LGTM

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1033:
workManager.onAnalysisOptionsChanged();
Maybe we should rename this method to reflect the fact that it is used after
options are changed.

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:23: final
ResultDescriptor<List<ErrorFilter>> CONFIGURED_ERROR_FILTERS =
ListResultDescriptor

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:28: * In the future, may support
changing severity as well.
Does returning `true` mean "include the value into the output" or "remove it
from the output"?

[Brian Wilkerson, 2015-10-28 02:09:08]

LGTM

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1033:
workManager.onAnalysisOptionsChanged();
Is that the only time it's used? (I actually prefer names that indicate what the
method does, rather than when it's used.)

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:28: : super('analyzer', const ['exclude',
'plugins', 'strong-mode', 'errors']);
Seems like these should be constants so that we can't forget to change them
everywhere if we change them.

[pquitslund, 2015-10-28 15:56:44]

Thanks for the comments!

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1033:
workManager.onAnalysisOptionsChanged();
On 2015/10/28 02:09:08, Brian Wilkerson wrote:
> Is that the only time it's used? (I actually prefer names that indicate what
the
> method does, rather than when it's used.)

Maybe `invalidateResults` or something?  It's pretty nuclear.  Or maybe
`nuclearOption`? ;)

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:23: final
ResultDescriptor<List<ErrorFilter>> CONFIGURED_ERROR_FILTERS =
On 2015/10/27 23:41:44, scheglov wrote:
> ListResultDescriptor

Done.

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error.dart:28: * In the future, may support
changing severity as well.
On 2015/10/27 23:41:44, scheglov wrote:
> Does returning `true` mean "include the value into the output" or "remove it
> from the output"?

Updated doc.

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1420363005/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:28: : super('analyzer', const ['exclude',
'plugins', 'strong-mode', 'errors']);
On 2015/10/28 02:09:08, Brian Wilkerson wrote:
> Seems like these should be constants so that we can't forget to change them
> everywhere if we change them.

Absolutely.  Another case of "real" enum-envy. :(  I've hoisted these to
constants as a quick fix.

[pquitslund, 2015-10-28 16:59:12]

Committed patchset #6 (id:100001) manually as
2f9c4af993df48cdd7180dcb79d36dbe6aa462f0 (presubmit successful).