Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728883004:
Verify resynthesized elements by comparing text presentations. (Closed)
|
Created:
3 years, 9 months ago by scheglov Modified:
3 years, 9 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionVerify resynthesized elements by comparing text presentations.
R=paulberry@google.com, brianwilkerson@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/686a60aacf564e7cd5c82cd711e6c5c953ece397
Patch Set 1 #
Total comments: 49
Patch Set 2 : Snapshot #Patch Set 3 : Snapshot: with variable and constructor initializers. #Patch Set 4 : Ready for review. #
Total comments: 6
Patch Set 5 : Final changes. #
Messages
Total messages: 12 (1 generated)
The implementation is not complete yet. We don't write expressions (with optional writing elements and types for sub-expressions) in constructor initializers, variable initializers, default parameter values and annotations. Synthetic elements are intentionally skipped. But most of the elements are there. Inferred types are shown explicitly. We can have flags to turn on additional information for some groups of tests, e.g. name offsets, or elements in expressions. So, we don't pollute every test with this information, but can see it where it is important. applyReplacements() updates resynthesize_common.dart with recorded actual text dumps, which is useful while I'm still working on recording more and more information. We could keep it in the future in case if we will need to extend the format with additional information. If changes are small, just failure will provide the new actual text, and we could update expectations manually.
Overall it looks good. I still have my original reservations, but agree this is a reasonable approach. Not sure you were ready for detailed comments, but... https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:1: import 'dart:io'; Missing copyright notice. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:12: '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart'; That won't work on other machines. :-) https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:35: if (/*updateExpectations &&*/ actualText != expected) { I assume you'll remove the comments before committing. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:76: buffer.writeln(''); You don't need an argument to writeln. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:124: if (e.name != '') { isNotEmpty? https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:135: expect(e.isEnum, isTrue); Should we expect the opposite is writeClassElement? https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:426: } Replace with writeType2? https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:458: writeList('<', '>', elements, ', ', writeTypeParameterElement); Probably want to include if empty. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/resynthesize_ast_test.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/resynthesize_ast_test.dart:48: defineReflectiveTests(ApplyReplacements); I assume you'll reverse the commenting before committing. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/resynthesize_common.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/resynthesize_common.dart:1325: fail_library_hasExtUri() { We might want to break this file up into smaller pieces.
https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:1: import 'dart:io'; On 2017/03/03 16:09:33, Brian Wilkerson wrote: > Missing copyright notice. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:12: '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart'; On 2017/03/03 16:09:33, Brian Wilkerson wrote: > That won't work on other machines. :-) No, it won't. I will null it before commit. Then the person who wants to perform mass-(re)generation will need to set the correct value. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:35: if (/*updateExpectations &&*/ actualText != expected) { On 2017/03/03 16:09:33, Brian Wilkerson wrote: > I assume you'll remove the comments before committing. Acknowledged. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:76: buffer.writeln(''); On 2017/03/03 16:09:33, Brian Wilkerson wrote: > You don't need an argument to writeln. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:124: if (e.name != '') { On 2017/03/03 16:09:33, Brian Wilkerson wrote: > isNotEmpty? Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:135: expect(e.isEnum, isTrue); On 2017/03/03 16:09:33, Brian Wilkerson wrote: > Should we expect the opposite is writeClassElement? Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:426: } On 2017/03/03 16:09:33, Brian Wilkerson wrote: > Replace with writeType2? Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:458: writeList('<', '>', elements, ', ', writeTypeParameterElement); On 2017/03/03 16:09:33, Brian Wilkerson wrote: > Probably want to include if empty. I don't think so. This would transform "class C {}" into "class C<> {}" then.
Seems like a decent start. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:12: '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart'; On 2017/03/03 16:24:29, scheglov wrote: > On 2017/03/03 16:09:33, Brian Wilkerson wrote: > > That won't work on other machines. :-) > > No, it won't. > I will null it before commit. > Then the person who wants to perform mass-(re)generation will need to set the > correct value. You should be able to dynamically figure out the correct path using Platform.script as a starting point. (Some heuristics may be required since there are multiple entry points that might eventually lead to this code). https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:46: int testFilePathOffset = traceString.indexOf(_testPath); Reading this code, it's really non-obvious what the string you are matching against is expected to look like. Consider replacing with a regular expression that does the same job. Or add a comment, e.g.: // Assuming traceString contains "$_testPath:$ignoredText:$invocationLine", figure out the value of invocationLine. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:49: int lineOffset = testFilePathOffset + _testPath.length + ':'.length; Nit: for sanity, verify that traceString[testFilePathOffset + _testPath.length] == ':' https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:54: int expectationOffset = _testCode.indexOf("r'''", invocationOffset); It would be nice to have a sanity check here to make sure that we didn't accidentally wind up matching a string later in the file. For example, maybe verify that _testCode.substring(invocationOffset, expectationOffset) doesn't contain any of the characters `'`, `"`, or `}`. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:55: expectationOffset += 5; Why 5? I would have expected `"r'''".length`, which is 4. Are you assuming that "r''''" is followed by a newline, and trying to skip the newline? If so, I think it would be safer to change this to `expectationOffset += "r'''".length;`, and on line 59 below, replace `actualText` with `'\n$actualText'`. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:133: void writeEnumElement(ClassElement e) { I'm concerned that this method makes a lot of assumptions about the shape of the element model (e.g. it doesn't check that "index" or"values" have the propert types). Since the purpose of these tests is to make sure the element model has been rebuilt correctly, that seems like it weakens the power of the test. What if instead we unified this logic with writeClassElement, and so for `enum E { foo, bar}`, the text that was output would look comething like this: enum E { int index; static List<E> values; static E foo; static E bar; } https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:134: // var e = _e as EnumElementImpl; Delete this line. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:170: Also write out conditional URIs. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:183: buffer.write('convariant '); s/convariant/covariant/ https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:237: Also write out conditional URIs. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:304: buffer.write('('); This makes a name decorated with an offset look like a function invocation (e.g. `foo(123)`). I think that might make things confusing. Consider changing the format to something that looks less like valid Dart code, e.g. `foo@123`. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:336: buffer.write('convariant '); s/convariant/covariant/ https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:429: One of the things that was hardest to get right when resynthesizing the element model was the synthetic function for the initializer. Can we write that out (along with its return type)? https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:439: buffer.write(type.displayName); The "→" character that we use when writing out function types is hard to type in an editor. Since some of these expectations are going to get edited manually, consider adding code here that translates "→" to "->". https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:442: void writeType2(DartType type) { IMHO this is more sophisticated than we ought to be getting. We should always write out the type that's in the element model, even if it's dynamic.
https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:46: int testFilePathOffset = traceString.indexOf(_testPath); On 2017/03/03 17:22:51, Paul Berry wrote: > Reading this code, it's really non-obvious what the string you are matching > against is expected to look like. Consider replacing with a regular expression > that does the same job. Or add a comment, e.g.: > > // Assuming traceString contains "$_testPath:$ignoredText:$invocationLine", > figure out the value of invocationLine. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:49: int lineOffset = testFilePathOffset + _testPath.length + ':'.length; On 2017/03/03 17:22:52, Paul Berry wrote: > Nit: for sanity, verify that traceString[testFilePathOffset + _testPath.length] > == ':' Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:54: int expectationOffset = _testCode.indexOf("r'''", invocationOffset); On 2017/03/03 17:22:51, Paul Berry wrote: > It would be nice to have a sanity check here to make sure that we didn't > accidentally wind up matching a string later in the file. For example, maybe > verify that _testCode.substring(invocationOffset, expectationOffset) doesn't > contain any of the characters `'`, `"`, or `}`. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:55: expectationOffset += 5; On 2017/03/03 17:22:51, Paul Berry wrote: > Why 5? I would have expected `"r'''".length`, which is 4. > > Are you assuming that "r''''" is followed by a newline, and trying to skip the > newline? > > If so, I think it would be safer to change this to `expectationOffset += > "r'''".length;`, and on line 59 below, replace `actualText` with > `'\n$actualText'`. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:133: void writeEnumElement(ClassElement e) { On 2017/03/03 17:22:52, Paul Berry wrote: > I'm concerned that this method makes a lot of assumptions about the shape of the > element model (e.g. it doesn't check that "index" or"values" have the propert > types). Since the purpose of these tests is to make sure the element model has > been rebuilt correctly, that seems like it weakens the power of the test. What > if instead we unified this logic with writeClassElement, and so for `enum E { > foo, bar}`, the text that was output would look comething like this: > > enum E { > int index; > static List<E> values; > static E foo; > static E bar; > } Done. Alternatively we could add more checks for these fields, but still don't include them into text. It's not like these fields might be different for different enums. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:170: On 2017/03/03 17:22:52, Paul Berry wrote: > Also write out conditional URIs. I'm adding TODO for now. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:183: buffer.write('convariant '); On 2017/03/03 17:22:52, Paul Berry wrote: > s/convariant/covariant/ Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:237: On 2017/03/03 17:22:52, Paul Berry wrote: > Also write out conditional URIs. I'm adding TODO for now. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:304: buffer.write('('); On 2017/03/03 17:22:52, Paul Berry wrote: > This makes a name decorated with an offset look like a function invocation (e.g. > `foo(123)`). I think that might make things confusing. > > Consider changing the format to something that looks less like valid Dart code, > e.g. `foo@123`. Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:336: buffer.write('convariant '); On 2017/03/03 17:22:52, Paul Berry wrote: > s/convariant/covariant/ Done. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:429: On 2017/03/03 17:22:52, Paul Berry wrote: > One of the things that was hardest to get right when resynthesizing the element > model was the synthetic function for the initializer. Can we write that out > (along with its return type)? I'm adding TODO for now. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:439: buffer.write(type.displayName); On 2017/03/03 17:22:52, Paul Berry wrote: > The "→" character that we use when writing out function types is hard to type in > an editor. Since some of these expectations are going to get edited manually, > consider adding code here that translates "→" to "->". It looks better with this character :-( If possible, I'd like to keep this character. But if it is too annoying, I can definitely replace it with ->. I expect that initial versions of expectations will be copied from failures and then patched to the correct state. And we can also include this character in the DartDoc of the class so that it can be copy/pasted. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:442: void writeType2(DartType type) { On 2017/03/03 17:22:52, Paul Berry wrote: > IMHO this is more sophisticated than we ought to be getting. We should always > write out the type that's in the element model, even if it's dynamic. Done.
https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:237: On 2017/03/03 18:46:31, scheglov wrote: > On 2017/03/03 17:22:52, Paul Berry wrote: > > Also write out conditional URIs. > > I'm adding TODO for now. Actually, ImportElement does not expose all conditional variants, you can see only the selected one, and we already print it.
The CL is ready for review. We now write variables, fields and constructor initializers. We write documentation and annotations. We don't write local elements - local variables and functions. We also skip variable initializers. We have a theory that we might drop these elements because the clients don't use them. We don't write yet additional information about expression initializers and annotations that might be useful to verify - elements and types associated with identifiers, method invocations, etc. I have not decided yet whether we want to add it or not. Should be easy to do in writeExpression().
lgtm assuming commented-out text is fixed before committing. https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:304: buffer.write('('); On 2017/03/03 18:46:31, scheglov wrote: > On 2017/03/03 17:22:52, Paul Berry wrote: > > This makes a name decorated with an offset look like a function invocation > (e.g. > > `foo(123)`). I think that might make things confusing. > > > > Consider changing the format to something that looks less like valid Dart > code, > > e.g. `foo@123`. > > Done. It looks like you haven't done this yet. Did you decide not to, or was this an oversight? Your call whether to change this or not. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/element_text.dart:18: '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart'; Change to null before committing. As I suggested earlier, you should be able to dynamically figure this out based on Platform.script. It's ok with me if you want to postpone this to a future CL. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/element_text.dart:42: if (/*updateExpectations &&*/ actualText != expected) { Remove commented out text before committing. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... File pkg/analyzer/test/src/summary/resynthesize_ast_test.dart (right): https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/resynthesize_ast_test.dart:47: // defineReflectiveTests(AstInferredTypeTest); Uncomment before committing.
https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/1/pkg/analyzer/test/src/summa... pkg/analyzer/test/src/summary/element_text.dart:304: buffer.write('('); On 2017/03/03 23:24:35, Paul Berry wrote: > On 2017/03/03 18:46:31, scheglov wrote: > > On 2017/03/03 17:22:52, Paul Berry wrote: > > > This makes a name decorated with an offset look like a function invocation > > (e.g. > > > `foo(123)`). I think that might make things confusing. > > > > > > Consider changing the format to something that looks less like valid Dart > > code, > > > e.g. `foo@123`. > > > > Done. > > It looks like you haven't done this yet. Did you decide not to, or was this an > oversight? > > Your call whether to change this or not. I'm sorry for the oversight. I made the change, but the after another change decided to revert some changes using IDEA's local history. It accidentally reverted this change too. Fixed. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... File pkg/analyzer/test/src/summary/element_text.dart (right): https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/element_text.dart:18: '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/src/summary/resynthesize_common.dart'; On 2017/03/03 23:24:35, Paul Berry wrote: > Change to null before committing. > > As I suggested earlier, you should be able to dynamically figure this out based > on Platform.script. It's ok with me if you want to postpone this to a future > CL. I will change it to null for now. Yes, initially I implemented scraping the file with changeElementText() invocation from stack traces. But it looked as over-engineering given that we have one, max two files to update. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/element_text.dart:42: if (/*updateExpectations &&*/ actualText != expected) { On 2017/03/03 23:24:35, Paul Berry wrote: > Remove commented out text before committing. Done. https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... File pkg/analyzer/test/src/summary/resynthesize_ast_test.dart (right): https://codereview.chromium.org/2728883004/diff/60001/pkg/analyzer/test/src/s... pkg/analyzer/test/src/summary/resynthesize_ast_test.dart:47: // defineReflectiveTests(AstInferredTypeTest); On 2017/03/03 23:24:35, Paul Berry wrote: > Uncomment before committing. Done.
Description was changed from ========== Verify resynthesized elements by comparing text presentations. R=brianwilkerson@google.com, paulberry@google.com BUG= ========== to ========== Verify resynthesized elements by comparing text presentations. R=paulberry@google.com, brianwilkerson@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/686a60aacf564e7cd5c82cd711e6c5c953ece397 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 686a60aacf564e7cd5c82cd711e6c5c953ece397 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
