Support for DartBlockBody, DartExpressionBody, DartOmit in java2dart.

Support for DartBlockBody, DartExpressionBody, DartOmit in java2dart.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2014-03-07 16:12:31]

DBC

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java:564:
@DartBlockBody({"_removedContexts[context] = true;"})
I don't understand how this is suppose to work. We don't have weak references in
Dart, so these entries will never be removed. Seems like we need a bigger
re-write than these two methods.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationProcessor.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationProcessor.java:194:
@DartBlockBody({})
If a method does nothing on the Dart side, don't we want to remove it and any
invocations of it?

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationQueue.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationQueue.java:168:
@DartBlockBody({"operations.removeWhere((_) =>
_.removeWhenSourceRemoved(source));"})
Is this re-write for performance, or is there some other reason for it? I think
we should minimize the number of re-writes because they will cause problems when
we rename methods and fields.

nit: I don't like using "_" as an identifier. How about "operation"? (It takes
less of my mental energy to understand the code that way.)

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java:42:
public class DirectoryBasedDartSdk implements DartSdk {
Is the goal here to be able to translate this class (because I thought we had
simply replaced it before)? If so, I don't think that will work very well. We
need to be able to do this using async IO, which I think will require bigger
changes than the translator can handle.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:40:
public class FileBasedSource implements Source {
Is the goal here to be able to translate this class (because I thought we had
simply replaced it before)?

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:182:
@DartBlockBody({"return new TimestampedData<String>(_file.lastModified(),
_file.readAsStringSync());"})
This needs to be re-written to use asynchronous IO (which can't happen quite
yet), so I don't understand the value of this re-write.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:241:
@DartBlockBody({"throw new UnsupportedOperationException();"})
Shouldn't we just omit this method?

https://codereview.chromium.org/189433006/diff/1/editor/util/plugins/com.goog...
File
editor/util/plugins/com.google.dart.java2dart/src/com/google/dart/java2dart/engine/EngineAnnotationProcessor.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/util/plugins/com.goog...
editor/util/plugins/com.google.dart.java2dart/src/com/google/dart/java2dart/engine/EngineAnnotationProcessor.java:65:
removeMethodInvocations((MethodDeclaration) node);
I'm skeptical about having the translator remove any statement containing a
reference to an omitted method. We could be having a much more serious impact on
the resulting code than we ever intended. We might be better off explicitly
re-writing those references.

But if we do decide that auto-removal of code is a good thing, then I think we
also need to handle references to omitted fields as well.

https://codereview.chromium.org/189433006/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/189433006/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/parser.dart:8563: class ToFormattedSourceVisitor
implements AstVisitor<Object> {
I don't understand why this class is here. It is part of the java2dart
translator, not part of the engine.

[scheglov, 2014-03-07 22:47:43]

PTAL

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java:564:
@DartBlockBody({"_removedContexts[context] = true;"})
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> I don't understand how this is suppose to work. We don't have weak references
in
> Dart, so these entries will never be removed. Seems like we need a bigger
> re-write than these two methods.

Done.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationProcessor.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationProcessor.java:194:
@DartBlockBody({})
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> If a method does nothing on the Dart side, don't we want to remove it and any
> invocations of it?

OK, I will just remove the whole class then.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationQueue.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/operation/OperationQueue.java:168:
@DartBlockBody({"operations.removeWhere((_) =>
_.removeWhenSourceRemoved(source));"})
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> Is this re-write for performance, or is there some other reason for it? I
think
> we should minimize the number of re-writes because they will cause problems
when
> we rename methods and fields.
> 
> nit: I don't like using "_" as an identifier. How about "operation"? (It takes
> less of my mental energy to understand the code that way.)

It is much better to use idiomatic Dart implementation - it is easier and more
performant.
Our JavaInterator implementation is not so good.

Anyway, I will just remove the whole class.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java:42:
public class DirectoryBasedDartSdk implements DartSdk {
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> Is the goal here to be able to translate this class (because I thought we had
> simply replaced it before)? If so, I don't think that will work very well. We
> need to be able to do this using async IO, which I think will require bigger
> changes than the translator can handle.

I'd like to keep this class as is.
This CL is mostly about adding support for annotations and using them to clean
up some places, not to add all possible annotations.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:40:
public class FileBasedSource implements Source {
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> Is the goal here to be able to translate this class (because I thought we had
> simply replaced it before)?

Yes, we have been translating it for a long time.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:182:
@DartBlockBody({"return new TimestampedData<String>(_file.lastModified(),
_file.readAsStringSync());"})
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> This needs to be re-written to use asynchronous IO (which can't happen quite
> yet), so I don't understand the value of this re-write.

This just maintains the status quo, but using a different way to archive it -
annotation vs. Engine semantic processor.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:241:
@DartBlockBody({"throw new UnsupportedOperationException();"})
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> Shouldn't we just omit this method?

Yes, we can.
Done.

https://codereview.chromium.org/189433006/diff/1/editor/util/plugins/com.goog...
File
editor/util/plugins/com.google.dart.java2dart/src/com/google/dart/java2dart/engine/EngineAnnotationProcessor.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/util/plugins/com.goog...
editor/util/plugins/com.google.dart.java2dart/src/com/google/dart/java2dart/engine/EngineAnnotationProcessor.java:65:
removeMethodInvocations((MethodDeclaration) node);
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> I'm skeptical about having the translator remove any statement containing a
> reference to an omitted method. We could be having a much more serious impact
on
> the resulting code than we ever intended. We might be better off explicitly
> re-writing those references.
> 
> But if we do decide that auto-removal of code is a good thing, then I think we
> also need to handle references to omitted fields as well.

OK, we actually don't have to remove _checkThread() method, we need just to
replace it with an empty body.

https://codereview.chromium.org/189433006/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/189433006/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/parser.dart:8563: class ToFormattedSourceVisitor
implements AstVisitor<Object> {
On 2014/03/07 16:12:31, Brian Wilkerson wrote:
> I don't understand why this class is here. It is part of the java2dart
> translator, not part of the engine.

You are right.
I used it once about a year ago, but we don't need it in production.
Removed.

[Brian Wilkerson, 2014-03-07 23:12:06]

LGTM

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/sdk/DirectoryBasedDartSdk.java:42:
public class DirectoryBasedDartSdk implements DartSdk {
> I'd like to keep this class as is.
> This CL is mostly about adding support for annotations and using them to clean
> up some places, not to add all possible annotations.

Sorry, I guess I wasn't very clear. I wasn't saying that you should add more
annotations, I was suggesting that we don't need any annotations because in a
couple of days the whole class will be re-written in Dart and we won' be
translating it from Java at all.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java
(right):

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:40:
public class FileBasedSource implements Source {
> Yes, we have been translating it for a long time.

I didn't know that. I thought we had hand-written the replacement. That said, I
think we will soon be hand-coding a replacement for it and won't need to
translate it any more.

https://codereview.chromium.org/189433006/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:182:
@DartBlockBody({"return new TimestampedData<String>(_file.lastModified(),
_file.readAsStringSync());"})
Ok. That's reasonable.

[scheglov, 2014-03-07 23:53:54]

Committed patchset #2 manually as r33468 (presubmit successful).