Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917863005:
Qualify library names in packages (Closed)
|
Created:
4 years, 7 months ago by vsm Modified:
4 years, 7 months ago CC:
dev-compiler+reviews_dartlang.org Base URL:
https://github.com/dart-lang/dev_compiler.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionQualify library names in packages
Partially addresses #504. This is WIP, but wanted your thoughts.
Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
E.g., this gets lowered:
import { src$interfaces } from "matcher"
- Should we thread package root through ModuleCompiler to let it packagify urls?
- How should we handle non-package urls? Pass in an explicit root dir
and go relative?
R=jmesserly@google.com
Committed: https://github.com/dart-lang/dev_compiler/commit/bea365789e72bcc0c9e3dd7e5f87992d6d17bb3f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use restoreAbsolute #
Total comments: 13
Patch Set 3 : Revert restoreUri changes and add build-root for non-package files #Patch Set 4 : One more revert #Patch Set 5 : Rebase #
Total comments: 7
Messages
Total messages: 27 (6 generated)
Description was changed from
==========
Qualify library names in packages
Partially addresses #504. This is WIP, but wanted your thoughts.
Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
E.g., this gets lowered:
import { src$interfaces } from "matcher"
- Should we thread package root through ModuleCompiler to let it packagify urls?
- How should we handle non-package urls? Pass in an explicit root dir
and go relative?
==========
to
==========
Qualify library names in packages
Partially addresses #504. This is WIP, but wanted your thoughts.
Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
E.g., this gets lowered:
import { src$interfaces } from "matcher"
- Should we thread package root through ModuleCompiler to let it packagify urls?
- How should we handle non-package urls? Pass in an explicit root dir
and go relative?
==========
vsm@google.com changed reviewers: + jmesserly@google.com
vsm@google.com changed reviewers: + sigmund@google.com
On 2016/04/26 20:11:52, vsm wrote: We may have multiple package roots (multi-resolver).
https://codereview.chromium.org/1917863005/diff/1/lib/src/compiler/code_gener... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:3769: var packageRelativePath = uri.pathSegments.skip(1).join('/'); Yeah in general it's not going to be unique. Also, surely we do not want $47 in our API signatures. :) I think the problem is `pathToJSIdentifier`... it's not designed to be run except on the file name. So last time we had discussed this in: https://github.com/dart-lang/dev_compiler/issues/504#issuecomment-211561183 ... we were going to have non-top-level libs to use some scheme where they're kind of hidden. Library name is used in two distinct ways: A. as a local variable in the module B. as an export/import member of the module Only B needs to worry about conflicts. So something like: // import import { unittest, _libs } from 'unittest' // note: this is a temp, conflicts if any will be automagically handled, // so we can use any friendly name here. src_matchers would work too. const matchers = _libs['src/matchers']; // if we're exporting export const _libraries = { 'src/matchers': matchers, //... } ... that would be a generally safe scheme, we only have one special name (_libraries). That seems like the route to go for a real fix. If we just want a quick hack, maybe we could use "_", which would not look too bad in the code. https://codereview.chromium.org/1917863005/diff/1/lib/src/compiler/module_bui... File lib/src/compiler/module_builder.dart (right): https://codereview.chromium.org/1917863005/diff/1/lib/src/compiler/module_bui... lib/src/compiler/module_builder.dart:153: name = path.withoutExtension(name); hmmm, this change seems bad for absolute paths, are they falling into? if they are then we need to do something else. encoding the full path "/user/whoever" is not good. Probably you could move the path handling into the other function.
BTW it would be nice to replace $47, but conceptually this LGTM, definitely the easiest way to get this going. We also chatted about stashing things in a "private libraries" object, that should be doable, in some sense is cleaner but it's a bit more generated code. @Siggi -- Vijay was thinking about implementing the reverse package:* mapping API for multi package, so you could pass in the file names via file paths and it would figure out the package: for you. That might be nice for Bazel
vsm@google.com changed reviewers: + paulberry@google.com
Using the analyzer's restoreAbsolute now. https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... File lib/src/analyzer/multi_package_resolver.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... lib/src/analyzer/multi_package_resolver.dart:46: var libIndex = segments.lastIndexOf('lib'); This is not yet tested and seems brittle... https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3768: var sourceFactory = context.sourceFactory as SourceFactoryImpl; sourceFactory has it's own restoreAbsolute that walks the resolvers in order - but it's preferring the file resolver to the package resolver.
Not sure if you saw my comments from the first round? The new code looks good though :) https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... File lib/src/analyzer/multi_package_resolver.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... lib/src/analyzer/multi_package_resolver.dart:56: /// Resolve [packagePath] by looking at each prefix in [searchPaths] and returning long line https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3768: var sourceFactory = context.sourceFactory as SourceFactoryImpl; On 2016/04/27 18:24:35, vsm wrote: > sourceFactory has it's own restoreAbsolute that walks the resolvers in order - > but it's preferring the file resolver to the package resolver. Oh crazy! Is that hard coded? You could change the order we pass the resolvers in. (I would guess we already pass in package resolver first though.... code is in lib/src/analyzer/context.dart in DDC)
Note: you might want to read my last comment first, since it might render the other comments moot. https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... File lib/src/analyzer/multi_package_resolver.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... lib/src/analyzer/multi_package_resolver.dart:35: Uri restoreAbsolute(Source source) { FYI, technically this breaks the interface contract of UriResolver.restoreAbsolute, which says "The computation should be based solely on [source.fullName]". The rationale is: some callers of restoreAbsolute start with a path in the file system and nothing else, and then they create a Source object whose .fullName points to that path and whose other properties (.uri, .etc.) are likely to be bogus. Then they pass this bogus Source object to restoreAbsolute(), get back a Uri, and then pass it to resolveAbsolute() to get a non-bogus Source object. I think this would be easy to fix; simply drop the uri.scheme checks at the top of the function, and set filePath = path.absolute(source.fullName). Having said that, restoreAbsolute() is used in such limited circumstances that if you want to chance it, you probably won't run into any trouble. https://codereview.chromium.org/1917863005/diff/20001/lib/src/analyzer/multi_... lib/src/analyzer/multi_package_resolver.dart:41: var filePath = path.absolute(uri.path); This won't work on Windows, since uri.path uses '/', and path.absolute() assumes the conventions of the host OS. My suggestion above (set filePath = path.absolute(source.fullName) would address this. https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3768: var sourceFactory = context.sourceFactory as SourceFactoryImpl; On 2016/04/27 18:24:35, vsm wrote: > sourceFactory has it's own restoreAbsolute that walks the resolvers in order - > but it's preferring the file resolver to the package resolver. Are you sure about this? Looking at the definition in package:analyzer/src/context/source.dart, it looks like it just visits the resolvers in order. https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3770: var uri = resolver.restoreAbsolute(library.source); Why not just: var uri = library.source.uri; This retrieves the URI that was used to find the package when it was first resolved, which I think gives you the information you want and saves a lot of legwork.
https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3770: var uri = resolver.restoreAbsolute(library.source); On 2016/04/27 19:19:57, Paul Berry wrote: > Why not just: > > var uri = library.source.uri; > > This retrieves the URI that was used to find the package when it was first > resolved, which I think gives you the information you want and saves a lot of > legwork. ah, thanks, I wondered that too! Figured maybe I was missing something. Yeah in my experience, library.source.uri always has the right thing. :)
thanks! still iterating but some quick replies... :-) https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3768: var sourceFactory = context.sourceFactory as SourceFactoryImpl; On 2016/04/27 19:19:57, Paul Berry wrote: > On 2016/04/27 18:24:35, vsm wrote: > > sourceFactory has it's own restoreAbsolute that walks the resolvers in order - > > but it's preferring the file resolver to the package resolver. > > Are you sure about this? Looking at the definition in > package:analyzer/src/context/source.dart, it looks like it just visits the > resolvers in order. yep, just need to set in the right order! will fix and get rid of the unneeded import. https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3770: var uri = resolver.restoreAbsolute(library.source); On 2016/04/27 19:24:21, John Messerly wrote: > On 2016/04/27 19:19:57, Paul Berry wrote: > > Why not just: > > > > var uri = library.source.uri; > > > > This retrieves the URI that was used to find the package when it was first > > resolved, which I think gives you the information you want and saves a lot of > > legwork. > > ah, thanks, I wondered that too! Figured maybe I was missing something. > > Yeah in my experience, library.source.uri always has the right thing. :) that should actually work for the bazel use case. according to siggi, we'll always be given a package:uri for anything that could be a package uri. it is handy though for compiling from the command line - i can glob in a list of files under packages/collection rather than listing each explicitly. i can remove from this cl though if y'all would prefer.
On 2016/04/27 20:56:09, vsm wrote: > thanks! still iterating but some quick replies... :-) > > https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... > File lib/src/compiler/code_generator.dart (right): > > https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... > lib/src/compiler/code_generator.dart:3768: var sourceFactory = > context.sourceFactory as SourceFactoryImpl; > On 2016/04/27 19:19:57, Paul Berry wrote: > > On 2016/04/27 18:24:35, vsm wrote: > > > sourceFactory has it's own restoreAbsolute that walks the resolvers in order > - > > > but it's preferring the file resolver to the package resolver. > > > > Are you sure about this? Looking at the definition in > > package:analyzer/src/context/source.dart, it looks like it just visits the > > resolvers in order. > > yep, just need to set in the right order! will fix and get rid of the unneeded > import. > > https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... > lib/src/compiler/code_generator.dart:3770: var uri = > resolver.restoreAbsolute(library.source); > On 2016/04/27 19:24:21, John Messerly wrote: > > On 2016/04/27 19:19:57, Paul Berry wrote: > > > Why not just: > > > > > > var uri = library.source.uri; > > > > > > This retrieves the URI that was used to find the package when it was first > > > resolved, which I think gives you the information you want and saves a lot > of > > > legwork. > > > > ah, thanks, I wondered that too! Figured maybe I was missing something. > > > > Yeah in my experience, library.source.uri always has the right thing. :) > > that should actually work for the bazel use case. according to siggi, we'll > always be given a package:uri for anything that could be a package uri. > > it is handy though for compiling from the command line - i can glob in a list of > files under packages/collection rather than listing each explicitly. > > i can remove from this cl though if y'all would prefer. Actually, I will rip out the restoreUri / glob code from this cl. Trivial to right a helper script for the command line case for now.
https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3770: var uri = resolver.restoreAbsolute(library.source); On 2016/04/27 20:56:08, vsm wrote: > On 2016/04/27 19:24:21, John Messerly wrote: > > On 2016/04/27 19:19:57, Paul Berry wrote: > > > Why not just: > > > > > > var uri = library.source.uri; > > > > > > This retrieves the URI that was used to find the package when it was first > > > resolved, which I think gives you the information you want and saves a lot > of > > > legwork. > > > > ah, thanks, I wondered that too! Figured maybe I was missing something. > > > > Yeah in my experience, library.source.uri always has the right thing. :) > > that should actually work for the bazel use case. according to siggi, we'll > always be given a package:uri for anything that could be a package uri. > > it is handy though for compiling from the command line - i can glob in a list of > files under packages/collection rather than listing each explicitly. > > i can remove from this cl though if y'all would prefer. Ah, fair point. Ok, I see why this is a use case that is worth supporting. And I see why in order to support it we need to implement the multi-package resolver's restoreAbsolute() method. However, I still believe we shouldn't be calling that method from here (nor should we be calling sourceFactory.restoreUri() from here). Instead, we need to ensure that library.source.uri is correct before passing the sources into the analyzer in the first place (otherwise things will go wrong elsewhere in the analyzer). For an example of how we do this in analyzer_cli, see https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_cli/lib/src/driver..... Note that we create the Source object, then use SourceFactory.restoreUri() to see if a better URI can be found for it, and if so, we recreate the source based on the better URI. This is all done prior to feeding the sources into the context, so the analyzer always sees correct URIs. It's particularly important that we get this right for DDC, because DDC outputs a summary, and the URIs contained in the summary are the URIs from library.source.uri. So if those URIs aren't correct, we'll run into trouble when someone tries to use the summary as a dependency of another package.
https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/20001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3770: var uri = resolver.restoreAbsolute(library.source); On 2016/04/27 21:13:49, Paul Berry wrote: > On 2016/04/27 20:56:08, vsm wrote: > > On 2016/04/27 19:24:21, John Messerly wrote: > > > On 2016/04/27 19:19:57, Paul Berry wrote: > > > > Why not just: > > > > > > > > var uri = library.source.uri; > > > > > > > > This retrieves the URI that was used to find the package when it was first > > > > resolved, which I think gives you the information you want and saves a lot > > of > > > > legwork. > > > > > > ah, thanks, I wondered that too! Figured maybe I was missing something. > > > > > > Yeah in my experience, library.source.uri always has the right thing. :) > > > > that should actually work for the bazel use case. according to siggi, we'll > > always be given a package:uri for anything that could be a package uri. > > > > it is handy though for compiling from the command line - i can glob in a list > of > > files under packages/collection rather than listing each explicitly. > > > > i can remove from this cl though if y'all would prefer. > > Ah, fair point. Ok, I see why this is a use case that is worth supporting. And > I see why in order to support it we need to implement the multi-package > resolver's restoreAbsolute() method. However, I still believe we shouldn't be > calling that method from here (nor should we be calling > sourceFactory.restoreUri() from here). Instead, we need to ensure that > library.source.uri is correct before passing the sources into the analyzer in > the first place (otherwise things will go wrong elsewhere in the analyzer). > > For an example of how we do this in analyzer_cli, see > https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_cli/lib/src/driver..... > Note that we create the Source object, then use SourceFactory.restoreUri() to > see if a better URI can be found for it, and if so, we recreate the source based > on the better URI. This is all done prior to feeding the sources into the > context, so the analyzer always sees correct URIs. > > It's particularly important that we get this right for DDC, because DDC outputs > a summary, and the URIs contained in the summary are the URIs from > library.source.uri. So if those URIs aren't correct, we'll run into trouble > when someone tries to use the summary as a dependency of another package. yeah +1 to all that @Vijay -- the DDC spot to fix this is probably https://github.com/dart-lang/dev_compiler/blob/ec5180eb173c09b62109195025bc98... that code there to map file relative paths to absolute file URIs never made me happy so this may fix it :)
PTAL Ripped out restoreUri code. Added build-root for non-package uris.
LGTM!
Description was changed from
==========
Qualify library names in packages
Partially addresses #504. This is WIP, but wanted your thoughts.
Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
E.g., this gets lowered:
import { src$interfaces } from "matcher"
- Should we thread package root through ModuleCompiler to let it packagify urls?
- How should we handle non-package urls? Pass in an explicit root dir
and go relative?
==========
to
==========
Qualify library names in packages
Partially addresses #504. This is WIP, but wanted your thoughts.
Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
E.g., this gets lowered:
import { src$interfaces } from "matcher"
- Should we thread package root through ModuleCompiler to let it packagify urls?
- How should we handle non-package urls? Pass in an explicit root dir
and go relative?
R=jmesserly@google.com
Committed:
https://github.com/dart-lang/dev_compiler/commit/bea365789e72bcc0c9e3dd7e5f87...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as bea365789e72bcc0c9e3dd7e5f87992d6d17bb3f (presubmit successful).
Message was sent while issue was closed.
lgtm - so in terms of how to use this, if I wanted the root to be "file:///" I'd just pass in "/" as the build root? https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3864: throw 'Invalid build root. $buildRoot does not contain ${uri.path}'; nit: maybe rephrase as "Invalid build root. ${uri.path} does not start with $buildRoot."? (technically it's uri.path that should contain build-root, instead of the other way around)
Message was sent while issue was closed.
https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3864: throw 'Invalid build root. $buildRoot does not contain ${uri.path}'; On 2016/04/28 15:13:28, Siggi Cherem (dart-lang) wrote: > nit: maybe rephrase as "Invalid build root. ${uri.path} does not start with > $buildRoot."? (technically it's uri.path that should contain build-root, instead > of the other way around) I was thinking "contain" in the directory sense not the string sense. Maybe I could word that better?
Message was sent while issue was closed.
https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3864: throw 'Invalid build root. $buildRoot does not contain ${uri.path}'; On 2016/04/28 15:31:45, vsm wrote: > On 2016/04/28 15:13:28, Siggi Cherem (dart-lang) wrote: > > nit: maybe rephrase as "Invalid build root. ${uri.path} does not start with > > $buildRoot."? (technically it's uri.path that should contain build-root, > instead > > of the other way around) > > I was thinking "contain" in the directory sense not the string sense. Maybe I > could word that better? Ah! :) makes sense. maybe: "is not a parent of"?
Message was sent while issue was closed.
https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:137: if (!_buildRoot.endsWith('/')) { AFAICT, buildRoot uses OS conventions, this won't work on Windows. Use `path.separator` (here and in the line below). https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3859: } else if (uri.path.startsWith(buildRoot)) { This won't be correct on Windows, because uri.path uses forward slashes, but buildRoot uses backslashes. Use uri.toFilePath() here and on line 3861. https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3861: uri.path.substring(buildRoot.length).replaceAll('/', separator); Use path.separator instead of '/'.
Message was sent while issue was closed.
vsm@google.com changed reviewers: + jakemac@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:137: if (!_buildRoot.endsWith('/')) { On 2016/04/28 15:49:30, Paul Berry wrote: > AFAICT, buildRoot uses OS conventions, this won't work on Windows. Use > `path.separator` (here and in the line below). Hmm, buildRoot might not actually be a native file path (though it is by default). jakemac passes in some sort of assert uri. Perhaps this should be a Uri?
Message was sent while issue was closed.
On 2016/04/28 21:18:04, vsm wrote: > https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... > File lib/src/compiler/code_generator.dart (right): > > https://codereview.chromium.org/1917863005/diff/80001/lib/src/compiler/code_g... > lib/src/compiler/code_generator.dart:137: if (!_buildRoot.endsWith('/')) { > On 2016/04/28 15:49:30, Paul Berry wrote: > > AFAICT, buildRoot uses OS conventions, this won't work on Windows. Use > > `path.separator` (here and in the line below). > > Hmm, buildRoot might not actually be a native file path (though it is by > default). jakemac passes in some sort of assert uri. > > Perhaps this should be a Uri? Actually, in my case it looks like it will never use the buildRoot anyways (should follow the same path as a package:uri). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
