[M] Change in dart/sdk[main]: [js_ast] Add annotations facility
0 views
Skip to first unread message
Stephen Adams (Gerrit)
Feb 6, 2023, 10:11:55 PM2/6/23
to Nate Biggs, Sigmund Cherem, rev...@dartlang.org
Attention is currently required from: Nate Biggs, Sigmund Cherem.
Stephen Adams would like Nate Biggs and Sigmund Cherem to review this change.
[js_ast] Add annotations facility
Change-Id: Id9012b303de0d2b3848a635bc34747f8c5101236
---
M pkg/js_ast/lib/src/nodes.dart
A pkg/js_ast/test/annotations_test.dart
2 files changed, 221 insertions(+), 20 deletions(-)
diff --git a/pkg/js_ast/lib/src/nodes.dart b/pkg/js_ast/lib/src/nodes.dart
index 264dfb8..02fc668 100644
--- a/pkg/js_ast/lib/src/nodes.dart
+++ b/pkg/js_ast/lib/src/nodes.dart
@@ -561,10 +561,33 @@
}
abstract class Node {
- JavaScriptNodeSourceInformation? get sourceInformation => _sourceInformation;
-
+ /// Field for storing source information and other annotations.
+ ///
+ /// Source information is common but not universal, so missing source
+ /// information is represented by `null`. Annotations are uncommon. As a
+ /// space-saving measure we pack both kinds of information into one field by
+ /// storing the user's JavaScriptNodeSourceInformation object directly in the
+ /// field if there are no annotations. If there are annotations, the field is
+ /// 'expanded' by having it reference an internal
+ /// [_SourceInformationAndAnnotations] object which has two fields.
JavaScriptNodeSourceInformation? _sourceInformation;
+ /// Returns the source information associated with this node.
+ JavaScriptNodeSourceInformation? get sourceInformation {
+ final source = _sourceInformation;
+ return source is _SourceInformationAndAnnotations
+ ? source._sourceInformation
+ : source;
+ }
+
+ /// Returns a list of annotations attached to this node.
+ List<Object> get annotations {
+ final source = _sourceInformation;
+ return source is _SourceInformationAndAnnotations
+ ? source._annotations
+ : const [];
+ }
+
T accept<T>(NodeVisitor<T> visitor);
void visitChildren<T>(NodeVisitor<T> visitor);
@@ -573,22 +596,44 @@
/// Shallow clone of node.
///
- /// Does not clone positions since the only use of this private method is
- /// create a copy with a new position.
+ /// Does not clone [_sourceInformation] since the only use of this private
+ /// method is create a copy with a new source information or annotations.
Node _clone();
- /// Returns a node equivalent to [this], but with new source position and end
- /// source position.
+ /// Returns a node equivalent to [this], but with new source position.
Node withSourceInformation(
- JavaScriptNodeSourceInformation? sourceInformation) {
- if (sourceInformation == _sourceInformation) {
- return this;
- }
- Node clone = _clone();
- // TODO(sra): Should existing data be 'sticky' if we try to overwrite with
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ if (!_shouldReplaceSourceInformation(newSourceInformation)) return this;
+ return _clone()
+ .._sourceInformation =
+ _replacementSourceInformation(newSourceInformation);
+ }
+
+ bool _shouldReplaceSourceInformation(
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ // TODO(sra): Should existing data be 'sticky' if we try to update with
// `null`?
- clone._sourceInformation = sourceInformation;
- return clone;
+ return newSourceInformation != sourceInformation;
+ }
+
+ JavaScriptNodeSourceInformation? _replacementSourceInformation(
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ final source = _sourceInformation;
+ return source is _SourceInformationAndAnnotations
+ ? _SourceInformationAndAnnotations(
+ newSourceInformation, source._annotations)
+ : newSourceInformation;
+ }
+
+ /// Returns as node equivalent to [this] but with an additional annotation.
+ Node withAnnotation(Object newAnnotation) {
+ return _clone().._sourceInformation = _replacementAnnotation(newAnnotation);
+ }
+
+ _SourceInformationAndAnnotations _replacementAnnotation(
+ Object newAnnotation) {
+ return _SourceInformationAndAnnotations(
+ sourceInformation, List.unmodifiable([...annotations, newAnnotation]));
}
bool get isCommaOperator => false;
@@ -612,6 +657,14 @@
}
}
+class _SourceInformationAndAnnotations
+ implements JavaScriptNodeSourceInformation {
+ final JavaScriptNodeSourceInformation? _sourceInformation;
+ final List<Object> _annotations;
+ _SourceInformationAndAnnotations(this._sourceInformation, this._annotations)
+ : assert(_sourceInformation is! _SourceInformationAndAnnotations);
+}
+
class Program extends Node {
final List<Statement> body;
Program(this.body);
@@ -649,9 +702,11 @@
// Override for refined return type.
@override
Statement withSourceInformation(
- JavaScriptNodeSourceInformation? sourceInformation) {
- if (sourceInformation == _sourceInformation) return this;
- return _clone().._sourceInformation = sourceInformation;
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ if (!_shouldReplaceSourceInformation(newSourceInformation)) return this;
+ return _clone()
+ .._sourceInformation =
+ _replacementSourceInformation(newSourceInformation);
}
@override
@@ -1312,9 +1367,11 @@
// Override for refined return type.
@override
Expression withSourceInformation(
- JavaScriptNodeSourceInformation? sourceInformation) {
- if (sourceInformation == _sourceInformation) return this;
- return _clone().._sourceInformation = sourceInformation;
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ if (!_shouldReplaceSourceInformation(newSourceInformation)) return this;
+ return _clone()
+ .._sourceInformation =
+ _replacementSourceInformation(newSourceInformation);
}
@override
diff --git a/pkg/js_ast/test/annotations_test.dart b/pkg/js_ast/test/annotations_test.dart
new file mode 100644
index 0000000..02ee164
--- /dev/null
+++ b/pkg/js_ast/test/annotations_test.dart
@@ -0,0 +1,135 @@
+// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:js_ast/js_ast.dart';
+
+class Source implements JavaScriptNodeSourceInformation {
+ final Object tag;
+ Source(this.tag);
+
+ @override
+ String toString() => 'Source($tag)';
+}
+
+void check(Node node, expectedSource, expectedAnnotations) {
+ Expect.equals('$expectedSource', '${node.sourceInformation}', 'source');
+ Expect.equals('$expectedAnnotations', '${node.annotations}', 'annotations');
+}
+
+void simpleTests(Node node) {
+ check(node, null, []);
+
+ final s1 = node.withSourceInformation(Source(1));
+ check(node, null, []);
+ check(s1, Source(1), []);
+
+ final a1 = node.withAnnotation(1);
+ check(node, null, []);
+ check(s1, Source(1), []);
+ check(a1, null, [1]);
+
+ final a2 = node.withAnnotation(2);
+ check(node, null, []);
+ check(s1, Source(1), []);
+ check(a1, null, [1]);
+ check(a2, null, [2]);
+
+ final s1a3 = s1.withAnnotation(3);
+ check(node, null, []);
+ check(s1, Source(1), []);
+ check(s1, Source(1), []);
+ check(a2, null, [2]);
+ check(s1a3, Source(1), [3]);
+
+ final s1a3a4 = s1a3.withAnnotation(4);
+ check(node, null, []);
+ check(s1, Source(1), []);
+ check(s1, Source(1), []);
+ check(a2, null, [2]);
+ check(s1a3, Source(1), [3]);
+ check(s1a3a4, Source(1), [3, 4]);
+
+ final a2s5 = a2.withSourceInformation(Source(5));
+ check(node, null, []);
+ check(s1, Source(1), []);
+ check(s1, Source(1), []);
+ check(a2, null, [2]);
+ check(s1a3, Source(1), [3]);
+ check(s1a3a4, Source(1), [3, 4]);
+ check(a2s5, Source(5), [2]);
+}
+
+bool debugging = false;
+
+/// Explore all combinations of withSourceInformation and withAnnotation.
+void testGraph(Node root) {
+ // At each node in the graph, all the previous checks are re-run to ensure
+ // that source information or annotations do not change.
+ List<void Function(String state)> tests = [];
+
+ void explore(
+ String state,
+ Node node,
+ int sourceDepth,
+ int annotationDepth,
+ JavaScriptNodeSourceInformation? expectedSource,
+ List<Object> expectedAnnotations) {
+ void newCheck(String currentState) {
+ if (debugging) {
+ print('In state $currentState check $state:'
+ ' source: $expectedSource, annotations: $expectedAnnotations');
+ }
+ Expect.equals(
+ '$expectedSource',
+ '${node.sourceInformation}',
+ ' at state $currentState for node at state $state:'
+ ' ${node.debugPrint()}');
+ Expect.equals(
+ '$expectedAnnotations',
+ '${node.annotations}',
+ ' at state $currentState for node at state $state:'
+ ' ${node.debugPrint()}');
+ }
+
+ tests.add(newCheck);
+
+ for (final test in tests) {
+ test(state);
+ }
+
+ if (sourceDepth < 3) {
+ final newSourceDepth = sourceDepth + 1;
+ final newSource = Source(newSourceDepth);
+ final newState = '$state-s$newSourceDepth';
+ final newNode = node.withSourceInformation(newSource);
+
+ explore(newState, newNode, newSourceDepth, annotationDepth, newSource,
+ expectedAnnotations);
+ }
+ if (annotationDepth < 3) {
+ final newAnnotationDepth = annotationDepth + 1;
+ final newAnnotation = 'a:$newAnnotationDepth';
+ final newAnnotations = [...expectedAnnotations, newAnnotation];
+ final newState = '$state-a$newAnnotationDepth';
+ final newNode = node.withAnnotation(newAnnotation);
+
+ explore(newState, newNode, sourceDepth, newAnnotationDepth,
+ expectedSource, newAnnotations);
+ }
+ }
+
+ explore('root', root, 0, 0, null, []);
+}
+
+void main() {
+ simpleTests(js('x + 1'));
+ simpleTests(js.statement('f()'));
+
+ testGraph(js('1'));
+ testGraph(js('x + 1'));
+ testGraph(js('f()'));
+ testGraph(js.statement('f()'));
+ testGraph(js.statement('break'));
+}
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Adams (Gerrit)
Feb 6, 2023, 10:11:56 PM2/6/23
to rev...@dartlang.org, Nate Biggs, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Nate Biggs, Sigmund Cherem.
1 comment:
Patchset:
I will be using this facility to embed resource identifers in the generated JavaScript AST to generate a map from files to the resource identifiers that they contain.
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
CBuild (Gerrit)
Feb 6, 2023, 10:40:15 PM2/6/23
to Stephen Adams, rev...@dartlang.org, Nate Biggs, Sigmund Cherem, Commit Queue
Attention is currently required from: Nate Biggs, Sigmund Cherem.
go/dart-cbuild result: FAILURE (NO REGRESSIONS DETECTED)
Details: https://goto.google.com/dart-cbuild/find/ba4a63d7ae46707d57bfd4b02a92d2ec8088b543
Nate Biggs (Gerrit)
Feb 7, 2023, 10:19:34 AM2/7/23
to Stephen Adams, rev...@dartlang.org, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Sigmund Cherem, Stephen Adams.
2 comments:
File pkg/js_ast/lib/src/nodes.dart:
Patch Set #2, Line 584:
ObjectRather than a `List<Object>` should we have an explicit `AnnotationData` object with fields for the different annotations? I know if there are many annotations that could be more space-intensive than a `List`. But the number of nodes with annotations should be small enough that this shouldn't be a huge hit.
Patch Set #2, Line 633:
_replacementAnnotationnit: This should be `_appendAnnotation` rather than replace.
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
Sigmund Cherem (Gerrit)
Feb 7, 2023, 11:42:04 AM2/7/23
to Stephen Adams, rev...@dartlang.org, Nate Biggs, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Nate Biggs, Stephen Adams.
2 comments:
Patchset:
I will be using this facility to embed resource identifers in the generated JavaScript AST to genera […]
👍 - nit: copy this comment to the CL description too 😊
File pkg/js_ast/lib/src/nodes.dart:
Patch Set #2, Line 584:
Object
Rather than a `List<Object>` should we have an explicit `AnnotationData` object with fields for the […]
Good question - @s...@google.com this got me pondering, what do you have in mind for these annotations going forward? Is the goal here to be agnostic about what the annotation can possibly be? Do you imagine storing here kernel AST nodes representing a constnat? Or do you imagine something simpler like a raw string?
If you decide to not change the API, consider adding a comment to clarify what is expected to be stored here.
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Adams (Gerrit)
Feb 7, 2023, 2:07:38 PM2/7/23
to rev...@dartlang.org
Attention is currently required from: Nate Biggs, Stephen Adams.
Stephen Adams uploaded patch set #3 to this change.
[js_ast] Add annotations facility
'Annotations' allow client information to be attached to js_ast nodes.
I will be using this facility to embed resource identifers in the generated JavaScript AST to generate a map from files to the resource identifiers that they contain.
Change-Id: Id9012b303de0d2b3848a635bc34747f8c5101236
---
M pkg/js_ast/lib/src/nodes.dart
A pkg/js_ast/test/annotations_test.dart
2 files changed, 225 insertions(+), 20 deletions(-)
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Adams (Gerrit)
Feb 7, 2023, 2:25:23 PM2/7/23
to rev...@dartlang.org, Nate Biggs, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Nate Biggs, Sigmund Cherem.
3 comments:
Patchset:
PTAL
File pkg/js_ast/lib/src/nodes.dart:
Patch Set #2, Line 584:
Object
Good question - @sra@google. […]
Ideally this library is independent from dart2js. What exactly is stored as an 'annotation' on a js_ast node is up to the client.
dart2js will store some objects here that it knows how to serialize.
Specifically, there will be an object or objects with the data describing the resource identifiers embedded in the generated JavaScript.Extended the API comments.
Patch Set #2, Line 633:
_replacementAnnotationnit: This should be `_appendAnnotation` rather than replace.
Done
To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
CBuild (Gerrit)
Feb 7, 2023, 3:09:37 PM2/7/23
to Stephen Adams, rev...@dartlang.org, Nate Biggs, Sigmund Cherem, Commit Queue
Attention is currently required from: Nate Biggs, Sigmund Cherem.
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/e2acd93595a6f99c3d6b486a99612e2755807671
Nate Biggs (Gerrit)
Feb 7, 2023, 4:23:40 PM2/7/23
to Stephen Adams, rev...@dartlang.org, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Sigmund Cherem, Stephen Adams.
Patch set 4:Code-Review +1
Stephen Adams (Gerrit)
Feb 7, 2023, 4:33:59 PM2/7/23
to rev...@dartlang.org, Nate Biggs, Sigmund Cherem, CBuild, Commit Queue
Attention is currently required from: Sigmund Cherem, Stephen Adams.
Patch set 4:Commit-Queue +2
Commit Queue (Gerrit)
Feb 7, 2023, 4:34:22 PM2/7/23
to Stephen Adams, rev...@dartlang.org, Nate Biggs, Sigmund Cherem, CBuild
Commit Queue submitted this change.
Approvals:
Nate Biggs: Looks good to me, approved
Stephen Adams: Commit
[js_ast] Add annotations facility
'Annotations' allow client information to be attached to js_ast nodes.
I will be using this facility to embed resource identifers in the generated JavaScript AST to generate a map from files to the resource identifiers that they contain.
Change-Id: Id9012b303de0d2b3848a635bc34747f8c5101236
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281320
Reviewed-by: Nate Biggs <nate...@google.com>
Commit-Queue: Stephen Adams <s...@google.com>
---
M pkg/js_ast/lib/src/nodes.dart
A pkg/js_ast/test/annotations_test.dart
2 files changed, 233 insertions(+), 20 deletions(-)
diff --git a/pkg/js_ast/lib/src/nodes.dart b/pkg/js_ast/lib/src/nodes.dart
index 264dfb8..b55eb71 100644
--- a/pkg/js_ast/lib/src/nodes.dart
+++ b/pkg/js_ast/lib/src/nodes.dart
@@ -561,10 +561,33 @@
}
abstract class Node {
- JavaScriptNodeSourceInformation? get sourceInformation => _sourceInformation;
-
+ /// Field for storing source information and other annotations.
+ ///
+ /// Source information is common but not universal, so missing source
+ /// information is represented by `null`. Annotations are uncommon. As a
+ /// space-saving measure we pack both kinds of information into one field by
+ /// storing the user's JavaScriptNodeSourceInformation object directly in the
+ /// field if there are no annotations. If there are annotations, the field is
+ /// 'expanded' by having it reference an internal
+ /// [_SourceInformationAndAnnotations] object which has two fields.
JavaScriptNodeSourceInformation? _sourceInformation;
+ /// Returns the source information associated with this node.
+ JavaScriptNodeSourceInformation? get sourceInformation {
+ final source = _sourceInformation;
+ return source is _SourceInformationAndAnnotations
+ ? source._sourceInformation
+ : source;
+ }
+
+ /// Returns a list of annotations attached to this node. See [withAnnotation].
+ List<Object> get annotations {
+ final source = _sourceInformation;
+ return source is _SourceInformationAndAnnotations
+ ? source._annotations
+ : const [];
+ }
+
T accept<T>(NodeVisitor<T> visitor);
void visitChildren<T>(NodeVisitor<T> visitor);
@@ -573,22 +596,49 @@+ /// Returns a node equivalent to [this] but with an additional annotation.
+ ///
+ /// Annotations are data attached to a Node. What exactly is stored as an
+ /// annotation is determined by the user of the js_ast library. The
+ /// annotations do not affect how the AST prints, but can be inspected either
+ /// while walking the AST, either directly in a visitor or indirectly, e.g. by
+ /// the enter/exit hooks in the printer.
+ Node withAnnotation(Object newAnnotation) {
+ return _clone().._sourceInformation = _appendedAnnotation(newAnnotation);
+ }
+
+ _SourceInformationAndAnnotations _appendedAnnotation(Object newAnnotation) {
+ return _SourceInformationAndAnnotations(
+ sourceInformation, List.unmodifiable([...annotations, newAnnotation]));
}
bool get isCommaOperator => false;
@@ -612,6 +662,14 @@
}
}
+class _SourceInformationAndAnnotations
+ implements JavaScriptNodeSourceInformation {
+ final JavaScriptNodeSourceInformation? _sourceInformation;
+ final List<Object> _annotations;
+ _SourceInformationAndAnnotations(this._sourceInformation, this._annotations)
+ : assert(_sourceInformation is! _SourceInformationAndAnnotations);
+}
+
class Program extends Node {
final List<Statement> body;
Program(this.body);
@@ -649,9 +707,11 @@
// Override for refined return type.
@override
Statement withSourceInformation(
- JavaScriptNodeSourceInformation? sourceInformation) {
- if (sourceInformation == _sourceInformation) return this;
- return _clone().._sourceInformation = sourceInformation;
+ JavaScriptNodeSourceInformation? newSourceInformation) {
+ if (!_shouldReplaceSourceInformation(newSourceInformation)) return this;
+ return _clone()
+ .._sourceInformation =
+ _replacementSourceInformation(newSourceInformation);
}
@override
@@ -1312,9 +1372,11 @@To view, visit change 281320. To unsubscribe, or for help writing mail filters, visit settings.
CBuild (Gerrit)
Feb 7, 2023, 5:16:37 PM2/7/23
to Commit Queue, Stephen Adams, rev...@dartlang.org, Nate Biggs, Sigmund Cherem
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/aec79c2b3c1603420365803606141502f6f1cac7
Reply all
Reply to author
Forward
0 new messages