The very first bits of resynthesizing elements from kernels.

The very first bits of resynthesizing elements from kernels.

Just parts of library, defining unit and class for now.
This is enough to pass 15 tests out of 522 :-)
Although some of them are passing accidentally.

Only strong mode tests for now.

We don't compare with Analyzer elements created using Analyzer builders,
because these builders will go away. But we miss testing for things
like name offsets.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/0d9357431d84ff853e10dcc10ce1be3b4f3a1ead

[Paul Berry, 2017-07-13 19:29:40]

Please get Siggi or Peter's approval on the addition of
temporaryUntilPrintsRemovedEnablePrinting.

Assuming one of them is ok with it, lgtm.

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:9000: //  String get name;
Would you mind adding a TODO comment indicating what you expect to happen to
this commented out code?

(Removing the commented out code would be ok as well)

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart (right):

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart:97: //  @override
Is this commented out because there's still work to do here, or did you intend
to delete this?

If there's still work to do here, consider doing this instead:

@override
void declareConstantVariable(String name, String value) {
  // TODO(scheglov): ...summary of the work that needs to be done...
  super.declareConstantVariable(name, value);
}

[Siggi Cherem (dart-lang), 2017-07-13 21:20:54]

Konstantin - I was hoping we could avoid on the
'temporaryUntilPrintsRemovedEnablePrinting' flag, because you will be able to do
without it likely tomorrow.

In particular, tomorrow Peter will likely land CL
https://codereview.chromium.org/2977013002/ that removes all prints except in
one place. As soon as that lands, there is a way to override it programatically,
so you wont need the flag altogether.

We need a couple steps to integrate with Peter's change:
   - provide a CompilerContext when using fasta from the kernel-driver
   - override the `report` and `reportWithoutLocation` options with
implementations that don't print.

Note that IKG and kernel driver are not setting a CompilerContext before
invoking fasta, but this is actually necessary to do. I also didn't know it was
necessary until I added the public APIs last week. You can see how I added the
context here:

 
https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/kernel_gen...


This can be a bit awkward to set up and my intend is to improve this by
integrating ProcessedOptiosn and CompilerContext together. When that is working,
removing "prints" should be done by setting the `onError` handler in the
CompilerOptions object.

[scheglov, 2017-07-13 21:24:08]

A quick comment about the 'temporaryUntilPrintsRemovedEnablePrinting' flag.

If we don't add it even temporary, analyzer tests will spam garbage on the
console. So, I will have to choose between landing and continuing working, but
seeing garbage; or waiting until your CL is landed and not moving kernel
integration forward :-(

[Siggi Cherem (dart-lang), 2017-07-13 22:17:42]

On 2017/07/13 21:24:08, scheglov wrote:
> A quick comment about the 'temporaryUntilPrintsRemovedEnablePrinting' flag.
> 
> If we don't add it even temporary, analyzer tests will spam garbage on the
> console. So, I will have to choose between landing and continuing working, but
> seeing garbage; or waiting until your CL is landed and not moving kernel
> integration forward :-(

Would you be OK to temporarily have this flag only on a local patch until
tomorrow? (so you don't have to see garbage while you are working on this?)

[scheglov, 2017-07-14 05:19:07]

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:9000: //  String get name;
On 2017/07/13 19:29:39, Paul Berry wrote:
> Would you mind adding a TODO comment indicating what you expect to happen to
> this commented out code?
> 
> (Removing the commented out code would be ok as well)

I removed the code.

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart (right):

https://codereview.chromium.org/2980863003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart:97: //  @override
On 2017/07/13 19:29:40, Paul Berry wrote:
> Is this commented out because there's still work to do here, or did you intend
> to delete this?
> 
> If there's still work to do here, consider doing this instead:
> 
> @override
> void declareConstantVariable(String name, String value) {
>   // TODO(scheglov): ...summary of the work that needs to be done...
>   super.declareConstantVariable(name, value);
> }

I will remove this code for now.

[scheglov, 2017-07-14 05:20:31]

OK, I rolled back the "temporaryUntilPrintsRemovedEnablePrinting" flag. It is OK
to see some garbage for a day or two.

[scheglov, 2017-07-14 05:21:00]

Description was changed from

==========
The very first bits of resynthesizing elements from kernels.

Just parts of library, defining unit and class for now.
This is enough to pass 15 tests out of 522 :-)
Although some of them are passing accidentally.

Only strong mode tests for now.

We don't compare with Analyzer elements created using Analyzer builders,
because these builders will go away. But we miss testing for things
like name offsets.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
The very first bits of resynthesizing elements from kernels.

Just parts of library, defining unit and class for now.
This is enough to pass 15 tests out of 522 :-)
Although some of them are passing accidentally.

Only strong mode tests for now.

We don't compare with Analyzer elements created using Analyzer builders,
because these builders will go away. But we miss testing for things
like name offsets.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/0d9357431d84ff853e10dcc10ce1be3b4f3a1ead
==========

[scheglov, 2017-07-14 05:21:02]

Committed patchset #2 (id:20001) manually as
0d9357431d84ff853e10dcc10ce1be3b4f3a1ead (presubmit successful).

[ahe, 2017-07-14 12:26:29]

I have landed CL 2977013002 now.

For future reference, you can use zones to silence print, see
https://www.dartlang.org/articles/libraries/zones#example-overriding-print

[Siggi Cherem (dart-lang), 2017-07-14 15:42:26]

BTW just sent https://codereview.chromium.org/2980093002/ which should take care
of the undesired printing.

[scheglov, 2017-07-14 15:49:46]

On 2017/07/14 15:42:26, Siggi Cherem (dart-lang) wrote:
> BTW just sent https://codereview.chromium.org/2980093002/ which should take
care
> of the undesired printing.

Thanks!