Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10332196:
Start working on unparse validation. (Closed)
|
Created:
8 years, 7 months ago by Anton Muhin Modified:
8 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionStart working on unparse validation.
R=ahe@google.com
Committed: https://code.google.com/p/dart/source/detail?r=7858
Patch Set 1 #
Total comments: 8
Patch Set 2 : Next iteration #
Total comments: 13
Patch Set 3 : Last iteration #Patch Set 4 : #Patch Set 5 : Start working on unparse validation. #
Messages
Total messages: 9 (0 generated)
And another RFC. The plan is to run some subset (say, co19 or maybe the whole suite) with additional unparse validation. Eventually this unparse validation should go into Dart backend, but for now I am simply piggybacking on JS backend (the plan is eventually to reemit code, compile to JS and run a test). Any suggestions/comments are most appreciated. https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... File lib/compiler/implementation/compiler.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:369: checkUnparse(element, this); I considered several other options, but found this one most straightforward. Other options: a) add another compiler task; b) add this functionality into validator (and pass element too) https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:626: Node newNode = newFunction.parseNode(compiler); as of now in some cases we cannot even parse unparsed code (is! provides something like varis!Type). Alas, for now error location reporting is off, I need to work some more to amend it.
https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... File lib/compiler/implementation/compiler.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:602: void checkUnparse(Element element, Compiler compiler) { I don't like having this method here :-) A task as you suggest above would be nice. Perhaps in a new file. https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:609: String unparsed = compiler.parser.parse(originalFunction).unparse(false); I think this should be originalFunction.parseNode(compiler); And perhaps you should store this result in originalNode as you use below. https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:619: originalFunction.getOrSet, Unfortunately, this will not work. Everything after line 610 should be replaced with something like: // Find the getOrSet token. // TODO(ahe): This is to frigging complicated. Simplify it. var dummy = new Listener(); Parser parser = new Parser(dummy); Token token = parser.parseModifiers(newTokens); Token getOrSet = parser.findGetOrSet(token); // TODO(ahe): This is also too frigging complicated. var script = element.getCompilationUnit().script; var lib = new LibraryElement(script); NodeListener listener = new NodeListener(compiler, lib); parser = new Parser(listener); compiler.withCurrentElement(lib, () { parser.parseFunction(newTokens, getOrSet); }); FunctionExpression newNode = listener.popNode();
PTAL https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... File lib/compiler/implementation/compiler.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:602: void checkUnparse(Element element, Compiler compiler) { On 2012/05/21 16:53:29, ahe wrote: > I don't like having this method here :-) > > A task as you suggest above would be nice. Perhaps in a new file. Done. https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:609: String unparsed = compiler.parser.parse(originalFunction).unparse(false); On 2012/05/21 16:53:29, ahe wrote: > I think this should be originalFunction.parseNode(compiler); > > And perhaps you should store this result in originalNode as you use below. Done. https://chromiumcodereview.appspot.com/10332196/diff/1/lib/compiler/implement... lib/compiler/implementation/compiler.dart:619: originalFunction.getOrSet, On 2012/05/21 16:53:29, ahe wrote: > Unfortunately, this will not work. > > Everything after line 610 should be replaced with something like: > > > // Find the getOrSet token. > // TODO(ahe): This is to frigging complicated. Simplify it. > var dummy = new Listener(); > Parser parser = new Parser(dummy); > Token token = parser.parseModifiers(newTokens); > Token getOrSet = parser.findGetOrSet(token); > > // TODO(ahe): This is also too frigging complicated. > var script = element.getCompilationUnit().script; > var lib = new LibraryElement(script); > NodeListener listener = new NodeListener(compiler, lib); > parser = new Parser(listener); > compiler.withCurrentElement(lib, () { > parser.parseFunction(newTokens, getOrSet); > }); > > FunctionExpression newNode = listener.popNode(); Done. https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; this trick is sketchy, but allows nice diagnostics. I can probably make it less sketchy with proper tweaking of reportDiagnostics if you prefer it this way.
LGTM. I removed the R line, feel free to add it back, but please only include people who did say "LGTM". https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:14: if (!validateUnparse) { Just one line: if (!validateUnparse) return; https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:18: if (element is! PartialFunctionElement) { Ditto. https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:30: // TODO(ahe): This is to frigging complicated. Simplify it. It's great that you copied this verbatim :-) https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; On 2012/05/21 19:14:39, antonmuhin wrote: > this trick is sketchy, but allows nice diagnostics. > > I can probably make it less sketchy with proper tweaking of reportDiagnostics if > you prefer it this way. I don't understand why you have to do this. How is your new handler different?
Peter, thanks a lot for review. I'll address remaining comments once in the office. https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; The old handler will lookup SourceFile via uri and will find the full version of the file. Therefore span would be incorrect. Example, js_helper has stringToString function with wrongly unparsed construct (is!) When we parse unparsed code, span woud point into copyright header of js_helper.dart :) With overridden handler: file:///usr/local/google/home/antonm/dart-all/dart/lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' String stringToString(var value){var res=value.toString(); if(resis!String)throw new IllegalArgumentException(value); return res;} ^ info: compiler cancelled: unexpected token '!' w/o it: lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file ^ info: compiler cancelled: unexpected token '!' info: compilation failed On 2012/05/22 08:49:04, ahe wrote: > On 2012/05/21 19:14:39, antonmuhin wrote: > > this trick is sketchy, but allows nice diagnostics. > > > > I can probably make it less sketchy with proper tweaking of reportDiagnostics > if > > you prefer it this way. > > I don't understand why you have to do this. How is your new handler different?
https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; On 2012/05/22 08:56:32, antonmuhin wrote: > The old handler will lookup SourceFile via uri and will find the full version of > the file. Therefore span would be incorrect. Example, js_helper has > stringToString function with wrongly unparsed construct (is!) When we parse > unparsed code, span woud point into copyright header of js_helper.dart :) > > With overridden handler: > > file:///usr/local/google/home/antonm/dart-all/dart/lib/compiler/implementation/lib/js_helper.dart:1:68: > unexpected token '!' > > String stringToString(var value){var res=value.toString(); if(resis!String)throw > new IllegalArgumentException(value); return res;} > > ^ > > > info: compiler cancelled: unexpected token '!' > > > > w/o it: > > lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' > > > // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file > > > ^ > > > info: compiler cancelled: unexpected token '!' > > > info: compilation failed > > > > On 2012/05/22 08:49:04, ahe wrote: > > On 2012/05/21 19:14:39, antonmuhin wrote: > > > this trick is sketchy, but allows nice diagnostics. > > > > > > I can probably make it less sketchy with proper tweaking of > reportDiagnostics > > if > > > you prefer it this way. > > > > I don't understand why you have to do this. How is your new handler different? > The source file should be found in the "currentElement" of the compiler. So it should be possible to remove your custom handler. If that still doesn't work, the element stored in lib is not setup correctly and we should figure out what is wrong.
https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:14: if (!validateUnparse) { On 2012/05/22 08:49:04, ahe wrote: > Just one line: > > if (!validateUnparse) return; Done. https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:18: if (element is! PartialFunctionElement) { On 2012/05/22 08:49:04, ahe wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:30: // TODO(ahe): This is to frigging complicated. Simplify it. On 2012/05/22 08:49:04, ahe wrote: > It's great that you copied this verbatim :-) :) https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; As far as I understand it goes like this: we get into Compiler.cancel with null for a node and proper token. Therefore we construct SourceSpan with offsets of synthesized tokens, but with URI of original file (because we use it for synthesizedLib). Now SourceSpan has no notion of original SourceFile and hence error handler refetches it from a sourceFile map in dart2js. If I make a fake URI, error handler will just choke as there will be no corresponding entry. I would fix it by passing SourceFile instead of uri into error handler. However, I suspect it was done differently as that minimized compiler API---in compiler API there is no notion of SourceFile, but ironically dart2js imports it. Another option would be to pass fully formatted message into error handler. What will be your preferences? And in any event, I'll land this change w/ error handling overrides and defer it to future CL (with corresponding TODO). On 2012/05/22 09:04:45, ahe wrote: > On 2012/05/22 08:56:32, antonmuhin wrote: > > The old handler will lookup SourceFile via uri and will find the full version > of > > the file. Therefore span would be incorrect. Example, js_helper has > > stringToString function with wrongly unparsed construct (is!) When we parse > > unparsed code, span woud point into copyright header of js_helper.dart :) > > > > With overridden handler: > > > > > file:///usr/local/google/home/antonm/dart-all/dart/lib/compiler/implementation/lib/js_helper.dart:1:68: > > unexpected token '!' > > > > > String stringToString(var value){var res=value.toString(); > if(resis!String)throw > > new IllegalArgumentException(value); return res;} > > > > > ^ > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > > > w/o it: > > > > lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' > > > > > > > > // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file > > > > > > > > ^ > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > info: compilation failed > > > > > > > > > > On 2012/05/22 08:49:04, ahe wrote: > > > On 2012/05/21 19:14:39, antonmuhin wrote: > > > > this trick is sketchy, but allows nice diagnostics. > > > > > > > > I can probably make it less sketchy with proper tweaking of > > reportDiagnostics > > > if > > > > you prefer it this way. > > > > > > I don't understand why you have to do this. How is your new handler > different? > > > > The source file should be found in the "currentElement" of the compiler. So it > should be possible to remove your custom handler. > > If that still doesn't work, the element stored in lib is not setup correctly and > we should figure out what is wrong.
https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; Shoot. I forgot about the compiler API. It would be cleaner with a compiler subclass for the validation, but your hack works for now. So please keep it. I think it is important that the error message is correct in this case. Otherwise it will be pretty hard to figure out what went wrong. On 2012/05/22 15:34:32, antonmuhin wrote: > As far as I understand it goes like this: > > we get into Compiler.cancel with null for a node and proper token. Therefore we > construct SourceSpan with offsets of synthesized tokens, but with URI of > original file (because we use it for synthesizedLib). Now SourceSpan has no > notion of original SourceFile and hence error handler refetches it from a > sourceFile map in dart2js. > > If I make a fake URI, error handler will just choke as there will be no > corresponding entry. > > I would fix it by passing SourceFile instead of uri into error handler. > > However, I suspect it was done differently as that minimized compiler API---in > compiler API there is no notion of SourceFile, but ironically dart2js imports > it. > > Another option would be to pass fully formatted message into error handler. > > What will be your preferences? > > And in any event, I'll land this change w/ error handling overrides and defer it > to future CL (with corresponding TODO). > > On 2012/05/22 09:04:45, ahe wrote: > > On 2012/05/22 08:56:32, antonmuhin wrote: > > > The old handler will lookup SourceFile via uri and will find the full > version > > of > > > the file. Therefore span would be incorrect. Example, js_helper has > > > stringToString function with wrongly unparsed construct (is!) When we parse > > > unparsed code, span woud point into copyright header of js_helper.dart :) > > > > > > With overridden handler: > > > > > > > > > file:///usr/local/google/home/antonm/dart-all/dart/lib/compiler/implementation/lib/js_helper.dart:1:68: > > > unexpected token '!' > > > > > > > > > String stringToString(var value){var res=value.toString(); > > if(resis!String)throw > > > new IllegalArgumentException(value); return res;} > > > > > > > > > ^ > > > > > > > > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > > > > > > > > > > > w/o it: > > > > > > lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' > > > > > > > > > > > > > > > // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS > file > > > > > > > > > > > > > > ^ > > > > > > > > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > > > > > > > > info: compilation failed > > > > > > > > > > > > > > > > > > On 2012/05/22 08:49:04, ahe wrote: > > > > On 2012/05/21 19:14:39, antonmuhin wrote: > > > > > this trick is sketchy, but allows nice diagnostics. > > > > > > > > > > I can probably make it less sketchy with proper tweaking of > > > reportDiagnostics > > > > if > > > > > you prefer it this way. > > > > > > > > I don't understand why you have to do this. How is your new handler > > different? > > > > > > > The source file should be found in the "currentElement" of the compiler. So it > > should be possible to remove your custom handler. > > > > If that still doesn't work, the element stored in lib is not setup correctly > and > > we should figure out what is wrong. >
https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... File lib/compiler/implementation/unparse_validator.dart (right): https://chromiumcodereview.appspot.com/10332196/diff/4001/lib/compiler/implem... lib/compiler/implementation/unparse_validator.dart:44: final oldHandler = compiler.handler; I am sorry, I've already submitted w/o it. Sending you hopefully better variant. On 2012/05/22 15:50:56, ahe wrote: > Shoot. I forgot about the compiler API. It would be cleaner with a compiler > subclass for the validation, but your hack works for now. So please keep it. I > think it is important that the error message is correct in this case. Otherwise > it will be pretty hard to figure out what went wrong. > > On 2012/05/22 15:34:32, antonmuhin wrote: > > As far as I understand it goes like this: > > > > we get into Compiler.cancel with null for a node and proper token. Therefore > we > > construct SourceSpan with offsets of synthesized tokens, but with URI of > > original file (because we use it for synthesizedLib). Now SourceSpan has no > > notion of original SourceFile and hence error handler refetches it from a > > sourceFile map in dart2js. > > > > If I make a fake URI, error handler will just choke as there will be no > > corresponding entry. > > > > I would fix it by passing SourceFile instead of uri into error handler. > > > > However, I suspect it was done differently as that minimized compiler API---in > > compiler API there is no notion of SourceFile, but ironically dart2js imports > > it. > > > > Another option would be to pass fully formatted message into error handler. > > > > What will be your preferences? > > > > And in any event, I'll land this change w/ error handling overrides and defer > it > > to future CL (with corresponding TODO). > > > > On 2012/05/22 09:04:45, ahe wrote: > > > On 2012/05/22 08:56:32, antonmuhin wrote: > > > > The old handler will lookup SourceFile via uri and will find the full > > version > > > of > > > > the file. Therefore span would be incorrect. Example, js_helper has > > > > stringToString function with wrongly unparsed construct (is!) When we > parse > > > > unparsed code, span woud point into copyright header of js_helper.dart :) > > > > > > > > With overridden handler: > > > > > > > > > > > > > > file:///usr/local/google/home/antonm/dart-all/dart/lib/compiler/implementation/lib/js_helper.dart:1:68: > > > > unexpected token '!' > > > > > > > > > > > > > > String stringToString(var value){var res=value.toString(); > > > if(resis!String)throw > > > > new IllegalArgumentException(value); return res;} > > > > > > > > > > > > > > ^ > > > > > > > > > > > > > > > > > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > > > > > > > > > > > > > > > > > > > > > w/o it: > > > > > > > > lib/compiler/implementation/lib/js_helper.dart:1:68: unexpected token '!' > > > > > > > > > > > > > > > > > > > > > > > > // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS > > file > > > > > > > > > > > > > > > > > > > > > ^ > > > > > > > > > > > > > > > > > > > > > > > > info: compiler cancelled: unexpected token '!' > > > > > > > > > > > > > > > > > > > > > > > > info: compilation failed > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 2012/05/22 08:49:04, ahe wrote: > > > > > On 2012/05/21 19:14:39, antonmuhin wrote: > > > > > > this trick is sketchy, but allows nice diagnostics. > > > > > > > > > > > > I can probably make it less sketchy with proper tweaking of > > > > reportDiagnostics > > > > > if > > > > > > you prefer it this way. > > > > > > > > > > I don't understand why you have to do this. How is your new handler > > > different? > > > > > > > > > > The source file should be found in the "currentElement" of the compiler. So > it > > > should be possible to remove your custom handler. > > > > > > If that still doesn't work, the element stored in lib is not setup correctly > > and > > > we should figure out what is wrong. > > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
