Check for cyclic reference in typedefs

Check for cyclic reference in typedefs

This CL introduces a post-processing queue to the resolution phase which is processing immediately after the resolution queue has been closed. 

The cyclic check of typedefs uses this post-processing queue to register the checking function during TypedefResolverVisitor.

R=ahe@google.com, karlklose@google.com

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

[ahe, 2013-04-02 08:22:39]

LGTM!

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:88: typedef void
PostProcess();
I'd call this PostProcessAction or VoidFunction.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:90: class PostProcessing
{
I'd call this PostProcessTask.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_types.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_types.dart:897: abstract class
DartTypeTraversal extends DartTypeVisitor {
As I understand it, there is one subclass of this class.  That subclass is
temporary.  So one could get the feeling that this class doesn't pull its own
weight.

Also, did you consider the visitChildren approach from AST nodes?

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3011:
visitor.visit(type);
This is probably slightly faster:

type.accept(visitor, null);

Generally, the visit method becomes polymorphic, but type.accept is monomorphic
here.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3040: //
Direct cyclicity.
I don't think "cyclicity" is a word.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3044: //
Cyclicity through one other typedef.
"cyclicity"

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3051:
compiler.reportErrorCode(element,
I'd drop this error code and use:

for (TypedefElement cycle in seenTypedefs) {
  compiler.reportErrorCode(element, MessageKind.CYCLIC_TYPEDEF_ONE, ...);
}

https://codereview.chromium.org/13261008/diff/2001/tests/language/cyclic_type...
File tests/language/cyclic_typedef_test.dart (right):

https://codereview.chromium.org/13261008/diff/2001/tests/language/cyclic_type...
tests/language/cyclic_typedef_test.dart:5: typedef /*A();*/
These comments aren't helping.  I think it would be better if you added some
prose to describe what you're doing and removed the comments /*typedef*/, etc.

[ahe, 2013-04-02 08:25:46]

One more thing: the explanation you have in the CL description is nice. Do you
think you could turn it into a comment somewhere in the code?

[Johnni Winther, 2013-04-02 09:40:14]

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:88: typedef void
PostProcess();
On 2013/04/02 08:22:39, ahe wrote:
> I'd call this PostProcessAction or VoidFunction.

PostProcessAction it is.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:90: class PostProcessing
{
On 2013/04/02 08:22:39, ahe wrote:
> I'd call this PostProcessTask.

Done.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_types.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_types.dart:897: abstract class
DartTypeTraversal extends DartTypeVisitor {
On 2013/04/02 08:22:39, ahe wrote:
> As I understand it, there is one subclass of this class.  That subclass is
> temporary.  So one could get the feeling that this class doesn't pull its own
> weight.
> 
> Also, did you consider the visitChildren approach from AST nodes?

Changed to use the visitChildren approach.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3011:
visitor.visit(type);
On 2013/04/02 08:22:39, ahe wrote:
> This is probably slightly faster:
> 
> type.accept(visitor, null);
> 
> Generally, the visit method becomes polymorphic, but type.accept is
monomorphic
> here.

Done.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3040: //
Direct cyclicity.
On 2013/04/02 08:22:39, ahe wrote:
> I don't think "cyclicity" is a word.

It is! (and not just in my vocabulary: http://en.wiktionary.org/wiki/cyclicity)

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3044: //
Cyclicity through one other typedef.
On 2013/04/02 08:22:39, ahe wrote:
> "cyclicity"

Ditto.

https://codereview.chromium.org/13261008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3051:
compiler.reportErrorCode(element,
On 2013/04/02 08:22:39, ahe wrote:
> I'd drop this error code and use:
> 
> for (TypedefElement cycle in seenTypedefs) {
>   compiler.reportErrorCode(element, MessageKind.CYCLIC_TYPEDEF_ONE, ...);
> }

Done.

https://codereview.chromium.org/13261008/diff/2001/tests/language/cyclic_type...
File tests/language/cyclic_typedef_test.dart (right):

https://codereview.chromium.org/13261008/diff/2001/tests/language/cyclic_type...
tests/language/cyclic_typedef_test.dart:5: typedef /*A();*/
On 2013/04/02 08:22:39, ahe wrote:
> These comments aren't helping.  I think it would be better if you added some
> prose to describe what you're doing and removed the comments /*typedef*/, etc.

It looked good in Sublime Text 2!

Changed.

[Johnni Winther, 2013-04-19 09:40:47]

PTAL

[Johnni Winther, 2013-09-03 08:57:30]

PTAL

[Johnni Winther, 2013-09-06 06:09:12]

ping

[Johnni Winther, 2013-09-17 07:50:45]

ping

[karlklose, 2013-09-17 08:56:15]

LGTM.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3273: } else
if (seenTypedefs.tail.tail.isEmpty) {
It would be easier to read if you computed (or tracked) the length of this list
and compared against a number here.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3292: }
Please use an else block here. The return is easy to miss.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:394: static const
CYCLIC_TYPEDEF = const MessageKind(
Please add a '.' to the end of the messages.

I think the error messages should point out the problem, like "a typedef can't
refer to itself." and you should provide a howToFix.

Also see Kathy and Peter's comments here:
https://codereview.chromium.org/23456030/

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:402: "Internal Error:
Recursive type variable bounds are not currently "
Please remove 'currently'.

[ahe, 2013-09-17 09:40:18]

LGTM (please address Karl's comments about diagnostics).

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3238:
checkCyclicReference);
Weird indentation.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3248: int
seenBoundsCount = 0;
Please document this field.

[Johnni Winther, 2013-09-18 07:57:14]

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3238:
checkCyclicReference);
On 2013/09/17 09:40:18, ahe wrote:
> Weird indentation.

Done.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3248: int
seenBoundsCount = 0;
On 2013/09/17 09:40:18, ahe wrote:
> Please document this field.

Done.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3273: } else
if (seenTypedefs.tail.tail.isEmpty) {
On 2013/09/17 08:56:15, karlklose wrote:
> It would be easier to read if you computed (or tracked) the length of this
list
> and compared against a number here.

Done.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3292: }
On 2013/09/17 08:56:15, karlklose wrote:
> Please use an else block here. The return is easy to miss.

Done.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:394: static const
CYCLIC_TYPEDEF = const MessageKind(
On 2013/09/17 08:56:15, karlklose wrote:
> Please add a '.' to the end of the messages.
> 
> I think the error messages should point out the problem, like "a typedef can't
> refer to itself." and you should provide a howToFix.
> 
> Also see Kathy and Peter's comments here:
> https://codereview.chromium.org/23456030/

Done.

https://codereview.chromium.org/13261008/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:402: "Internal Error:
Recursive type variable bounds are not currently "
On 2013/09/17 08:56:15, karlklose wrote:
> Please remove 'currently'.

Done.

[ahe, 2013-09-18 07:58:25]

Adding Kathy and Luke to look at the error messages (if they have time).
See the file called warnings.dart.

[Johnni Winther, 2013-09-18 08:43:34]

Committed patchset #10 manually as r27595 (presubmit successful).