Re: Change in sdk[master]: [VM] Support patching of fields.

[Vyacheslav Egorov]

I would talk to Peter tomorrow about this (patched_sdk is a legacy way of patching thing. we should have already switched to fasta for generating patched SDK).

It would help if you could upload some sort of reproduction patch of what you are trying to patch which causes it to break like this so that we have something as a reference. 

// Vyacheslav Egorov

On Thu, Jan 18, 2018 at 8:14 PM, Régis Crelier (Gerrit) <noreply-gerritcoderevie...@google.com> wrote:

Patch Set 3:

DBC: It might be good to test if this sort of patching already works with CFE. Because if it does not then Dart 2 builds will break.

Good call!
If I try to patch a field in the core lib, CFE complains:

FAILED: patched_sdk/lib/libraries.json
python ../../tools/patch_sdk.py --quiet vm /usr/local/google/home/regis/dart4/sdk/sdk /usr/local/google/home/regis/dart4/sdk/out/DebugX64/gen/runtime/vm/patches patched_sdk /usr/local/google/home/regis/dart4/sdk/.packages
Unhandled exception:
NoSuchMethodError: Class 'FieldDeclarationImpl' has no instance getter 'name'.
Receiver: Instance of 'FieldDeclarationImpl'
Tried calling: name
#0 Object.noSuchMethod (dart:core-patch/dart:core/object_patch.dart:46)
#1 _qualifiedName (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:653:32)
#2 PatchFinder.visitClassDeclaration (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:621:19)
#3 ClassDeclarationImpl.accept (package:analyzer/src/dart/ast/ast.dart:1906:49)
#4 NodeListImpl.accept (package:analyzer/src/dart/ast/ast.dart:8050:20)
#5 CompilationUnitImpl.visitChildren (package:analyzer/src/dart/ast/ast.dart:2502:21)
#6 GeneralizingAstVisitor.visitNode (package:analyzer/dart/ast/visitor.dart:431:10)
#7 GeneralizingAstVisitor.visitCompilationUnit (package:analyzer/dart/ast/visitor.dart:219:51)
#8 new PatchFinder.parseAndVisit (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:607:5)
#9 _patchLibrary (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:455:27)
#10 _applyPatch (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:412:20)
#11 _main (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:118:5)
<asynchronous suspension>
#12 main (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:62:11)
<asynchronous suspension>
#13 _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:275)
#14 _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:163)

What is your feeling? Is it difficult to support field patching in the CFE? If so, we should replace the BigInt fields we need to patch with getters and forget about field patching.

View Change

To view, visit change 35384. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: master

Gerrit-MessageType: comment

Gerrit-Change-Id: Ida19f3542b24899a306cce0c5a1ee625f578e675

Gerrit-Change-Number: 35384

Gerrit-PatchSet: 3

Gerrit-Owner: Régis Crelier <re...@google.com>

Gerrit-Reviewer: Régis Crelier <re...@google.com>

Gerrit-Reviewer: Siva Annamalai <as...@google.com>

Gerrit-CC: Dart Reviews <rev...@dartlang.org>

Gerrit-CC: Vyacheslav Egorov <veg...@google.com>

Gerrit-CC: commi...@chromium.org <commi...@chromium.org>

Gerrit-Comment-Date: Thu, 18 Jan 2018 19:14:52 +0000

Gerrit-HasComments: No

Gerrit-HasLabels: No

[Siva Annamalai]

​The current version you have (does not require field patching) seems easier as we don't have an additional requirement from the FE team (they already have a number of issues to work on)​. Why don't we go with it.

On Thu, Jan 18, 2018 at 5:30 PM, Régis Crelier (Gerrit) <noreply-gerritcoderevie...@google.com> wrote:

Patch Set 3:

Slava,

As you requested, I uploaded a cl containing both this cl and an example of field patching that fails under CFE, but works under the VM:
https://dart-review.googlesource.com/#/c/sdk/+/35804

The new file runtime/lib/bigint_patch.dart is simply a copy of sdk/lib/core/bigint.dart where private classes named "_BigInt..." are renamed to "_BigInteger..." and where public class BigInt is patched to refer to the renamed class _BigIntegerImpl.
This is only an example and not to be committed.

Thanks,
Regis

In case we decide not to support field patching, we can solve the issue by using getters. This cl (https://dart-review.googlesource.com/#/c/sdk/+/35521) shows how to do that and how to provide different implementations of the new BigInt class for each of the VM, dart2js, and devc.

View Change

To view, visit change 35384. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: master

Gerrit-MessageType: comment

Gerrit-Change-Id: Ida19f3542b24899a306cce0c5a1ee625f578e675

Gerrit-Change-Number: 35384

Gerrit-PatchSet: 3

Gerrit-Owner: Régis Crelier <re...@google.com>

Gerrit-Reviewer: Régis Crelier <re...@google.com>

Gerrit-Reviewer: Siva Annamalai <as...@google.com>

Gerrit-CC: Dart Reviews <rev...@dartlang.org>

Gerrit-CC: Vyacheslav Egorov <veg...@google.com>

Gerrit-CC: commi...@chromium.org <commi...@chromium.org>

Gerrit-Comment-Date: Fri, 19 Jan 2018 01:30:20 +0000

Gerrit-HasComments: No

Gerrit-HasLabels: No

[Régis Crelier]

Sounds good to me.

On Thu, Jan 18, 2018 at 5:58 PM, Siva Annamalai <as...@google.com> wrote:

​The current version you have (does not require field patching) seems easier as we don't have an additional requirement from the FE team (they already have a number of issues to work on)​. Why don't we go with it.