Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993813003:
implement top-level JS annotated getters (Closed)
|
Created:
4 years, 7 months ago by Harry Terkelsen Modified:
4 years, 7 months ago CC:
dev-compiler+reviews_dartlang.org Base URL:
git@github.com:dart-lang/dev_compiler.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionimplement top-level JS annotated getters
BUG=
R=jmesserly@google.com
Committed: https://github.com/dart-lang/dev_compiler/commit/9c39ea05b06506fa0b6596ecb5413c23221931f5
Patch Set 1 #
Total comments: 26
Patch Set 2 : #Patch Set 3 : #
Total comments: 11
Patch Set 4 : #Patch Set 5 : #Messages
Total messages: 8 (2 generated)
het@google.com changed reviewers: + jacobr@google.com, jmesserly@google.com
Hey! This is a great start. I have a handful of comments mainly because this CL steps into several a tricky areas (global names, JS interop annotations, library-level state) If it helps I'm happy to chat in person. I did my comments in a linear scan, so many of them are moot based on my suggestion in _emitTopLevelName, but I left in case they become relevant. BTW feel free to ping me for a chat, I know this is probably a wall of text :) https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:84: /// The top-level reference to 'self' if this is a library tagged with @JS() reword this comment? https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:86: bool libraryTaggedJS = false; Perhaps call this "_isInteropLibrary" Note: I have a comment below that might require removing this field https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:231: libraryTaggedJS = true; is this attribute per-library or is it global for the whole module? So one important thing about DDC's CodeGenerator class, it doesn't necessarily generating things in order. If you're compiling a library cycle, and you have classes that depend on each other, it might need to generate things out of order. so it's not really safe to cache per-library state in this method. That's the reason "currentLibrary" is used often. What you could do is have a `HashMap<LibraryElement, String>` to track this property and jsPrefix. But, I have a better suggestion below in _emitTopLevelName. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:233: if (prefix != null && !prefix.isEmpty) { will we always have a name? in other words does it make sense to @JS() on the library without a name? https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:234: _jsPrefix = js.call(prefix); I feel like we'd be better off validating our user-controlled input ("prefix") before calling `js.call`. Our JS parser is an internal API, it can throw exceptions for invalid input. We do call it directly from the SDK inline-JS but that's restricted to only dart:* libraries. As a simplification, can we just split on "." and store the list of strings? Actually, since these are all properties off the global object, you don't even need to validate that they are legal identifiers. Any string will work in JS. We'll automagically quote it if necessary. (if you did have to validate, it'd be good to unify with `toJSIdentifier` in module_builder. Note, that function looks too conservative to me, it might be worth redoing to match ES2015 spec. I'm fairly sure ES allows unicode identifiers: http://www.ecma-international.org/ecma-262/6.0/#sec-identifier-names) https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:235: assert(_isValidJSName(_jsPrefix)); Should this be an error we issue? I think of "assert" as for invariants that are under our control -- e.g. an invariant in the compiler, or something Analyzer should guarantee, and they should never happen. If they do it means we have a bug in our compiler. But if the error is in user's input code, then we should issue a message and abort the compile. I think this is because they control the JS builtin string. For example this is how we deal with an import directive that is not part of the inputs. There isn't any infrastructure to help yet, because generally we try and issue this sort of error from Analyzer, that way they show up in IDE/Analyzer CLI, which is more user friendly. That is a bit harder though. Maybe a simple approach we could do now is to collect a list of AnalysisErrors as we compile inside CodeGenerator, then if we have any, return a JSModuleFile.invalid from CodeGenerator.compile. Maybe like ModuleCompiler does this for "missingPartErrorCode" and "unusedPartWarningCode". https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:302: bool _isValidJSName(JS.Expression jsName) { based on suggestion above, I'm not sure you'll need this function https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:317: JS.PropertyAccess _mergeJSNames(Iterable<JS.Expression> names) { same here, I don't think this will be needed https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:318: assert(names.every(_isValidJSName)); (this comment is probably moot based on my other ones) I don't think this assert is needed again? Since we already know these names can only flow to use from the _jsPrefix, and we already checked it. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2399: (libraryTaggedJS && e.getter.isExternal))) { `e.getter.isExternal && _libraryTaggedJS(e.library)` would allow removing libraryTaggedJS as noted above https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2407: return _mergeJSNames( rather than merging js_ast structures, IMO it would be easier to just make the List<String> and build the dotted identifier: List<String> names = ...; var prop = _dartGlobal as JS.PropertyAccess; // type not needed but for clarity for (var name in names) { prop = new JS.Access(prop, _propertyName(name)); } https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2408: [_dartGlobal, _jsPrefix, name].where((x) => x != null)); (probably a moot comment) _jsPrefix is the only one that can be null, right? so we don't need .where to check that. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:3103: var typeName; this code seems duplicated with above. (it sort of had that problem already, but the more code added the more obvious it becomes :) )
thanks John, PTAL https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:84: /// The top-level reference to 'self' if this is a library tagged with @JS() On 2016/05/18 22:20:26, John Messerly wrote: > reword this comment? Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:86: bool libraryTaggedJS = false; On 2016/05/18 22:20:27, John Messerly wrote: > Perhaps call this "_isInteropLibrary" > > Note: I have a comment below that might require removing this field Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:231: libraryTaggedJS = true; On 2016/05/18 22:20:27, John Messerly wrote: > is this attribute per-library or is it global for the whole module? > > So one important thing about DDC's CodeGenerator class, it doesn't necessarily > generating things in order. If you're compiling a library cycle, and you have > classes that depend on each other, it might need to generate things out of > order. > > so it's not really safe to cache per-library state in this method. That's the > reason "currentLibrary" is used often. > > What you could do is have a `HashMap<LibraryElement, String>` to track this > property and jsPrefix. But, I have a better suggestion below in > _emitTopLevelName. It's per-library. I made the map. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:233: if (prefix != null && !prefix.isEmpty) { On 2016/05/18 22:20:27, John Messerly wrote: > will we always have a name? in other words does it make sense to @JS() on the > library without a name? A @JS annotation on the library is required for js interop https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:234: _jsPrefix = js.call(prefix); On 2016/05/18 22:20:27, John Messerly wrote: > I feel like we'd be better off validating our user-controlled input ("prefix") > before calling `js.call`. Our JS parser is an internal API, it can throw > exceptions for invalid input. We do call it directly from the SDK inline-JS but > that's restricted to only dart:* libraries. > > As a simplification, can we just split on "." and store the list of strings? > > Actually, since these are all properties off the global object, you don't even > need to validate that they are legal identifiers. Any string will work in JS. > We'll automagically quote it if necessary. > > (if you did have to validate, it'd be good to unify with `toJSIdentifier` in > module_builder. Note, that function looks too conservative to me, it might be > worth redoing to match ES2015 spec. I'm fairly sure ES allows unicode > identifiers: > http://www.ecma-international.org/ecma-262/6.0/#sec-identifier-names) I am just splitting on dots now without validation. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:235: assert(_isValidJSName(_jsPrefix)); On 2016/05/18 22:20:26, John Messerly wrote: > Should this be an error we issue? > > I think of "assert" as for invariants that are under our control -- e.g. an > invariant in the compiler, or something Analyzer should guarantee, and they > should never happen. If they do it means we have a bug in our compiler. But if > the error is in user's input code, then we should issue a message and abort the > compile. I think this is because they control the JS builtin string. > > For example this is how we deal with an import directive that is not part of the > inputs. > > There isn't any infrastructure to help yet, because generally we try and issue > this sort of error from Analyzer, that way they show up in IDE/Analyzer CLI, > which is more user friendly. > > That is a bit harder though. Maybe a simple approach we could do now is to > collect a list of AnalysisErrors as we compile inside CodeGenerator, then if we > have any, return a JSModuleFile.invalid from CodeGenerator.compile. Maybe like > ModuleCompiler does this for "missingPartErrorCode" and "unusedPartWarningCode". I think you cannot just put arbitrary strings in the @JS annotation but I don't think there's any documentation. All of the examples I've seen use dotted identifiers. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:302: bool _isValidJSName(JS.Expression jsName) { On 2016/05/18 22:20:26, John Messerly wrote: > based on suggestion above, I'm not sure you'll need this function Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:317: JS.PropertyAccess _mergeJSNames(Iterable<JS.Expression> names) { On 2016/05/18 22:20:27, John Messerly wrote: > same here, I don't think this will be needed Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:318: assert(names.every(_isValidJSName)); On 2016/05/18 22:20:27, John Messerly wrote: > (this comment is probably moot based on my other ones) > > I don't think this assert is needed again? Since we already know these names can > only flow to use from the _jsPrefix, and we already checked it. Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2399: (libraryTaggedJS && e.getter.isExternal))) { On 2016/05/18 22:20:26, John Messerly wrote: > `e.getter.isExternal && _libraryTaggedJS(e.library)` would allow removing > libraryTaggedJS as noted above Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2407: return _mergeJSNames( On 2016/05/18 22:20:26, John Messerly wrote: > rather than merging js_ast structures, IMO it would be easier to just make the > List<String> and build the dotted identifier: > > List<String> names = ...; > var prop = _dartGlobal as JS.PropertyAccess; // type not needed but for clarity > for (var name in names) { > prop = new JS.Access(prop, _propertyName(name)); > } > Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:2408: [_dartGlobal, _jsPrefix, name].where((x) => x != null)); On 2016/05/18 22:20:27, John Messerly wrote: > (probably a moot comment) _jsPrefix is the only one that can be null, right? so > we don't need .where to check that. Done. https://codereview.chromium.org/1993813003/diff/1/lib/src/compiler/code_gener... lib/src/compiler/code_generator.dart:3103: var typeName; On 2016/05/18 22:20:27, John Messerly wrote: > this code seems duplicated with above. > > (it sort of had that problem already, but the more code added the more obvious > it becomes :) ) Done.
LGTM with one tweak to where _libraryJSPrefixes is computed https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (left): https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:3043: var annotationName = getAnnotationName(classElem, isPublicJSAnnotation); fyi Siggi made a change to this in: https://codereview.chromium.org/1998473002/diff/1/lib/src/compiler/code_gener... (I just noticed it go by...) https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:89: final _libraryJsPrefixes = new HashMap<LibraryElement, List<String>>(); BTW, we're not consistent yet, but naming convention here should be uppercase "JS", in other words, _libraryJSPrefixes https://www.dartlang.org/effective-dart/style/#do-capitalize-acronyms-and-abb... https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:238: _libraryJsPrefixes[library] = libraryJsPrefix; I think you'll need to compute these on-demand, below in _getJSName/_emitJSInterop. The problem is a case like this: import 'package:jquery/jquery.js'; // uses @JS interop main() { $('p').hide(); } If "package:jquery" is coming from a summary, we won't find it in our list of compile units. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:287: String _getJsName(Element e) { _getJSName https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:288: if (!_libraryJsPrefixes.containsKey(e.library)) return null; I think this is where you can implement the cache, if it's needed. It may or may not be worth caching these. I'm not sure how quick a lookup is via findAnnotation. It might be fast enough assuming most libraries don't have library-level metadata. Ultimately I think we'll have to discuss with Analyzer folks to get a faster way to access info like this when the summary is deserialized. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:301: JS.Expression _emitJsInterop(Element e) { _emitJSInterop
https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... File lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:89: final _libraryJsPrefixes = new HashMap<LibraryElement, List<String>>(); On 2016/05/19 17:38:04, John Messerly wrote: > BTW, we're not consistent yet, but naming convention here should be uppercase > "JS", in other words, _libraryJSPrefixes > > https://www.dartlang.org/effective-dart/style/#do-capitalize-acronyms-and-abb... Done. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:238: _libraryJsPrefixes[library] = libraryJsPrefix; On 2016/05/19 17:38:04, John Messerly wrote: > I think you'll need to compute these on-demand, below in > _getJSName/_emitJSInterop. The problem is a case like this: > > import 'package:jquery/jquery.js'; // uses @JS interop > main() { > $('p').hide(); > } > > If "package:jquery" is coming from a summary, we won't find it in our list of > compile units. Done. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:287: String _getJsName(Element e) { On 2016/05/19 17:38:04, John Messerly wrote: > _getJSName Done. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:288: if (!_libraryJsPrefixes.containsKey(e.library)) return null; On 2016/05/19 17:38:04, John Messerly wrote: > I think this is where you can implement the cache, if it's needed. It may or may > not be worth caching these. > > I'm not sure how quick a lookup is via findAnnotation. It might be fast enough > assuming most libraries don't have library-level metadata. > > Ultimately I think we'll have to discuss with Analyzer folks to get a faster way > to access info like this when the summary is deserialized. Done. https://codereview.chromium.org/1993813003/diff/40001/lib/src/compiler/code_g... lib/src/compiler/code_generator.dart:301: JS.Expression _emitJsInterop(Element e) { On 2016/05/19 17:38:05, John Messerly wrote: > _emitJSInterop Done.
Description was changed from ========== implement top-level JS annotated getters BUG= ========== to ========== implement top-level JS annotated getters BUG= R=jmesserly@google.com Committed: https://github.com/dart-lang/dev_compiler/commit/9c39ea05b06506fa0b6596ecb541... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 9c39ea05b06506fa0b6596ecb5413c23221931f5 (presubmit successful). |
