fix #30138, reland with windows fix

pkg/dev_compiler/lib/src/compiler/source_map_printer.dart

Index: pkg/dev_compiler/lib/src/compiler/source_map_printer.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/source_map_printer.dart b/pkg/dev_compiler/lib/src/compiler/source_map_printer.dart
index e164f011e07d6eda46afccbe1c4f6ed71c2af3d1..f90c63a18ad1421815cc8d4a32931ddd95a6169f 100644
--- a/pkg/dev_compiler/lib/src/compiler/source_map_printer.dart
+++ b/pkg/dev_compiler/lib/src/compiler/source_map_printer.dart
@@ -3,7 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:analyzer/dart/ast/ast.dart';
-import 'package:analyzer/dart/ast/standard_resolution_map.dart';
+import 'package:analyzer/src/generated/source.dart' show LineInfo;
 import 'package:source_maps/source_maps.dart' hide Printer;
 import 'package:source_span/source_span.dart' show SourceLocation;
 
@@ -19,12 +19,9 @@ class SourceMapPrintingContext extends JS.SimpleJavaScriptPrintingContext {
   /// The source_maps builder we write JavaScript code to.
   final sourceMap = new SourceMapBuilder();
 
-  /// The cache of URIs for paths.
-  final _sourceUrlCache = <String, Object>{};
-
-  CompilationUnit unit;
-  String sourcePath;
-  AstNode _currentTopLevelDeclaration;
+  Uri _sourceUri;
+  LineInfo _lineInfo;
+  AstNode _topLevelNode;
 
   @override
   void emit(String code) {
@@ -46,44 +43,52 @@ class SourceMapPrintingContext extends JS.SimpleJavaScriptPrintingContext {
   void enterNode(JS.Node jsNode) {
     AstNode node = jsNode.sourceInformation;
     if (node == null || node.offset == -1 || node.isSynthetic) return;
-    if (unit == null) {
+    if (_topLevelNode == null) {
       // This is a top-level declaration.  Note: consecutive top-level
       // declarations may come from different compilation units due to
       // parts.
-      _currentTopLevelDeclaration = node;
-      unit = node.getAncestor((n) => n is CompilationUnit);
-      var source = resolutionMap.elementDeclaredByCompilationUnit(unit).source;
-      // Use the uri for dart: uris instead of the path of the source file
-      // on disk as that results in much cleaner stack traces.
-      // Example:
-      // source.uri = dart:core/object.dart
-      // source.fullName = gen/patched_sdk/lib/core/object.dart
-      sourcePath =
-          source.isInSystemLibrary ? source.uri.toString() : source.fullName;
+      var unit =
+          node.getAncestor((n) => n is CompilationUnit) as CompilationUnit;
+      // This happens for synthetic nodes created by AstFactory.
+      // We don't need to mark positions for them because we'll have a position
+      // for the next thing down.
+      if (unit == null) return;
+
+      _topLevelNode = node;
+      var source = unit.element.source;
+      _lineInfo = unit.lineInfo;
+      // TODO(jmesserly): this needs serious cleanup.
+      // There does appear to be something strange going on with Analyzer
+      // Source URIs if we try and use them directly on Windows.
+      // See also compiler.dart placeSourceMap, which could use cleanup too.
+      var sourcePath = source.fullName;
+      _sourceUri = source.isInSystemLibrary
+          ? source.uri // this gives cleaner stack traces for SDK libs
+          : (sourcePath.startsWith('package:')
+              ? Uri.parse(sourcePath)
+              // TODO(jmesserly): shouldn't this be path.toUri?
+              : new Uri.file(sourcePath));

[Jennifer Messerly, 2017/07/14 23:41:45]

I have no idea why this line here works in the old code, especially on Windows
where `new Uri.file` won't work without `windows: true` on windows...  it's
possible that "fullName" is already normalized in some way, although I've read a
ton of code in Analyzer and I'm not seeing how they would differ. In any case,
this should replicate the original logic, so it's got a decent chance of
working.

[Bob Nystrom, 2017/07/17 18:20:17]

If you omit "windows:", it defaults to true if the current OS is windows, I
think.

[Jennifer Messerly, 2017/07/17 19:29:45]

ah, thanks Bob, that makes more sense! Could I change it to `path.toUri`? That
feels cleaner to me.

FWIW, my current theory is that `source.fullName` gives an absolute path, vs
`source.uri` giving a relative path (in some cases). The relative path URI then
caused problems in DDC's `placeSourceMap` code.

I want to untangle this DDC code at some point; it has the look of a bug that
was fixed by adding little workarounds in different places, perhaps because the
root cause wasn't found. I don't think DDC has clear boundaries/normalization
between:

* paths on disk
* Dart URI space (file/relative path URIs, plus the made up "dart" and "package"
schemes)
* JS module URI space (browser URIs or node.js file/relative path URIs)
* Source map URI space (browser URIs, but includes the fake "dart:", perhaps the
DDC extension makes this work somehow?)

Anyways ... one of these days I'll need to get a Windows machine to dig into
this further. For now I'm hoping to preserve the original logic (with
refactoring to simplify it).

     }
-    // Skip MethodDeclarations - in the case of a one line function it finds the
-    // declaration rather than the body and confuses devtools.
-    if (node is MethodDeclaration) return;
+
     _mark(node.offset, _getIdentifier(node));
   }
 
   void exitNode(JS.Node jsNode) {
     AstNode node = jsNode.sourceInformation;
-    if (unit == null || node == null || node.offset == -1 || node.isSynthetic) {
+    if (_topLevelNode == null ||
+        node == null ||
+        node.offset == -1 ||
+        node.isSynthetic) {
       return;
     }
 
-    // TODO(jmesserly): in many cases marking the end will be unnecessary.
-    // Skip MethodDeclarations - in the case of a one line function it finds the
-    // declaration rather than the body and confuses devtools.
-    if (node is! MethodDeclaration) {
-      _mark(node.end);
-    }
+    // TODO(jmesserly): in most cases marking the end will be unnecessary.
+    _mark(node.end);
 
-    if (identical(node, _currentTopLevelDeclaration)) {
-      unit = null;
-      sourcePath = null;
-      _currentTopLevelDeclaration == null;
+    if (identical(node, _topLevelNode)) {
+      _sourceUri = null;
+      _lineInfo = null;
+      _topLevelNode = null;
     }
   }
 
@@ -92,23 +97,17 @@ class SourceMapPrintingContext extends JS.SimpleJavaScriptPrintingContext {
       node is SimpleIdentifier ? node.name : null;
 
   void _mark(int offset, [String identifier]) {
-    var loc = unit.lineInfo.getLocation(offset);
+    var loc = _lineInfo.getLocation(offset);
     // Chrome Devtools wants a mapping for the beginning of
     // a line, so bump locations at the end of a line to the beginning of
     // the next line.
-    var next = unit.lineInfo.getLocation(offset + 1);
+    var next = _lineInfo.getLocation(offset + 1);
     if (next.lineNumber == loc.lineNumber + 1) {
       loc = next;
     }
-    var sourceUrl = _sourceUrlCache.putIfAbsent(
-        sourcePath,
-        () =>
-            sourcePath.startsWith('dart:') || sourcePath.startsWith('package:')
-                ? sourcePath
-                : new Uri.file(sourcePath));
     sourceMap.addLocation(
         new SourceLocation(offset,
-            sourceUrl: sourceUrl,
+            sourceUrl: _sourceUri,
             line: loc.lineNumber - 1,
             column: loc.columnNumber - 1),
         new SourceLocation(buffer.length, line: _line, column: _column),