Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1434863003:
initial generic method comment parsing (Closed)
|
Created:
5 years, 1 month ago by Jennifer Messerly Modified:
5 years, 1 month ago CC:
reviews_dartlang.org, vsm Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptioninitial generic method comment parsing
R=brianwilkerson@google.com, leafp@google.com
Committed: https://github.com/dart-lang/sdk/commit/3cb4cdec03d3f4f728fed49bea5319d553094e2b
Patch Set 1 #
Total comments: 33
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : add two negative generic method test cases #Patch Set 5 : #Patch Set 6 : re-sort members #
Messages
Total messages: 17 (4 generated)
Description was changed from ========== generic method comment parsing ========== to ========== initial generic method comment parsing ==========
jmesserly@google.com changed reviewers: + brianwilkerson@google.com, leafp@google.com
For discussion, here's the initial scanner/parser changes.
This is missing a few important cases. Right now you'd have to write:
num/*=T*/ min/*<T extends num>*/(num/*=T*/x, num/*=T*/y);
it's not yet possible to write:
/*=T*/ f/*<T>*/(/*=T*/x, /*=T*/y);
because it doesn't know to look for comments in those places. But you could
write it as:
dynamic/*=T*/ f/*<T>*/(dynamic/*=T*/x, dynamic/*=T*/y);
I'll keep working on that, but I wanted to send out what I have so far to see
thoughts on the general approach. This is currently passing tests.
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2129: bool parseGenericMethodComments
= false;
aside, we don't actually need this flag in the parser -- it just tells us we
don't need to look for the special TokenTypes that the scanner will have
produced. After chatting with Leaf & Vijay, it sounds like maybe we can remove
this flag here, if that sounds good.
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3485: // only work inside generic
methods?
This feature weirds me out. I kind of wonder if it would be nicer to handle
these comments later, after we already know we're in a generic method, and we
know what type parameter names to expect. Then we could only match comments that
exactly refer to those type parameters.
That said, this is all a very temporary measure, and searching our corpus of
code didn't find any other uses of comments of the form /*=T*/.
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3924: if (parseGenericMethodComments)
{
as noted above, this guard shouldn't really be necessary, but it lets us skip
the comment search. Premature optimization?
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3943: * Matches a generic comment
type substitution and injects it into the token
oops, fixed this locally. Not sure how it happened
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3975: void _injectTokenList(Token
firstToken) {
I used the same pattern as the existing _injectToken.
Is there anything to be worried about here? It is mutating the token linked
list, but we seem to do that in other places. Not sure if I should be marking
each token as synthetic or something like that?
Anyway something to try out in the UI -- what does this colorize like.
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:6251: * Normally this is handled by
[_parseGenericMethodTypeParameters], but if the
fixed this locally, will be in next upload
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:9833: // TODO(jmesserly): is this a
bug? it doesn't actually use type parameters,
not sure if this existing test was broken or ...
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/scanner_test.dart (right):
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/scanner_test.dart:1063: void
_assertComment(TokenType commentType, String source, {bool
genericMethodComments: false}) {
Just reran formatter & sort members on this file, will be fixed next upload
Drive-by comments https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:2129: bool parseGenericMethodComments = false; On 2015/11/11 18:28:06, John Messerly wrote: > aside, we don't actually need this flag in the parser -- it just tells us we > don't need to look for the special TokenTypes that the scanner will have > produced. After chatting with Leaf & Vijay, it sounds like maybe we can remove > this flag here, if that sounds good. My two cents: I'd actually lean slightly toward leaving the flag here, since it will make it easier to find the code we need to remove once generic methods are implemented for real and we want to get rid of the comment syntax. But I don't feel terribly strongly either way. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3929: Token list = _scanGenericMethodComment(comment, t.offset + prefixLen); What will happen if the comment is matched by Scanner._matchGenericMethodCommentType() but is not actually grammatically valid (e.g. "/*<hi, mom!>*/"). Will the parser get horribly confused?
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:2129: bool parseGenericMethodComments = false; On 2015/11/11 19:26:53, Paul Berry wrote: > On 2015/11/11 18:28:06, John Messerly wrote: > > aside, we don't actually need this flag in the parser -- it just tells us we > > don't need to look for the special TokenTypes that the scanner will have > > produced. After chatting with Leaf & Vijay, it sounds like maybe we can remove > > this flag here, if that sounds good. > > My two cents: I'd actually lean slightly toward leaving the flag here, since it > will make it easier to find the code we need to remove once generic methods are > implemented for real and we want to get rid of the comment syntax. > > But I don't feel terribly strongly either way. good point. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3929: Token list = _scanGenericMethodComment(comment, t.offset + prefixLen); On 2015/11/11 19:26:53, Paul Berry wrote: > What will happen if the comment is matched by > Scanner._matchGenericMethodCommentType() but is not actually grammatically valid > (e.g. "/*<hi, mom!>*/"). Will the parser get horribly confused? Should be exactly as if you wrote that without the comment, a parse error of some sort. That's a good point though. We could use some more negative tests for these cases in parser_test. I'll add some.
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3927: if (t.type == TokenType.GENERIC_METHOD_TYPE_LIST) { Bug here. the /*=T*/ comments weren't working because of it. The tests weren't checking the tree well enough to catch it. Fixing this & improving the test coverage by validating the parameter and return types.
This lgtm. If you want to move it to a later stage so that you can restrict the type replacement to the scope of generic methods that's fine with me as well. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... File pkg/analyzer/test/generated/parser_test.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:213: // a<E>(b)<F>(c).d>G?(e) Did something go wrong at the .d<G> in this comment? https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:6049: expect(method.returnType, isNotNull); Maybe also expect(method.returnType.name.name, "T") ?
jmesserly@google.com changed reviewers: + paulberry@google.com
Thanks Leaf! Updated. also I can wait to hear back from Brian or Paul ... no rush ... figure y'all likely have more pressing fires to put out. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... File pkg/analyzer/test/generated/parser_test.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:213: // a<E>(b)<F>(c).d>G?(e) On 2015/11/11 21:04:05, Leaf wrote: > Did something go wrong at the .d<G> in this comment? it was like that in the test I copied from too. Fixed. I refactored too so they now share all validation logic. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:6049: expect(method.returnType, isNotNull); On 2015/11/11 21:04:05, Leaf wrote: > Maybe also expect(method.returnType.name.name, "T") ? yup, added in the latest. The /*=T*/ was actually not covered at all and totally busted.
Oh by the way, the latest version handles /*=T*/ in places types can be omitted, such as return & parameter types, as well as after final/const/var.
LGTM https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:2129: bool parseGenericMethodComments = false; I agree, I'd leave the flag in. You'll probably eventually want to add it to AnalysisOptions and use that to initialize the field, then we can plumb it all the way through to the .analysis_options file so that you can test the code more easily. That said, we had a conversation in the DDC meeting this morning about whether this should be a separate flag, or whether it should be implied by strongMode. I'm fine with the way you have it. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3485: // only work inside generic methods? Given that this only applies to return types and parameter types, perhaps the extra work should be done there rather than for all type names. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3931: // TODO(jmesserly): detach the old comment token? It shouldn't actually hurt anything if you leave it. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:4342: TypeArgumentList _maybeParseTypeArguments() { For consistency with "_parseOptionalReturnType", perhaps "_parseOptionalTypeArguments"? https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:8182: // TODO(jmesserly): should we report these messages? It would help you find bugs while using this feature, but given that it is short-lived it doesn't seem necessary. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/scanner.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/scanner.dart:249: class GenericMethodCommentToken extends CommentToken { It isn't clear to me that we need a separate class. DocumentationCommentToken exists as a separate class because it has an additional field. Instead of 'is' tests (which are expensive) we could test the token type of the comment tokens. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/scanner.dart:739: * A flag indicating whether to parse generic method comments, of the form Maybe "parse" --> "scan" in both the comment and the name? https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... File pkg/analyzer/test/generated/parser_test.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:9833: // TODO(jmesserly): is this a bug? it doesn't actually use type parameters, The implementation of the test certainly doesn't match the comment. I'd add type parameters and mark the test as failing.
paulberry@google.com changed reviewers: - paulberry@google.com
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3485: // only work inside generic methods? On 2015/11/11 21:53:42, Brian Wilkerson wrote: > Given that this only applies to return types and parameter types, perhaps the > extra work should be done there rather than for all type names. I would think it could potentially apply to other types names, such as those used in local variable declarations, e.g.: void f/*<T>*/() { var/*=T*/ x; }
https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3485: // only work inside generic methods? On 2015/11/11 21:53:42, Brian Wilkerson wrote: > Given that this only applies to return types and parameter types, perhaps the > extra work should be done there rather than for all type names. EDIT: Paul beat me to it. But here was my comment. in theory it's to cover all places generic type parameters can appear. Some examples: List<num/*=T*/> foo; foo = new List<num/*=T*/>(); <String, dynamic/*=V*/>{} if (x is dynamic/*=K*/) { ... } var firstClassType = dynamic/*=T*/; (I suspect the best choice for the "replaced" type will be whatever the upper type bound is. Typically `dynamic` but can be other types if `T extends SomeType` ... I wish this could be enforced but doesn't seem possible in the parser.)
On 2015/11/11 22:04:12, John Messerly wrote: > https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... > File pkg/analyzer/lib/src/generated/parser.dart (right): > > https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... > pkg/analyzer/lib/src/generated/parser.dart:3485: // only work inside generic > methods? > On 2015/11/11 21:53:42, Brian Wilkerson wrote: > > Given that this only applies to return types and parameter types, perhaps the > > extra work should be done there rather than for all type names. > > EDIT: Paul beat me to it. But here was my comment. > > in theory it's to cover all places generic type parameters can appear. Some > examples: > > List<num/*=T*/> foo; > > foo = new List<num/*=T*/>(); > > <String, dynamic/*=V*/>{} > > if (x is dynamic/*=K*/) { ... } > > var firstClassType = dynamic/*=T*/; > > (I suspect the best choice for the "replaced" type will be whatever the upper > type bound is. Typically `dynamic` but can be other types if `T extends > SomeType` ... I wish this could be enforced but doesn't seem possible in the > parser.) BTW, agreed with those other comments Brian, fixing things up now. Thank you!!! @Paul I also added the negative test case. It looks like it's reporting the same errors as the "real syntax", which is good. Feels good to have some coverage there :)
Thanks! Think I've addressed everyone's (super helpful) comments. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:3931: // TODO(jmesserly): detach the old comment token? On 2015/11/11 21:53:42, Brian Wilkerson wrote: > It shouldn't actually hurt anything if you leave it. Sounds good. Removed the TODO. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:4342: TypeArgumentList _maybeParseTypeArguments() { On 2015/11/11 21:53:42, Brian Wilkerson wrote: > For consistency with "_parseOptionalReturnType", perhaps > "_parseOptionalTypeArguments"? Done. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/parser.dart:8182: // TODO(jmesserly): should we report these messages? On 2015/11/11 21:53:42, Brian Wilkerson wrote: > It would help you find bugs while using this feature, but given that it is > short-lived it doesn't seem necessary. Good point. Removed the TODO. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/scanner.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/scanner.dart:249: class GenericMethodCommentToken extends CommentToken { On 2015/11/11 21:53:42, Brian Wilkerson wrote: > It isn't clear to me that we need a separate class. DocumentationCommentToken > exists as a separate class because it has an additional field. Instead of 'is' > tests (which are expensive) we could test the token type of the comment tokens. Good catch. Yeah I think my order was I made the class, then realized I wanted two distinct types ... then realized checking the .type was faster anyway ... but forgot to go back and clean up the class. Removed! https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/scanner.dart:739: * A flag indicating whether to parse generic method comments, of the form On 2015/11/11 21:53:42, Brian Wilkerson wrote: > Maybe "parse" --> "scan" in both the comment and the name? Sounds good to me. Done. https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... File pkg/analyzer/test/generated/parser_test.dart (right): https://codereview.chromium.org/1434863003/diff/1/pkg/analyzer/test/generated... pkg/analyzer/test/generated/parser_test.dart:9833: // TODO(jmesserly): is this a bug? it doesn't actually use type parameters, On 2015/11/11 21:53:42, Brian Wilkerson wrote: > The implementation of the test certainly doesn't match the comment. I'd add type > parameters and mark the test as failing. Done
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 3cb4cdec03d3f4f728fed49bea5319d553094e2b (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
