capture lint<=>automation correspondences (for scoring)

pq commented 5 years ago

Follow-up from #1374, we should identify all lints that have server quick assists that can act as "fixes" so we can update the scorecard.

[x] capture assist associations in analyzer source (analyzer PR)
[x] extract assist mappings in our scoring (https://github.com/dart-lang/linter/pull/1415)
[ ] create an exclude list for unfixable lints
[ ] rank and open issues for top candidates

/cc @bwilkerson

pq commented 5 years ago

Thinking over this a little more, I think this is less about fix vs. assist but rather whether a remedy can be automatically applied by a tool (and note that the formatter is another one).

With that in mind we might have a table like this:

name	automation
`always_declare_return_types`
`always_put_control_body_on_new_line`
`always_put_required_named_parameters_first`
`always_require_non_null_named_parameters`	fix
`always_specify_types`
`annotate_overrides`	fix
`avoid_annotating_with_dynamic`	fix
`avoid_as`
`avoid_bool_literals_in_conditional_expressions`
`avoid_catches_without_on_clauses`
`avoid_catching_errors`
`avoid_classes_with_only_static_members`
`avoid_double_and_int_checks`
`avoid_empty_else`	fix
`avoid_field_initializers_in_const_classes`
`avoid_function_literals_in_foreach_calls`
`avoid_implementing_value_types`
`avoid_init_to_null`	fix
`avoid_js_rounded_ints`
`avoid_null_checks_in_equality_operators`
`avoid_positional_boolean_parameters`
`avoid_private_typedef_functions`
`avoid_relative_lib_imports`
`avoid_renaming_method_parameters`
`avoid_return_types_on_setters`	fix
`avoid_returning_null`
`avoid_returning_null_for_future`
`avoid_returning_null_for_void`
`avoid_returning_this`
`avoid_setters_without_getters`
`avoid_shadowing_type_parameters`
`avoid_single_cascade_in_expression_statements`
`avoid_slow_async_io`
`avoid_types_as_parameter_names`
`avoid_types_on_closure_parameters`	fix
`avoid_unused_constructor_parameters`
`avoid_void_async`
`await_only_futures`	fix
`camel_case_types`
`cancel_subscriptions`
`cascade_invocations`
`close_sinks`
`comment_references`
`constant_identifier_names`
`control_flow_in_finally`
`curly_braces_in_flow_control_structures`
`directives_ordering`
`empty_catches`	fix
`empty_constructor_bodies`	fix
`empty_statements`	fix
`file_names`
`flutter_style_todos`
`hash_and_equals`
`implementation_imports`
`invariant_booleans`
`iterable_contains_unrelated_type`
`join_return_with_assignment`
`library_names`
`library_prefixes`
`lines_longer_than_80_chars`
`list_remove_unrelated_type`
`literal_only_boolean_expressions`
`no_adjacent_strings_in_list`
`no_duplicate_case_values`
`non_constant_identifier_names`	fix
`null_closures`
`omit_local_variable_types`
`one_member_abstracts`
`only_throw_errors`
`overridden_fields`
`package_api_docs`
`package_names`
`package_prefixed_library_names`
`parameter_assignments`
`prefer_adjacent_string_concatenation`
`prefer_asserts_in_initializer_lists`
`prefer_bool_in_asserts`
`prefer_collection_literals`	fix
`prefer_conditional_assignment`	fix
`prefer_const_constructors`
`prefer_const_constructors_in_immutables`
`prefer_const_declarations`	fix
`prefer_const_literals_to_create_immutables`
`prefer_constructors_over_static_methods`
`prefer_contains`
`prefer_equal_for_default_values`
`prefer_expression_function_bodies`
`prefer_final_fields`	fix
`prefer_final_in_for_each`
`prefer_final_locals`	fix
`prefer_foreach`
`prefer_function_declarations_over_variables`
`prefer_generic_function_type_aliases`
`prefer_initializing_formals`
`prefer_int_literals`
`prefer_interpolation_to_compose_strings`
`prefer_is_empty`
`prefer_is_not_empty`	fix
`prefer_iterable_whereType`
`prefer_mixin`
`prefer_single_quotes`
`prefer_typing_uninitialized_variables`
`prefer_void_to_null`
`public_member_api_docs`
`recursive_getters`
`slash_for_doc_comments`
`sort_constructors_first`
`sort_pub_dependencies`
`sort_unnamed_constructors_first`
`super_goes_last`
`test_types_in_equals`
`throw_in_finally`
`type_annotate_public_apis`
`type_init_formals`	fix
`unawaited_futures`
`unnecessary_await_in_return`
`unnecessary_brace_in_string_interps`
`unnecessary_const`
`unnecessary_getters_setters`
`unnecessary_lambdas`	fix
`unnecessary_new`
`unnecessary_null_aware_assignments`
`unnecessary_null_in_if_null_operators`
`unnecessary_overrides`
`unnecessary_parenthesis`
`unnecessary_statements`
`unnecessary_this`	fix
`unrelated_type_equality_checks`
`use_function_type_syntax_for_parameters`
`use_rethrow_when_possible`
`use_setters_to_change_properties`
`use_string_buffers`
`use_to_and_as_if_applicable`
`valid_regexps`
`void_checks`

pq commented 5 years ago

@bwilkerson: I wonder how you feel about adding some breadcrumbs to the analyzer sources to capture correspondences between assists and lints in-line. For example, the assist added by _addProposal_convertIntoFinalField() could be seen to map to prefer_finals. What if we added a comment to it's body or, better still, maybe in compute() where it's called?

Future<List<Assist>> compute() async {
  ...
  await _addProposal_assignToLocalVariable();
  await _addProposal_convertClassToMixin();
  // lint: prefer_finals
  await _addProposal_convertIntoFinalField();
  ...
}

I'm guessing a string would be enough though we might consider an annotation if we want to protect against accidentally breaking the expected format. (@lint_rule('prefer_finals')?)

Just spitballing so any thoughts appreciated!

bwilkerson commented 5 years ago

That's not a bad idea, but it does have the drawback that other tools can't reasonably use the information.

I had a slightly different idea earlier this morning. Server defines a class named AssistKind (in analyzer_plugin/lib/utilities/assist/assist.dart). An instance of this class is attached to every assist that is created (defined in DartAssistKind in analysis_server/lib/src/services/correction/assist.dart). What I think we should do is add a field to AssistKind whose value is a list of the names of the lints for which the assist can be used like a fix.

Capturing the information in the data structure would not only serve the immediate goal, but would then allow us to pass this information to server's clients so that they could use this to present "fixes" for lint violations in places where assists would otherwise not be appropriate (such as the "Dart Analysis" view in IntelliJ).

pq commented 5 years ago

Oh! Now that's so much better. I love that it could solve the "align assists and fixes" problem too. I'm sold!

pq commented 5 years ago

@bwilkerson: given this datastructure would live in server, are you imagining we'd parse it out of the source the way we currently do for fixes?

FWIW: here's where we do that:

https://github.com/dart-lang/linter/blob/36316c3995344b2d169b1d08afce26fc47b47d8f/tool/scorecard.dart#L112-L128

Ideally I could add a _getLintsWithAssists counterpart there.

bwilkerson commented 5 years ago

I imagine we could do something similar (though personally I would have used the parser to create an AST and then visited it :-). If we make the list of lint rule names a named parameter for the AssistKind constructor, then we can look for uses of that name.

pq commented 5 years ago

Food for thought:

https://dart-review.googlesource.com/c/sdk/+/91857

I would have used the parser to create an AST and then visited it :-)

Oh. Now that's a great idea! If it makes sense to tackle assists that way, I'll update the fix scoring at the same time.

Thanks!

dart-lang / linter

capture lint<=>automation correspondences (for scoring) #1375