Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
587323004:
Classify method and property accesses semantically. (Closed)
|
Created:
6 years, 3 months ago by Paul Berry Modified:
6 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionClassify method and property accesses semantically.
R=brianwilkerson@google.com
Committed: https://code.google.com/p/dart/source/detail?r=40597
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address review comments #Patch Set 3 : Comment fix #
Total comments: 10
Messages
Total messages: 10 (1 generated)
paulberry@google.com changed reviewers: + brianwilkerson@google.com, johnniwinther@google.com
This is in response to Johnni's request on Wednesday for a visitor class that can be used by both tree shaking and CPS generation, which decodes the semantics of MethodInvocation, PrefixedIdentifier, PropertyAccess, and SimpleIdentifier. I wound up not implementing a visitor, but instead creating "classify..." functions, for two reasons: 1. this allows separation of concerns between the visiting strategy (which is likely to be different for tree shaking and CPS generation) and the semantic analysis itself (which will be the same). 2. it paves the way for possibly moving the semantic analysis into the analyzer's resolver pass (see below). I went ahead and modified the tree shaker to use the new "classify..." functions, but not the CPS generation code. Problems: 1. There are a large number of cases that still aren't handled properly. For instance, what if an identifier appears inside a ForEachStatement, or a ShowCombinator, or a TypeParameter? Getting all these cases right seems error prone. 2. The code uses a lot of "is" checks (which, as I understand it, tend to be slow). The "is" checks at the top of classifyBareIdentifier() seem especially troublesome. 3. Users of this code are forced to use "is" checks to decode the results. IMHO, the right way to address 1 and 2 is to merge this code into the analyzer's resolver pass, and store the resulting semantics objects in the AST. This will ensure that we cover all the cases (because resolution already must handle all the cases), and it will reduce the need for most of the "is" checks, because they are mostly to decode the information that is implicitly encoded in the static elements by resolution. I'd rather not do this until after we've stopped translating, because I don't want to have to go through the exercise of rewriting this logic in Java. I'm less certain about how to address 3. I'd love to hear comments and suggestions.
I didn't read all of the tests. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:213: } It's also possible for the static element to be a PropertyAccessorElement: typedef int F(int x); class C { F f; int three() { return f(3); } } https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:231: } This is missing the case where the target is a PrefixedIdentifier representing a class: import 'lib.dart' as 'p'; f() { p.SomeClass.staticMethod(); } https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:242: Element staticElement = node.identifier.staticElement; staticElement == suffixElement https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:266: return new DynamicPropertyAccess(node.prefix, node.identifier); Do we represent method tear-offs as property accesses? https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:284: if ((parent is FunctionDeclaration && parent.name == node) || Replace this 'if' with if (node.inDeclarationContext()) { return null; } https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:296: // TODO(paulberry): handle this case. It might be cleanest to not visit the children of a TypeName when visiting the AST structure. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:323: } Missing ParameterElement?
PTAL. Thanks for the comments, Brian. Adding method tear-offs (which can be thought of as "accessing a method as though it's a property") reminded me that I also needed to handle the converse case (invoking a property as though it's a method). And that made me realize that the division into MethodInvocationSemantics and AccessSemantics was a false dichotomy. I've also simplified the return values of the "classify..." functions so that instead of using a class hierarchy to classify the different kinds of semantics, everything returns a single concrete class (AccessSemantics), which includes an enum (AccessKind) to specify the type of access. This makes things cleaner in the clients, since they can now cover all the cases by simply switching on AccessKind. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:213: } On 2014/09/22 19:44:48, Brian Wilkerson wrote: > It's also possible for the static element to be a PropertyAccessorElement: > > typedef int F(int x); > > class C { > F f; > int three() { > return f(3); > } > } Done. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:231: } On 2014/09/22 19:44:47, Brian Wilkerson wrote: > This is missing the case where the target is a PrefixedIdentifier representing a > class: > > import 'lib.dart' as 'p'; > > f() { > p.SomeClass.staticMethod(); > } Done. It turned out that I was able to handle this by simply changing "if (target is SimpleIdentifier)" to "if (target is Identifier)". https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:242: Element staticElement = node.identifier.staticElement; On 2014/09/22 19:44:48, Brian Wilkerson wrote: > staticElement == suffixElement Done. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:266: return new DynamicPropertyAccess(node.prefix, node.identifier); On 2014/09/22 19:44:47, Brian Wilkerson wrote: > Do we represent method tear-offs as property accesses? Method tear-offs weren't being handled properly. They are now represented as an AccessSemantics object whose kind is STATIC_METHOD, LOCAL_FUNCTION, or DYNAMIC, and whose isInvoke boolean is true. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:284: if ((parent is FunctionDeclaration && parent.name == node) || On 2014/09/22 19:44:47, Brian Wilkerson wrote: > Replace this 'if' with > > if (node.inDeclarationContext()) { > return null; > } Aha, that's much better. Thanks! https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:296: // TODO(paulberry): handle this case. On 2014/09/22 19:44:48, Brian Wilkerson wrote: > It might be cleanest to not visit the children of a TypeName when visiting the > AST structure. Hmm, that may be true. I'm not sure whether I want to saddle clients of this code with that responsibility, though. For now, I've expanded the TODO comment to include this as on option. https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:323: } On 2014/09/22 19:44:48, Brian Wilkerson wrote: > Missing ParameterElement? Done.
https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:296: // TODO(paulberry): handle this case. It looks like you're currently exiting if you're visiting the child of a type name, so it isn't clear what new responsibilities clients would be have to perform that they don't have to perform today, but I could easily be missing something. https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:74: * The kind of access nit: Missing period? https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:352: AccessSemantics classifyBareIdentifier(SimpleIdentifier node) { Rename to classifySimpleIdentifier (for consistency)? https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:358: if (parent is TypeName) { Do we need to do something with type literals? https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:361: // the AST structure. Ah. I missed the connection that clients were doing the visiting and then calling these methods to classify nodes. I'll reverse my earlier comment and say that it's better to be defensive. That raises the larger question of how clients should know which nodes to visit. Clients shouldn't, for example, visit the method name in a method invocation, but should visit the target and arguments. I wonder whether there's a good way to encode that information too.
https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/1/pkg/analyzer2dart/lib/src/id... pkg/analyzer2dart/lib/src/identifier_semantics.dart:296: // TODO(paulberry): handle this case. On 2014/09/23 17:03:58, Brian Wilkerson wrote: > It looks like you're currently exiting if you're visiting the child of a type > name, so it isn't clear what new responsibilities clients would be have to > perform that they don't have to perform today, but I could easily be missing > something. Acknowledged. https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... File pkg/analyzer2dart/lib/src/identifier_semantics.dart (right): https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:74: * The kind of access On 2014/09/23 17:03:58, Brian Wilkerson wrote: > nit: Missing period? Done. https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:352: AccessSemantics classifyBareIdentifier(SimpleIdentifier node) { On 2014/09/23 17:03:58, Brian Wilkerson wrote: > Rename to classifySimpleIdentifier (for consistency)? Ok, I've renamed it, and I've also taken your advice to err on the side of defensiveness by having this function check whether the parent is a PropertyAccess, PrefixedIdentifier, or MethodInvocation. https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:358: if (parent is TypeName) { On 2014/09/23 17:03:58, Brian Wilkerson wrote: > Do we need to do something with type literals? Good point--I don't know yet. I've added a TODO comment to capture this. https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/identifier_semantics.dart:361: // the AST structure. On 2014/09/23 17:03:58, Brian Wilkerson wrote: > Ah. I missed the connection that clients were doing the visiting and then > calling these methods to classify nodes. I'll reverse my earlier comment and say > that it's better to be defensive. > > That raises the larger question of how clients should know which nodes to visit. > Clients shouldn't, for example, visit the method name in a method invocation, > but should visit the target and arguments. I wonder whether there's a good way > to encode that information too. Acknowledged.
LGTM
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 40597 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... File pkg/analyzer2dart/lib/src/tree_shaker.dart (right): https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/tree_shaker.dart:14: import 'package:analyzer2dart/src/identifier_semantics.dart'; What is the motivation for using a package and not relative import here? Because of http://dartbug.com/19579 the (unnecessary) use of package imports using symbolic links disrupts the workflow in the Dart Editor (on Windows); I end up in unanalyzed/read-only territory all the time, and have to manually find and open the actual file to inspect declarations transitively :(
Message was sent while issue was closed.
https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... File pkg/analyzer2dart/lib/src/tree_shaker.dart (right): https://codereview.chromium.org/587323004/diff/40001/pkg/analyzer2dart/lib/sr... pkg/analyzer2dart/lib/src/tree_shaker.dart:14: import 'package:analyzer2dart/src/identifier_semantics.dart'; On 2014/09/25 12:09:36, Johnni Winther wrote: > What is the motivation for using a package and not relative import here? > > Because of http://dartbug.com/19579 the (unnecessary) use of package imports > using symbolic links disrupts the workflow in the Dart Editor (on Windows); I > end up in unanalyzed/read-only territory all the time, and have to manually find > and open the actual file to inspect declarations transitively :( Hmm, I hadn't actually given it much thought. This line was automatically generated by the analyzer's "Import library" quick fix. After some discussion with Brian, we're not really sure why the analyzer inserts "package:" imports rather than relative imports in this case (Brian's recollection was that it was dictated by the style guide, but we double checked and couldn't substantiate that). Personally, I would prefer to standardize on always using relative imports when referring to files within the same package. But I'm not confident that I understand all the consequences of that. I'll start an email discussion with Bob about this to see if he has any insight, and I'll include you. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
