Verify resynthesized elements by comparing text presentations.

pkg/analyzer/test/src/summary/element_text.dart

+import 'dart:io';

[Brian Wilkerson, 2017/03/03 16:09:33]

Missing copyright notice.

[scheglov, 2017/03/03 16:24:29]

Done.

+
+import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/dart/element/type.dart';
+import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/dart/element/type.dart';
+import 'package:analyzer/src/generated/source.dart';
+import 'package:analyzer/src/generated/utilities_dart.dart';
+import 'package:test/test.dart';
+
+const String _testPath =
+    '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart';

[Brian Wilkerson, 2017/03/03 16:09:33]

That won't work on other machines. :-)

[scheglov, 2017/03/03 16:24:29]

No, it won't.
I will null it before commit.
Then the person who wants to perform mass-(re)generation will need to set the
correct value.

[Paul Berry, 2017/03/03 17:22:52]

You should be able to dynamically figure out the correct path using
Platform.script as a starting point.  (Some heuristics may be required since
there are multiple entry points that might eventually lead to this code).

+
+final List<_Replacement> _replacements = [];
+
+String _testCode;
+LineInfo _testCodeLines;
+void applyReplacements() {
+  if (_testPath != null && _replacements.isNotEmpty) {
+    _replacements.sort((a, b) => b.offset - a.offset);
+    String newCode = _testCode;
+    _replacements.forEach((r) {
+      newCode =
+          newCode.substring(0, r.offset) + r.text + newCode.substring(r.end);
+    });
+    new File(_testPath).writeAsStringSync(newCode);
+  }
+}
+
+void checkElementText(LibraryElement library, String expected,
+    {bool updateExpectations: false, bool withOffsets: false}) {
+  var writer = new _ElementWriter(withOffsets: withOffsets);
+  writer.writeLibraryElement(library);
+  String actualText = writer.buffer.toString();
+  if (/*updateExpectations &&*/ actualText != expected) {

[Brian Wilkerson, 2017/03/03 16:09:33]

I assume you'll remove the comments before committing.

[scheglov, 2017/03/03 16:24:29]

Acknowledged.

+    if (_testCode == null) {
+      _testCode = new File(_testPath).readAsStringSync();
+      _testCodeLines = new LineInfo.fromContent(_testCode);
+    }
+
+    try {
+      throw 42;
+    } catch (e, trace) {
+      String traceString = trace.toString();
+
+      int testFilePathOffset = traceString.indexOf(_testPath);

[Paul Berry, 2017/03/03 17:22:51]

Reading this code, it's really non-obvious what the string you are matching
against is expected to look like.  Consider replacing with a regular expression
that does the same job.  Or add a comment, e.g.:

// Assuming traceString contains "$_testPath:$ignoredText:$invocationLine",
figure out the value of invocationLine.

[scheglov, 2017/03/03 18:46:31]

Done.

+      expect(testFilePathOffset, isNonNegative);
+
+      int lineOffset = testFilePathOffset + _testPath.length + ':'.length;

[Paul Berry, 2017/03/03 17:22:52]

Nit: for sanity, verify that traceString[testFilePathOffset + _testPath.length]
== ':'

[scheglov, 2017/03/03 18:46:31]

Done.

+      int invocationLine = int.parse(traceString.substring(
+          lineOffset, traceString.indexOf(':', lineOffset)));
+      int invocationOffset = _testCodeLines.getOffsetOfLine(invocationLine - 1);
+
+      int expectationOffset = _testCode.indexOf("r'''", invocationOffset);

[Paul Berry, 2017/03/03 17:22:51]

It would be nice to have a sanity check here to make sure that we didn't
accidentally wind up matching a string later in the file.  For example, maybe
verify that _testCode.substring(invocationOffset, expectationOffset) doesn't
contain any of the characters `'`, `"`, or `}`.

[scheglov, 2017/03/03 18:46:30]

Done.

+      expectationOffset += 5;

[Paul Berry, 2017/03/03 17:22:51]

Why 5?  I would have expected `"r'''".length`, which is 4.

Are you assuming that "r''''" is followed by a newline, and trying to skip the
newline?

If so, I think it would be safer to change this to `expectationOffset +=
"r'''".length;`, and on line 59 below, replace `actualText` with
`'\n$actualText'`.

[scheglov, 2017/03/03 18:46:31]

Done.

+      int expectationEnd = _testCode.indexOf("'''", expectationOffset);
+
+      _replacements
+          .add(new _Replacement(expectationOffset, expectationEnd, actualText));
+    }
+  }
+
+  expect(actualText, expected);
+}
+
+class _ElementWriter {
+  final bool withOffsets;
+  final StringBuffer buffer = new StringBuffer();
+
+  _ElementWriter({this.withOffsets: false});
+
+  bool isDynamicType(DartType type) => type is DynamicTypeImpl;
+
+  void newLineIfNotEmpty() {
+    if (buffer.isNotEmpty) {
+      buffer.writeln('');

[Brian Wilkerson, 2017/03/03 16:09:33]

You don't need an argument to writeln.

[scheglov, 2017/03/03 16:24:29]

Done.

+    }
+  }
+
+  void writeClassElement(ClassElement e) {
+    if (e.isAbstract) {
+      buffer.write('abstract ');
+    }
+    buffer.write('class ');
+
+    writeName(e);
+    writeTypeParameterElements(e.typeParameters);
+
+    if (e.supertype != null && e.supertype.displayName != 'Object' ||
+        e.mixins.isNotEmpty) {
+      buffer.write(' extends ');
+      writeType(e.supertype);
+    }
+
+    writeList(' with ', '', e.mixins, ', ', writeType);
+    writeList(' implements ', '', e.interfaces, ', ', writeType);
+
+    buffer.writeln(' {');
+
+    e.fields.forEach(writeFieldElement);
+    e.accessors.forEach(writePropertyAccessorElement);
+
+    expect(e.constructors, isNotEmpty);
+    if (e.constructors.length == 1 &&
+        e.constructors[0].isSynthetic &&
+        e.mixins.isEmpty) {
+      expect(e.constructors[0].parameters, isEmpty);
+    } else {
+      e.constructors.forEach(writeConstructorElement);
+    }
+
+    e.methods.forEach(writeMethodElement);
+    buffer.writeln('}');
+  }
+
+  void writeConstructorElement(ConstructorElement e) {
+    buffer.write('  ');
+
+    if (e.isSynthetic) {
+      buffer.write('synthetic ');
+    }
+
+    buffer.write(e.enclosingElement.name);
+    if (e.name != '') {

[Brian Wilkerson, 2017/03/03 16:09:33]

isNotEmpty?

[scheglov, 2017/03/03 16:24:29]

Done.

+      buffer.write('.');
+      writeName(e);
+    }
+
+    writeParameterElements(e.parameters);
+    buffer.writeln(';');
+  }
+
+  void writeEnumElement(ClassElement e) {

[Paul Berry, 2017/03/03 17:22:52]

I'm concerned that this method makes a lot of assumptions about the shape of the
element model (e.g. it doesn't check that "index" or"values" have the propert
types).  Since the purpose of these tests is to make sure the element model has
been rebuilt correctly, that seems like it weakens the power of the test.  What
if instead we unified this logic with writeClassElement, and so for `enum E {
foo, bar}`,  the text that was output would look comething like this:

enum E {
  int index;
  static List<E> values;
  static E foo;
  static E bar;
}

[scheglov, 2017/03/03 18:46:31]

Done.

Alternatively we could add more checks for these fields, but still don't include
them into text.
It's not like these fields might be different for different enums.

+//    var e = _e as EnumElementImpl;

[Paul Berry, 2017/03/03 17:22:52]

Delete this line.

+    expect(e.isEnum, isTrue);

[Brian Wilkerson, 2017/03/03 16:09:33]

Should we expect the opposite is writeClassElement?

[scheglov, 2017/03/03 16:24:29]

Done.

+
+    buffer.write('enum ');
+    writeName(e);
+    buffer.write(' { ');
+
+    bool firstEnumValue = true;
+    for (FieldElement field in e.fields) {
+      if (field.name == 'index') {
+        expect(field.isSynthetic, isTrue);
+        expect(field.isStatic, isFalse);
+        continue;
+      }
+      if (field.name == 'values') {
+        expect(field.isSynthetic, isTrue);
+        expect(field.isStatic, isTrue);
+        continue;
+      }
+      if (firstEnumValue) {
+        firstEnumValue = false;
+      } else {
+        buffer.write(', ');
+      }
+      expect(field.isStatic, isTrue);
+      writeName(field);
+    }
+
+    buffer.writeln(' }');
+  }
+
+  void writeExportElement(ExportElement e) {
+    buffer.write('export ');
+    writeUri(e, e.exportedLibrary.source);
+
+    e.combinators.forEach(writeNamespaceCombinator);
+

[Paul Berry, 2017/03/03 17:22:52]

Also write out conditional URIs.

[scheglov, 2017/03/03 18:46:30]

I'm adding TODO for now.

+    buffer.writeln(';');
+  }
+
+  void writeFieldElement(FieldElement e) {
+    if (!e.isSynthetic) {
+      buffer.write('  ');
+
+      if (e.isStatic) {
+        buffer.write('static ');
+      }
+
+      if (e is FieldElementImpl && e.isCovariant) {
+        buffer.write('convariant ');

[Paul Berry, 2017/03/03 17:22:52]

s/convariant/covariant/

[scheglov, 2017/03/03 18:46:30]

Done.

+      }
+
+      writePropertyInducingElement(e);
+    }
+  }
+
+  void writeFunctionElement(FunctionElement e) {
+    if (e.isExternal) {
+      buffer.write('external ');
+    }
+
+    writeType2(e.returnType);
+
+    writeName(e);
+
+    writeTypeParameterElements(e.typeParameters);
+    writeParameterElements(e.parameters);
+
+    // TODO(scheglov) async*
+
+    buffer.writeln(' {}');
+  }
+
+  void writeFunctionTypeAliasElement(FunctionTypeAliasElement e) {
+    buffer.write('typedef ');
+    writeType2(e.returnType);
+
+    writeName(e);
+
+    writeTypeParameterElements(e.typeParameters);
+    writeParameterElements(e.parameters);
+
+    buffer.writeln(';');
+  }
+
+  void writeImportElement(ImportElement e) {
+    if (!e.isSynthetic) {
+      buffer.write('import ');
+      writeUri(e, e.importedLibrary.source);
+
+      if (e.isDeferred) {
+        buffer.write(' deferred');
+      }
+
+      if (e.prefix != null) {
+        buffer.write(' as ');
+        writeName(e.prefix);
+        if (withOffsets) {
+          buffer.write('(${e.prefixOffset})');
+        }
+      }
+
+      e.combinators.forEach(writeNamespaceCombinator);
+

[Paul Berry, 2017/03/03 17:22:52]

Also write out conditional URIs.

[scheglov, 2017/03/03 18:46:31]

I'm adding TODO for now.

[scheglov, 2017/03/03 19:03:06]

Actually, ImportElement does not expose all conditional variants, you can see
only the selected one, and we already print it.

+      buffer.writeln(';');
+    }
+  }
+
+  void writeLibraryElement(LibraryElement e) {
+    if (e.displayName != '') {
+      buffer.write('library ');
+      writeName(e);
+      buffer.writeln(';');
+    }
+
+    e.imports.forEach(writeImportElement);
+    e.exports.forEach(writeExportElement);
+    e.parts.forEach(writePartElement);
+
+    e.units.forEach(writeUnitElement);
+  }
+
+  void writeList<T>(String open, String close, List<T> items, String separator,
+      writeItem(T item),
+      {bool includeEmpty: false}) {
+    if (!includeEmpty && items.isEmpty) {
+      return;
+    }
+    buffer.write(open);
+    bool first = true;
+    for (T item in items) {
+      if (!first) {
+        buffer.write(separator);
+      }
+      writeItem(item);
+      first = false;
+    }
+    buffer.write(close);
+  }
+
+  void writeMethodElement(MethodElement e) {
+    buffer.write('  ');
+
+    if (e.isStatic) {
+      buffer.write('static ');
+    }
+
+    if (e.isExternal) {
+      buffer.write('external ');
+    }
+
+    writeType2(e.returnType);
+
+    writeName(e);
+
+    writeTypeParameterElements(e.typeParameters);
+    writeParameterElements(e.parameters);
+
+    // TODO(scheglov) async*
+
+    if (e.isAbstract) {
+      buffer.writeln(';');
+    } else {
+      buffer.writeln(' {}');
+    }
+  }
+
+  void writeName(Element e) {
+    buffer.write(e.displayName);
+    if (withOffsets) {
+      buffer.write('(');

[Paul Berry, 2017/03/03 17:22:52]

This makes a name decorated with an offset look like a function invocation (e.g.
`foo(123)`).  I think that might make things confusing.

Consider changing the format to something that looks less like valid Dart code,
e.g. `foo@123`.

[scheglov, 2017/03/03 18:46:31]

Done.

[Paul Berry, 2017/03/03 23:24:35]

It looks like you haven't done this yet.  Did you decide not to, or was this an
oversight?

Your call whether to change this or not.

[scheglov, 2017/03/04 02:21:58]

I'm sorry for the oversight.
I made the change, but the after another change decided to revert some changes
using IDEA's local history.
It accidentally reverted this change too.
Fixed.

+      buffer.write(e.nameOffset);
+      buffer.write(')');
+    }
+  }
+
+  void writeNamespaceCombinator(NamespaceCombinator e) {
+    if (e is ShowElementCombinator) {
+      buffer.write(' show ');
+      buffer.write(e.shownNames.join(', '));
+    } else if (e is HideElementCombinator) {
+      buffer.write(' hide ');
+      buffer.write(e.hiddenNames.join(', '));
+    }
+  }
+
+  void writeParameterElement(ParameterElement e) {
+    String closeString;
+    ParameterKind kind = e.parameterKind;
+    if (kind == ParameterKind.REQUIRED) {
+      closeString = '';
+    } else if (kind == ParameterKind.POSITIONAL) {
+      buffer.write('[');
+      closeString = ']';
+    } else if (kind == ParameterKind.NAMED) {
+      buffer.write('{');
+      closeString = '}';
+    } else {
+      fail('Unknown parameter kind: $kind');
+    }
+
+    if (e.isCovariant) {
+      buffer.write('convariant ');

[Paul Berry, 2017/03/03 17:22:52]

s/convariant/covariant/

[scheglov, 2017/03/03 18:46:30]

Done.

+    }
+
+    if (e.isFinal) {
+      buffer.write('final ');
+    }
+
+    writeType2(e.type);
+
+    if (e is FieldFormalParameterElement) {
+      buffer.write('this.');
+    }
+
+    writeName(e);
+
+    buffer.write(closeString);
+  }
+
+  void writeParameterElements(List<ParameterElement> elements) {
+    writeList('(', ')', elements, ', ', writeParameterElement,
+        includeEmpty: true);
+  }
+
+  void writePartElement(CompilationUnitElement e) {
+    buffer.write('part ');
+    writeUri(e, e.source);
+    buffer.writeln(';');
+  }
+
+  void writePropertyAccessorElement(PropertyAccessorElement e) {
+    if (e.isSynthetic) {
+      return;
+    }
+
+    if (e.enclosingElement is ClassElement) {
+      buffer.write('  ');
+
+      if (e.isStatic) {
+        buffer.write('static ');
+      }
+    }
+
+    if (e.isExternal) {
+      buffer.write('external ');
+    }
+
+    writeType2(e.returnType);
+
+    if (e.isGetter) {
+      buffer.write('get ');
+    } else {
+      buffer.write('set ');
+    }
+
+    writeName(e);
+
+    if (e.isSetter || e.parameters.isNotEmpty) {
+      writeParameterElements(e.parameters);
+    }
+
+    expect(e.typeParameters, isEmpty);
+
+    expect(e.isSynchronous, isTrue);
+    expect(e.isAsynchronous, isFalse);
+    expect(e.isGenerator, isFalse);
+
+    if (e.isAbstract) {
+      buffer.writeln(';');
+    } else {
+      buffer.writeln(' {}');
+    }
+  }
+
+  void writePropertyInducingElement(PropertyInducingElement e) {
+    if (!e.isSynthetic) {
+      DartType type = e.type;
+      expect(type, isNotNull);
+      bool hasDynamicType = isDynamicType(type);
+
+      if (e.isFinal) {
+        buffer.write('final ');
+      } else if (e.isConst) {
+        buffer.write('const ');
+      } else if (hasDynamicType) {
+        buffer.write('var ');
+      }
+
+      if (!hasDynamicType) {
+        writeType(type);
+        buffer.write(' ');
+      }

[Brian Wilkerson, 2017/03/03 16:09:33]

Replace with writeType2?

[scheglov, 2017/03/03 16:24:29]

Done.

+
+      writeName(e);
+

[Paul Berry, 2017/03/03 17:22:52]

One of the things that was hardest to get right when resynthesizing the element
model was the synthetic function for the initializer.  Can we write that out
(along with its return type)?

[scheglov, 2017/03/03 18:46:31]

I'm adding TODO for now.

+      buffer.writeln(';');
+    }
+  }
+
+  void writeTopLevelVariableElement(TopLevelVariableElement e) {
+    writePropertyInducingElement(e);
+  }
+
+  void writeType(DartType type) {
+    buffer.write(type.displayName);

[Paul Berry, 2017/03/03 17:22:52]

The "→" character that we use when writing out function types is hard to type in
an editor.  Since some of these expectations are going to get edited manually,
consider adding code here that translates "→" to "->".

[scheglov, 2017/03/03 18:46:31]

It looks better with this character :-(
If possible, I'd like to keep this character.
But if it is too annoying, I can definitely replace it with ->.
I expect that initial versions of expectations will be copied from failures and
then patched to the correct state.
And we can also include this character in the DartDoc of the class so that it
can be copy/pasted.

+  }
+
+  void writeType2(DartType type) {

[Paul Berry, 2017/03/03 17:22:52]

IMHO this is more sophisticated than we ought to be getting.  We should always
write out the type that's in the element model, even if it's dynamic.

[scheglov, 2017/03/03 18:46:31]

Done.

+    if (type != DynamicTypeImpl.instance) {
+      writeType(type);
+      buffer.write(' ');
+    }
+  }
+
+  void writeTypeParameterElement(TypeParameterElement e) {
+    writeName(e);
+    if (e.bound != null) {
+      buffer.write(' extends ');
+      writeType(e.bound);
+    }
+  }
+
+  void writeTypeParameterElements(List<TypeParameterElement> elements) {
+    writeList('<', '>', elements, ', ', writeTypeParameterElement);

[Brian Wilkerson, 2017/03/03 16:09:33]

Probably want to include if empty.

[scheglov, 2017/03/03 16:24:29]

I don't think so.
This would transform "class C {}" into "class C<> {}" then.

+  }
+
+  void writeUnitElement(CompilationUnitElement e) {
+    if (e.library.definingCompilationUnit != e) {
+      buffer.writeln('-' * 20);
+      buffer.writeln('unit: ${e.source.shortName}');
+      buffer.writeln('');
+    }
+    e.functionTypeAliases.forEach(writeFunctionTypeAliasElement);
+    e.enums.forEach(writeEnumElement);
+    e.types.forEach(writeClassElement);
+    e.topLevelVariables.forEach(writeTopLevelVariableElement);
+    e.accessors.forEach(writePropertyAccessorElement);
+    e.functions.forEach(writeFunctionElement);
+  }
+
+  void writeUri(UriReferencedElement e, Source source) {
+    String uri = e.uri ?? source.uri.toString();
+    buffer.write('\'$uri\'');
+    if (withOffsets) {
+      buffer.write('(');
+      buffer.write('${e.uriOffset}, ');
+      buffer.write('${e.uriEnd})');
+      buffer.write(')');
+    }
+  }
+}
+
+class _Replacement {
+  final int offset;
+  final int end;
+  final String text;
+  _Replacement(this.offset, this.end, this.text);
+}