Warn about overriding operator== but not hashCode.

Warn about overriding operator== but not hashCode.

R=kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24463

[ahe, 2013-06-24 13:09:17]

I'd like a full review now.  I think the CL is ready to commit (pending
objections from Paul and Nicolas).

Paul: Please notice that I have marked cpa_inference_test as Fail.  I'm not sure
what is wrong, and I assume you can take a look next Thursday.  Let me know if
you feel otherwise.

Nicolas: Please notice that I have commented out a test in
call_site_simple_type_inferer_test.dart.  I'm not sure what is wrong or how to
debug it.  I hope you can take a look.

[polux, 2013-06-24 13:41:38]

On 2013/06/24 13:09:17, ahe wrote:
> I'd like a full review now.  I think the CL is ready to commit (pending
> objections from Paul and Nicolas).
> 
> Paul: Please notice that I have marked cpa_inference_test as Fail.  I'm not
sure
> what is wrong, and I assume you can take a look next Thursday.  Let me know if
> you feel otherwise.
> 

I'll have a look next Thursday.

> Nicolas: Please notice that I have commented out a test in
> call_site_simple_type_inferer_test.dart.  I'm not sure what is wrong or how to
> debug it.  I hope you can take a look.

[ahe, 2013-06-24 19:17:53]

https://codereview.chromium.org/17588005/diff/2001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/analyze_dart2js_test.dart (right):

https://codereview.chromium.org/17588005/diff/2001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/analyze_dart2js_test.dart:23:
'sdk/lib/_internal/compiler/implementation/constants.dart':
These will be removed in an updated version of CL 17569004.

[ngeoffray, 2013-06-24 19:56:14]

Code looks good, but personally I think that making it a lint thing is a bit
extreme. I'd prefer a command line flag --warn-on-hashCode. There are some cases
(as seen in the other CL), where you write classes with relational operators,
and you don't want to be bothered on implementing hashCode, since that's not the
purpose of your code....

https://codereview.chromium.org/17588005/diff/2001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/call_site_simple_type_inferer_test.dart
(right):

https://codereview.chromium.org/17588005/diff/2001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/call_site_simple_type_inferer_test.dart:254: //
runTest(TEST_10, (inferrer) => [subclassOfInterceptor(inferrer),
That's an ordering issue in the inferrer. Paul had the same problem last week.
I'm fixing it.

[ahe, 2013-06-24 20:34:09]

On 2013/06/24 19:56:14, ngeoffray wrote:
> Code looks good, but personally I think that making it a lint thing is a bit
> extreme. I'd prefer a command line flag --warn-on-hashCode. There are some
cases
> (as seen in the other CL), where you write classes with relational operators,
> and you don't want to be bothered on implementing hashCode, since that's not
the
> purpose of your code....

This is from Object.hashCode:

   * All objects have hash codes. Hash codes are guaranteed to be the
   * same for objects that are equal when compared using the equality
   * operator [:==:]. Other than that there are no guarantees about

As soon as you override operator== without overriding hashCode, you violate that
contract, and that leads to very subtle bugs that are hard to debug.  When you
violate this contract you should at least throw an exception.

That I have to debate this point with you only strengthens my resolve that this
"lint" is the correct thing to turn on by default.  You're a great programmer,
but you're inadvertently leaving a ticking bomb in your code.

[ahe, 2013-06-25 15:36:40]

I think we have a compromise now:

This diagnostic is not a "warning", it is a "hint".  It gets a different color
from warnings, and can be suppressed using "--suppress-hints".

Kasper argues that it is better to turn hints on by default, as our users won't
see them otherwise.  I think this is a great observation.  I still don't think
Nicolas likes the hint.

Separately, Kasper is considering discussing a language change in the next
language meeting: if you override operator==, but not hashCode, you get a
hashCode implementation that throws an exception.

PTAL

[kasperl, 2013-06-26 09:22:27]

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_api_test.dart:24: const ['Warning: Using
"new Symbol"', // Issue 10565.
Should these be updated to hints?

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_dart2js_test.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_dart2js_test.dart:23:
'sdk/lib/_internal/compiler/implementation/constants.dart':
Warning -> Hint?

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_helper.dart:109: // Suppress white listed
warnings.
warnings -> hints.

[ahe, 2013-06-26 10:45:51]

PTAL

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_api_test.dart:24: const ['Warning: Using
"new Symbol"', // Issue 10565.
On 2013/06/26 09:22:28, kasperl wrote:
> Should these be updated to hints?

Done.

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_dart2js_test.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_dart2js_test.dart:23:
'sdk/lib/_internal/compiler/implementation/constants.dart':
On 2013/06/26 09:22:28, kasperl wrote:
> Warning -> Hint?

Done.

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/17588005/diff/10001/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_helper.dart:109: // Suppress white listed
warnings.
On 2013/06/26 09:22:28, kasperl wrote:
> warnings -> hints.

Done.

[kasperl, 2013-06-26 10:51:21]

LGTM.

[ahe, 2013-06-26 11:20:21]

Committed patchset #4 manually as r24463 (presubmit successful).

[Johnni Winther, 2013-06-28 14:14:55]

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_api_test.dart:26:
'sdk/lib/html/dart2js/html_dart2js.dart':
Have these been reported as bugs? (Please add issue number)

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_dart2js_test.dart (right):

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_dart2js_test.dart:23:
'sdk/lib/_internal/compiler/implementation/constants.dart':
Nice catch!

[ahe, 2013-06-28 17:59:26]

Thank you, Johnni.

I'll follow up with another CL.

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
File dart/tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/17588005/diff/25002/dart/tests/compiler/dart2...
dart/tests/compiler/dart2js/analyze_api_test.dart:26:
'sdk/lib/html/dart2js/html_dart2js.dart':
On 2013/06/28 14:14:55, Johnni Winther wrote:
> Have these been reported as bugs? (Please add issue number)

Done.