Enhance DirectiveListener to collect import/exports with combinators.

pkg/front_end/lib/src/fasta/source/directive_listener.dart

Index: pkg/front_end/lib/src/fasta/source/directive_listener.dart
diff --git a/pkg/front_end/lib/src/fasta/source/directive_listener.dart b/pkg/front_end/lib/src/fasta/source/directive_listener.dart
index 2adf931162d80de7e7e4b86f3420fb8a2e382937..ce4709a5a2390c8173dc0a1450757fe4ac08c9a1 100644
--- a/pkg/front_end/lib/src/fasta/source/directive_listener.dart
+++ b/pkg/front_end/lib/src/fasta/source/directive_listener.dart
@@ -7,11 +7,13 @@
 library front_end.src.fasta.source.directive_listener;
 
 import '../fasta_codes.dart' show FastaMessage, codeExpectedBlockToSkip;
+import '../parser/identifier_context.dart';
 import '../parser/listener.dart';
 import '../quote.dart';
 import '../scanner/token.dart';
+import 'stack_listener.dart';
 
-/// Listener that records the URIs from imports, exports, and part directives.
+/// Listener that records imports, exports, and part directives.
 ///
 /// This is normally used in combination with the `TopLevelParser`, which skips
 /// over the body of declarations like classes and function that are irrelevant
@@ -19,60 +21,102 @@ import '../scanner/token.dart';
 /// any top-level declaration, but we recommend to continue parsing the entire
 /// file in order to gracefully handle input errors.
 class DirectiveListener extends Listener {
-  /// Collects URIs that occur on any import directive.
-  final Set<String> imports = new Set<String>();
+  final Stack _stack = new Stack();
 
-  /// Collects URIs that occur on any export directive.
-  final Set<String> exports = new Set<String>();
+  /// Export directives with URIs and combinators.
+  final List<ImportDirective> imports = <ImportDirective>[];
+
+  /// Export directives with URIs and combinators.
+  final List<ExportDirective> exports = <ExportDirective>[];
 
   /// Collects URIs that occur on any part directive.
   final Set<String> parts = new Set<String>();
 
-  DirectiveListener();
+  bool _inDirective = false;
 
-  /// Set when entering the context of a directive, null when the parser is not
-  /// looking at a directive.
-  Set<String> _current = null;
+  DirectiveListener();
 
-  bool get _inDirective => _current != null;
+  @override
+  beginExport(_) {
+    _inDirective = true;
+  }
 
   @override
   beginImport(_) {
-    _current = imports;
+    _inDirective = true;
   }
 
   @override
-  beginExport(_) {
-    _current = exports;
+  void beginLiteralString(Token token) {
+    if (_inDirective) {
+      _push(unescapeString(token.lexeme));
+    }
   }
 
   @override
   beginPart(_) {
-    _current = parts;
+    _inDirective = true;
+  }
+
+  @override
+  void endCombinators(int count) {
+    List<String> names = _popList(count);

[Siggi Cherem (dart-lang), 2017/05/15 22:18:53]

I'm not opposed to keeping it as it is, but given that this DirectiveListener is
so specialized for a small piece of the language, I'd be open to do this without
the stack and simply by having an extra field or two to help us with the
parsing. For example, in the same way you have `_inDirective`, we could add a
`Set<String> _combinatorNames` and `List<NamespaceCombinator> _combinators` that
gets filled as you parse them.

- they all get cleared on every new import/export
- handleIdentifier will append to _combinatorNames
- endShow/endHide consume the _combinatorNames and add an object to _combinators
- endCombinators does almost nothing, or changes the state somehow (e.g.
_inCombinators = false)
- endImport/endExport simply use _combinators.

WDYT?

[scheglov, 2017/05/16 00:14:24]

Thank you, good idea.
I will send a new CL with these changes.

+    _push(names);
   }
 
   @override
   endExport(export, semicolon) {
-    _current = null;
+    List<NamespaceCombinator> combinators = _pop();
+    String uri = _pop();
+    exports.add(new ExportDirective(uri, combinators));
+    _inDirective = false;
+  }
+
+  @override
+  void endHide(Token hideKeyword) {
+    List<String> names = _pop();
+    _push(new NamespaceCombinator.hide(names));
+  }
+
+  @override
+  void endIdentifierList(int count) {
+    if (_inDirective) {
+      _push(_popList(count) ?? <String>[]);
+    }
   }
 
   @override
   endImport(import, deferred, asKeyword, semicolon) {
-    _current = null;
+    List<NamespaceCombinator> combinators = _pop();
+    String uri = _pop();
+    imports.add(new ImportDirective(uri, combinators));
+    _inDirective = false;
   }
 
   @override
   endPart(part, semicolon) {
-    _current = null;
+    String uri = _pop();
+    parts.add(uri);
+    _inDirective = false;
   }
 
   @override
-  void beginLiteralString(Token token) {
-    if (_inDirective) {
-      _current.add(unescapeString(token.lexeme));
+  void endShow(Token showKeyword) {
+    List<String> names = _pop();
+    _push(new NamespaceCombinator.show(names));
+  }
+
+  @override
+  void handleIdentifier(Token token, IdentifierContext context) {
+    if (_inDirective && context == IdentifierContext.combinator) {
+      _push(token.lexeme);
     }
   }
 
+  /// Defines how native clauses are handled. By default, they are not handled
+  /// and an error is thrown;
+  Token handleNativeClause(Token token) => null;
+
   @override
   Token handleUnrecoverableError(Token token, FastaMessage message) {
     if (message.code == codeExpectedBlockToSkip) {
@@ -82,7 +126,43 @@ class DirectiveListener extends Listener {
     return super.handleUnrecoverableError(token, message);
   }
 
-  /// Defines how native clauses are handled. By default, they are not handled
-  /// and an error is thrown;
-  Token handleNativeClause(Token token) => null;
+  T _pop<T>() {
+    var value = _stack.pop() as T;
+    return value;
+  }
+
+  List<T> _popList<T>(int n) {
+    return _stack.popList(n);
+  }
+
+  void _push<T>(T value) {
+    _stack.push(value);
+  }
+}
+
+class ExportDirective {
+  final String uri;
+  final List<NamespaceCombinator> combinators;

[Siggi Cherem (dart-lang), 2017/05/15 22:18:53]

Just double checking - I was wondering whether we need a list here - I can't
recall if the spec allows more than one combinator, but I'm guessing we might
want it regardless in the case of error-recovery. Correct?

[scheglov, 2017/05/16 00:14:24]

The specification does not limit the number of combinators.
importSpecification:
    import uri (as identifier)? combinator* ‘;’

That said, I have never seen more than one outside of tests.

+
+  ExportDirective(this.uri, this.combinators);
+}
+
+class ImportDirective {

[Siggi Cherem (dart-lang), 2017/05/15 22:18:53]

consider using a single Directive class for both import/export. Other than
keeping them separate as you do on the lists above, there seems no need to have
different types for their internal representation.

[scheglov, 2017/05/16 00:14:24]

Done.

+  final String uri;
+  final List<NamespaceCombinator> combinators;
+
+  ImportDirective(this.uri, this.combinators);
+}
+
+class NamespaceCombinator {
+  final bool isShow;
+  final Set<String> names;
+
+  NamespaceCombinator.hide(List<String> names)
+      : isShow = false,
+        names = names.toSet();
+
+  NamespaceCombinator.show(List<String> names)
+      : isShow = true,
+        names = names.toSet();
 }