Lint rule: Do not override fields.

Implementation of the mentioned rule.

[Alexei Diaz, 2016-04-09 21:13:09]

alexeidiaz@google.com changed reviewers:
+ pquitslund@google.com

[Alexei Diaz, 2016-04-09 21:13:10]

Hi Phil,

I saw this issue in github (https://github.com/dart-lang/sdk/issues/25567) and
though giving it a try. Hopefully it is in a state where through a code review
can come to completion. I chose you as a reviewer since I saw commits from you
in the repo, please route to the best reviewer if that was not the right choice.

Thanks
Alexei

[Alexei Diaz, 2016-04-09 21:13:31]

Description was changed from

==========
Hint if a final field "overrides" non-final field.

BUG=25567
==========

to

==========
Hint if a final field "overrides" non-final field.
==========

[Lasse Reichstein Nielsen, 2016-04-09 22:03:36]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2016-04-09 22:03:37]

https://codereview.chromium.org/1870423002/diff/1/lib/src/rules/final_overrid...
File lib/src/rules/final_override_non_final.dart (right):

https://codereview.chromium.org/1870423002/diff/1/lib/src/rules/final_overrid...
lib/src/rules/final_override_non_final.dart:39: Object something = 'ipsum';
I'd argue that this is also bad.
You are shadowing the field in the superclass, but not removing it, so you now
have two fields called "something", only one of which is accessible. It is, at
least, a waste of memory.

[Alexei Diaz, 2016-04-10 23:12:29]

Agreed, I guess I was trying to implement only what was described in the github
issue report, certainly there is no need to check only for final fields.

Thanks for the review!

[Alexei Diaz, 2016-04-10 23:22:12]

Addressed comments for patch 1, apologies for the multiple replies, haven't
completely figured the code review tool.

https://codereview.chromium.org/1870423002/diff/1/lib/src/rules/final_overrid...
File lib/src/rules/final_override_non_final.dart (right):

https://codereview.chromium.org/1870423002/diff/1/lib/src/rules/final_overrid...
lib/src/rules/final_override_non_final.dart:39: Object something = 'ipsum';
Fixed

[pquitslund, 2016-04-11 16:20:31]

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

[pquitslund, 2016-04-11 16:20:32]

Fantastic.  Thanks for hacking on this!

I've added a few comments and added Brian Wilkerson who can see what I missed.

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules.dart
File lib/src/rules.dart (right):

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules.dart#newc...
lib/src/rules.dart:53: ..register(new FinalOverrideNonFinal())
I think a better name might be `OverridenField`?

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
File lib/src/rules/final_override_non_final.dart (right):

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:5: library
linter.src.rules.final_override_non_final;
As per the previous comment, I think a rule name like `overridden_field` may
read slightly better.

Open to other ideas though!

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:7: import
'package:linter/src/linter.dart';
We generally sort our members. (Alphabetically.)

In IntelliJ, that can be done using the menu Code>Sort Members in Dart File.

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:13: const desc = r'Field overriden';
To stay consistent with the other lint descriptions maybe change to: 

`Do not override fields.`

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:92: ;
Unneeded `;`?

[Alexei Diaz, 2016-04-11 17:30:23]

Description was changed from

==========
Hint if a final field "overrides" non-final field.
==========

to

==========
Implementation of the mentioned rule.
==========

[Alexei Diaz, 2016-04-11 17:30:50]

Thanks for the review!

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules.dart
File lib/src/rules.dart (right):

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules.dart#newc...
lib/src/rules.dart:53: ..register(new FinalOverrideNonFinal())
Done

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
File lib/src/rules/final_override_non_final.dart (right):

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:5: library
linter.src.rules.final_override_non_final;
Done

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:7: import
'package:linter/src/linter.dart';
I guess I overlooked it because I took for granted the dart formatter would sort
them.

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:13: const desc = r'Field overriden';
Done

https://codereview.chromium.org/1870423002/diff/20001/lib/src/rules/final_ove...
lib/src/rules/final_override_non_final.dart:92: ;
Done

[Brian Wilkerson, 2016-04-11 18:13:35]

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:83: final parentClass =
member.enclosingElement as ClassElement;
I believe that member can be null if the variable is marked as overriding
something but doesn't:

class Super {}

class Sub extends Super {
  @override
  int y;
}

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:85: if (field.name ==
node.fields.variables.first.name.name &&
"node.fields.variables.first"? Shouldn't that be "variable"? Otherwise I think
something like the following is broken:

class Super {
  int x, y;
}

class Sub extends Super {
  @override
  int y;
}

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:94: .where((v) => v?.element != null &&
v.element.isOverride)
Do you only want to catch cases where the field is marked as overriding an
inherited field? What about

class Super {
  int x;
}

class Sub extends Super {
  int x;
}

[Alexei Diaz, 2016-04-11 18:28:53]

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:83: final parentClass =
member.enclosingElement as ClassElement;
Fixed

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:85: if (field.name ==
node.fields.variables.first.name.name &&
Hopefully is fixed now, PTAL.

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:94: .where((v) => v?.element != null &&
v.element.isOverride)
I think
https://github.com/dart-lang/linter/blob/master/lib/src/rules/annotate_overri...
takes care of that case. Would it be better to add this verification there?

[Brian Wilkerson, 2016-04-11 18:55:31]

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:94: .where((v) => v?.element != null &&
v.element.isOverride)
It's possible for clients to enable this lint rule without enabling the
annotate_overrides rule, so you can't depend on them using it to ensure that the
field is marked as overriding another. I think we really want this rule to catch
all violations and not depend on the extra information, but that does require
more computational time because then you have to check every field in every
class.

https://codereview.chromium.org/1870423002/diff/80001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/80001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:89: if (node.fields.variables.any((v) =>
v.name.name == field.name) &&
This does too much work. For each field in the superclass, you look to see
whether any of the fields in the subclass' field declaration match. There's no
reason to loop through the list again.

That said, looking at it more I have to wonder why we need this code at all.
Seems like the method could be written as:

  ExecutableElement member = _getOverriddenMember(variable.element);
  if (member is PropertyAccessorElement && member.isSynthetic) {
    rule.reportLint(variable.name);
  }

That is: if there is an overridden member, and that member is a getter or setter
that is synthetic, that means that there is a field declared in the superclass
that this field is overriding.

But I haven't tested it. By the way, you should add the test cases I suggested
just to make sure the code handles them correctly. (More tests are better. IMO.)

[Alexei Diaz, 2016-04-11 19:17:02]

Thanks for the review, this looks much better now!

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/60001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:94: .where((v) => v?.element != null &&
v.element.isOverride)
Added the verification, my GUESS, is that the expensive computation was already
performed since I am only filtering, hopefully the extra cost is negligible.

https://codereview.chromium.org/1870423002/diff/80001/lib/src/rules/overriden...
File lib/src/rules/overriden_field.dart (right):

https://codereview.chromium.org/1870423002/diff/80001/lib/src/rules/overriden...
lib/src/rules/overriden_field.dart:89: if (node.fields.variables.any((v) =>
v.name.name == field.name) &&
Done

[Brian Wilkerson, 2016-04-11 20:02:00]

lgtm