Skip to content

Commit

Permalink
[dart2js] Clone call receiver for protobuf replacement placeholder.
Browse files Browse the repository at this point in the history
Adding a reference to node.receiver in the replacement placeholder was modifying its parent pointer eagerly. When running Dart2js with serialization the pointers were reset by the serialization process after the closed world phase. However, when running without serialization the pointer stayed corrupted and lead to a malfomed tree in later phases. This manifested as an error trying to lookup the location (i.e. the kernel Location) of the receiver.

This fixes the issue by cloning the receiver of the call for the placeholder. This avoids updating the original receiver's parent pointer.

Today the receiver is always a VariableGet referencing a variable defined in a surrounding Let scope (this is how the CFE encodes x..a()..b()). We could cast the receiver but to be a bit more resilient I've opted to clone the node. The cloner in the kernel package fails on free variable (i.e. if the variable is not declared in the cloning scope). But in this case we want to simply use the same declaration reference from the cloned node. I've added a cloner that provides that fallback behavior.

I also considered using a closure to lazily create the replacement but closures are expensive and we'd be creating one per field per proto which would be a considerable cost for large programs. There wasn't really a clean way to do the same with a static tearoff.

Change-Id: I7ef0e24a9457cfa9f2e116eb3569b0652d321c08
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352563
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
  • Loading branch information
natebiggs authored and Commit Queue committed Feb 15, 2024
1 parent 6cfb95c commit 276aa29
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion pkg/compiler/lib/src/ir/protobuf_impacts.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.

import 'package:kernel/ast.dart' as ir;
import 'package:kernel/clone.dart' as ir;
import 'package:kernel/type_algebra.dart' as ir;

import '../kernel/element_map.dart';
Expand Down Expand Up @@ -139,7 +140,7 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
ir.InstanceInvocation node) {
return ir.InstanceInvocation(
ir.InstanceAccessKind.Instance,
node.receiver,
_CloneVisitorLenientVariables().clone(node.receiver),
_builderInfoAddMethod.name,
ir.Arguments(
<ir.Expression>[
Expand Down Expand Up @@ -207,3 +208,12 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
source: node, replacement: _buildProtobufMetadataPlaceholder(node)));
}
}

/// Clones nodes but returns same variable declaration on VariableGet if the
/// declaration is not in scope.
class _CloneVisitorLenientVariables extends ir.CloneVisitorNotMembers {
@override
ir.VariableDeclaration getVariableClone(ir.VariableDeclaration variable) {
return super.getVariableClone(variable) ?? variable;
}
}

0 comments on commit 276aa29

Please sign in to comment.