fix #30138, reland with windows fix

fix #30138, reland with windows fix

Revert "Revert "fix #30138, synethic nodes causing crash generating source maps""

This reverts commit b7e3d9185d267ad378cac5b521397576a94aa7b7.

R=vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/34361bb02ab2e7e35b957d151f620f6c3c1ba0c1

[Jennifer Messerly, 2017-07-14 23:41:44]

jmesserly@google.com changed reviewers:
+ vsm@google.com

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

There's only a small tweak vs the original CL

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:70: : new
Uri.file(sourcePath));
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.

[vsm, 2017-07-17 15:05:22]

vsm@google.com changed reviewers:
+ rnystrom@google.com

[vsm, 2017-07-17 15:05:22]

Adding Bob - who fixed a few other win path issues recently too & might have an
idea.

Otherwise, lgtm

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

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:70: : new
Uri.file(sourcePath));
On 2017/07/14 23:41:45, Jennifer Messerly wrote:
> 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...

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

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

Thank you!

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:70: : new
Uri.file(sourcePath));
On 2017/07/17 18:20:17, Bob Nystrom wrote:
> On 2017/07/14 23:41:45, Jennifer Messerly wrote:
> > 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...
> 
> If you omit "windows:", it defaults to true if the current OS is windows, I
> think.

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).

[Bob Nystrom, 2017-07-17 20:30:12]

On 2017/07/17 19:29:45, Jennifer Messerly wrote:
> Thank you!
> 
>
https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
> File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):
> 
>
https://codereview.chromium.org/2977183002/diff/1/pkg/dev_compiler/lib/src/co...
> pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:70: : new
> Uri.file(sourcePath));
> On 2017/07/17 18:20:17, Bob Nystrom wrote:
> > On 2017/07/14 23:41:45, Jennifer Messerly wrote:
> > > 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...
> > 
> > If you omit "windows:", it defaults to true if the current OS is windows, I
> > think.
> 
> ah, thanks Bob, that makes more sense! Could I change it to `path.toUri`? That
> feels cleaner to me.

Yes, you *should* be able to, as long as you aren't inadvertently relying on
weird edge case behavior of Uri.file(). You probably aren't, but it's hard to
tell.

> 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?)

If you think that's bad, take a look at test.dart. It doesn't have access to the
path package (!).

– bob

[Jennifer Messerly, 2017-07-17 22:58:30]

Description was changed from

==========
fix #30138, reland with windows fix

Revert "Revert "fix #30138, synethic nodes causing crash generating source
maps""

This reverts commit b7e3d9185d267ad378cac5b521397576a94aa7b7.
==========

to

==========
fix #30138, reland with windows fix

Revert "Revert "fix #30138, synethic nodes causing crash generating source
maps""

This reverts commit b7e3d9185d267ad378cac5b521397576a94aa7b7.

R=vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/34361bb02ab2e7e35b957d151f620f6c3c1ba0c1
==========

[Jennifer Messerly, 2017-07-17 22:58:31]

Committed patchset #1 (id:1) manually as
34361bb02ab2e7e35b957d151f620f6c3c1ba0c1 (presubmit successful).