Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863053003:
Adds support for --generic-method-syntax (Closed)
|
Created:
4 years, 8 months ago by eernst Modified:
3 years, 11 months ago CC:
reviews_dartlang.org, floitsch Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdds support for --generic-method-syntax
This CL adds support for parsing and ignoring declarations of type
parameters on methods and functions, and passing type arguments to
method and function invocations. The type parameters thus declared do
not get a representation during static analysis, so any usage will
cause warnings about undefined types; hence, this CL transforms usage
of generic methods from syntax errors to static warnings. A followup
CL will eliminate the static warnings.
R=johnniwinther@google.com
Committed: https://github.com/dart-lang/sdk/commit/440b17901ef07ee68ec911f0f13c9bcc72857c18
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added support for parsing generic methods (buggy, will upload again when fixed) #
Total comments: 29
Patch Set 3 : Review response #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Review response 2 (braces) #Patch Set 6 : Rebased up to current github master #
Messages
Total messages: 24 (4 generated)
eernst@google.com changed reviewers: + johnniwinther@google.com
First patch just adds support for the --generic-method-syntax option, uploaded separately for better readability. Next patch(es) will add support for actually parsing the generic methods.
lgtm https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_func... File tests/language/generic_functions_test.options (right): https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_func... tests/language/generic_functions_test.options:3: enableGenericMethodSyntax: true Is the 'enableGenericMethods' not currently supported by the analyzer, i.e. is this change just in preparation for support?
eernst@google.com changed reviewers: + ahe@google.com
Early upload (still buggy), to ensure Peter gets an opportunity to respond before his vacation. Hope to upload a fixed version later today. https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_func... File tests/language/generic_functions_test.options (right): https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_func... tests/language/generic_functions_test.options:3: enableGenericMethodSyntax: true On 2016/04/06 12:34:58, Johnni Winther wrote: > Is the 'enableGenericMethods' not currently supported by the analyzer, i.e. is > this change just in preparation for support? Oops, same spelling but not same thing: This change was wrong and it is reverted in the next patch ('enableGenericMethods' is supported by the analyzer today). https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_meth... File tests/language/generic_methods_test.options (right): https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_meth... tests/language/generic_methods_test.options:3: enableGenericMethodSyntax: true Wrong update, reverted in next patch. https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_send... File tests/language/generic_sends_test.options (right): https://codereview.chromium.org/1863053003/diff/1/tests/language/generic_send... tests/language/generic_sends_test.options:3: enableGenericMethodSyntax: true Wrong update, reverted in next patch.
I've looked at the code in parser directory. I don't think it's great to resolve the grammar ambiguity in the way it's done, but if that's what required from the spec, it's OK. However, I'll strongly encourage you to reconsider this part of the specification. The goal should be to produce great error messages and enable code completion while you're typing and you haven't typed everything that's needed in a grammatical correct program. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... File pkg/compiler/lib/src/parser/parser.dart (right): https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:108: final bool _enableGenericMethodSyntax; Please avoid private fields, a subclass outside this library may need access to this for diet parsing, testing, etc. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:110: Parser(this.listener, ParserOptions _parserOptions, Remove _ from parameter. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:112: parserOptions = _parserOptions, In: parserOption = parserOptions The left-hand side refers to the field, the right hand side refers to the parameter because parameters aren't in scope on the left-hand side. If you want to be explicit about that, use: this.parserOptions = parserOptions But it isn't necessary. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:534: /// Checks for '<' type (',' type)* '>', returns null iff failing. Avoid using academic short-hand and notation in documentation. "iff" is an abbreviation of "if and only" that matters a lot when being in mathematics, but in documentation, simply using "if" is better. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:534: /// Checks for '<' type (',' type)* '>', returns null iff failing. How about: Returns next token after match token if [token] matches `'<' type (',' type)* '>'`. Otherwise returns null. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:538: Token tryParseTypeArgumentsNested(Token token) { Below I question if this method is necessary. However, if it really is, it should be shared with tryParseTypeArguments, like so: Token tryParseTypeArguments(Token token, bool isNested) { } https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:539: final kind = token.kind; Remove this variable. It's confusing that token is overwritten but kind isn't. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:560: /// Checks for identifier ('.' identifier)?, returns null iff failing. iff -> if. What does "failing" mean? How about: Returns next token if [token] matches `identifier ('.' identifier)?`. Otherwise returns null. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:562: Token tryParseQualified(Token token) { There's probably suboptimal error recovery here. For example, it's natural for people to try to access type variables as fields of classes or instances: lib a; class Foo<T> { } lib b; import 'a.dart' as a; main() { m<a.Foo.T>(); } https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:571: /// Checks for typeName typeArguments? and returns null iff failing. iff -> if, but what about: Returns next token after match if [token] matches `typeName typeArguments?`. Otherwise returns null. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:581: /// Checks for '<' type (',' type)* '>' '(' and returns null iff failing. iff -> if, but what about: Returns last token of match if [token] matches `'<' type (',' type)* '>' '('. Otherwise returns null. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:592: token = tryParseType(token); Is this really necessary? If so, perhaps push back on the specification to ensure that it isn't. I'm concerned that this approach will make it hard to produce good error messages. As far as I understand, the syntax is introduced with a known ambiguity that is resolved in favor of generic arguments based on the argument that m<...>(...) is infrequent. If that is indeed the case, it follows that if the parser sees m<...>(...), the user is likely attempting to pass arguments to a generic method. If so, what happens if you accidentally forget or have an extra comma? m<a b>(c); m<a,b,>(c); In these cases, you'll probably get some weird errors and the parser will not recover well from the error. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:602: /// Returns true iff the tokens starting from [token] match iff->if. We normally emit "otherwise returns false." https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token token) { Rename to "isValidMethodTypeArguments" to signal that a trailing parenthesis is required? Can I assume that you can't "curry" type arguments as part of a tear-off operation? That is, this doesn't work (assuming m is a generic method): var f = m<a, b>; f(); https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:609: } Please reverse the order of all these methods. It's easier to read code if you start with the main entry point (isValidTypeArguments) followed by the methods it uses. Generally, try to sort methods so that f comes before g if f calls g.
On 2016/04/08 09:53:33, ahe wrote: > I've looked at the code in parser directory. I don't think it's great to resolve > the grammar ambiguity in the way it's done, but if that's what required from the > spec, it's OK. > > However, I'll strongly encourage you to reconsider this part of the > specification. The goal should be to produce great error messages and enable > code completion while you're typing and you haven't typed everything that's > needed in a grammatical correct program. > Just saw this, will return after TGIF with a more complete response. But for now: Peter, how would you like the ambiguity to be resolved? And how could we achieve those qualities regarding code completion and error messages? The spec for these things has not been written (there is https://codereview.chromium.org/1177073002/, but that's explicitly marked as "just a draft").
On 2016/04/08 11:59:26, eernst wrote: > On 2016/04/08 09:53:33, ahe wrote: > > I've looked at the code in parser directory. I don't think it's great to > resolve > > the grammar ambiguity in the way it's done, but if that's what required from > the > > spec, it's OK. > > > > However, I'll strongly encourage you to reconsider this part of the > > specification. The goal should be to produce great error messages and enable > > code completion while you're typing and you haven't typed everything that's > > needed in a grammatical correct program. > > > > Just saw this, will return after TGIF with a more complete response. But for > now: > > Peter, how would you like the ambiguity to be resolved? And how could we achieve > those qualities regarding code completion and error messages? > > The spec for these things has not been written (there is > https://codereview.chromium.org/1177073002/, but that's explicitly marked as > "just a draft"). I'm not sure I can effectively explain it without a whiteboard. Can we talk face-to-face about that when I'm back from vacation? You can land this CL if you have an up-to-date l-g-t-m from Johnni.
On 2016/04/08 12:04:56, ahe wrote: > On 2016/04/08 11:59:26, eernst wrote: > > On 2016/04/08 09:53:33, ahe wrote: > > > I've looked at the code in parser directory. I don't think it's great to > > resolve > > > the grammar ambiguity in the way it's done, but if that's what required from > > the > > > spec, it's OK. > > > > > > However, I'll strongly encourage you to reconsider this part of the > > > specification. The goal should be to produce great error messages and enable > > > code completion while you're typing and you haven't typed everything that's > > > needed in a grammatical correct program. > > > > > > > Just saw this, will return after TGIF with a more complete response. But for > > now: > > > > Peter, how would you like the ambiguity to be resolved? And how could we > achieve > > those qualities regarding code completion and error messages? > > > > The spec for these things has not been written (there is > > https://codereview.chromium.org/1177073002/, but that's explicitly marked as > > "just a draft"). > > I'm not sure I can effectively explain it without a whiteboard. Can we talk > face-to-face about that when I'm back from vacation? You can land this CL if you > have an up-to-date l-g-t-m from Johnni. Very good, let's do that!
On 2016/04/08 13:29:05, eernst wrote: > On 2016/04/08 12:04:56, ahe wrote: > > On 2016/04/08 11:59:26, eernst wrote: > > > On 2016/04/08 09:53:33, ahe wrote: > > > > I've looked at the code in parser directory. I don't think it's great to > > > resolve > > > > the grammar ambiguity in the way it's done, but if that's what required > from > > > the > > > > spec, it's OK. > > > > > > > > However, I'll strongly encourage you to reconsider this part of the > > > > specification. The goal should be to produce great error messages and > enable > > > > code completion while you're typing and you haven't typed everything > that's > > > > needed in a grammatical correct program. > > > > > > > > > > Just saw this, will return after TGIF with a more complete response. But for > > > now: > > > > > > Peter, how would you like the ambiguity to be resolved? And how could we > > achieve > > > those qualities regarding code completion and error messages? > > > > > > The spec for these things has not been written (there is > > > https://codereview.chromium.org/1177073002/, but that's explicitly marked as > > > "just a draft"). > > > > I'm not sure I can effectively explain it without a whiteboard. Can we talk > > face-to-face about that when I'm back from vacation? You can land this CL if > you > > have an up-to-date l-g-t-m from Johnni. > > Very good, let's do that! (I'm still planning to have the fixes ready for the detailed comments this afternoon, so there will be another patch upload on that.)
PTAL, remaining issues: Having or not having a field `enableGenericMethodSyntax`, and merging or not merging `tryParseNestedTypeArguments` and `tryParseMethodTypeArguments` (renamed). Everything concerned with error reporting will be discussed an about a week irl, so I haven't addressed that topic in this patch. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... File pkg/compiler/lib/src/parser/parser.dart (right): https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:108: final bool _enableGenericMethodSyntax; On 2016/04/08 09:53:33, ahe wrote: > Please avoid private fields, a subclass outside this library may need access to > this for diet parsing, testing, etc. I'm not sure what the right approach would be here. This value is available as 'parserOptions.enableGenericMethodSyntax' and the only reason why I do not use that expression in the few locations where this value is needed is that it is (presumably) a rather performance critical set of locations (parsing an expression will happen very, very frequently, and checking whether it is a message send with type arguments will happen very frequently, too), so I wanted use a field in the same class rather than a getter invocation on another object. But clients outside the parser itself would hardly need to worry so much about micro-performance. So is the extra field introduced here worthwhile at all, just to avoid that getter invocation? And if this kind of micro-optimization is justified (inside the parser), would it be useful to export such a field to the world? At this point I just made it public. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:110: Parser(this.listener, ParserOptions _parserOptions, On 2016/04/08 09:53:32, ahe wrote: > Remove _ from parameter. Done. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:112: parserOptions = _parserOptions, On 2016/04/08 09:53:32, ahe wrote: > In: > > parserOption = parserOptions > > The left-hand side refers to the field, the right hand side refers to the > parameter because parameters aren't in scope on the left-hand side. If you want > to be explicit about that, use: > > this.parserOptions = parserOptions > > But it isn't necessary. Acknowledged. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:534: /// Checks for '<' type (',' type)* '>', returns null iff failing. On 2016/04/08 09:53:33, ahe wrote: > How about: > > Returns next token after match token if [token] matches `'<' type (',' type)* > '>'`. > Otherwise returns null. Done. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:538: Token tryParseTypeArgumentsNested(Token token) { On 2016/04/08 09:53:32, ahe wrote: > Below I question if this method is necessary. However, if it really is, it > should be shared with tryParseTypeArguments, like so: > > Token tryParseTypeArguments(Token token, bool isNested) { > } The old (abandoned) CL did express the same distinction using a boolean argument, but the two bodies actually differ in several ways: ----------------------------------------------------------- * if (!identical(token?.kind, LT_TOKEN)) return null; * token = token.next; if (token == null) return null; token = tryParseType(token); * while (token != null && identical(token.kind, COMMA_TOKEN)) { * token = token.next; * if (token == null) return null; * token = tryParseType(token); * } if (token == null) return null; if (identical(token.kind, GT_TOKEN)) return token.next; if (!identical(token.kind, GT_GT_TOKEN)) return null; Token syntheticToken = new SymbolToken(GT_INFO, token.charOffset + 1); syntheticToken.next = token.next; return syntheticToken; ----------------------------------------------------------- * if (!identical(token?.kind, LT_TOKEN)) return null; BeginGroupToken beginToken = token; Token endToken = beginToken.endGroup; * token = token.next; if (token == null || endToken == null || !identical(endToken.next?.kind, OPEN_PAREN_TOKEN)) return null; token = tryParseType(token); * while (token != null && identical(token.kind, COMMA_TOKEN)) { * token = token.next; * if (token == null) return null; * token = tryParseType(token); * } if (!identical(token?.kind, GT_TOKEN)) return null; return token.next; ----------------------------------------------------------- I guess the readability of a single method would be lower than the readability of these two methods separately, and it would also cost a little bit extra at runtime when we keep testing that boolean argument. So do you still think it's better to merge them? https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:539: final kind = token.kind; On 2016/04/08 09:53:33, ahe wrote: > Remove this variable. It's confusing that token is overwritten but kind isn't. Done. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:560: /// Checks for identifier ('.' identifier)?, returns null iff failing. On 2016/04/08 09:53:33, ahe wrote: > iff -> if. > > What does "failing" mean? How about: > > Returns next token if [token] matches `identifier ('.' identifier)?`. > Otherwise returns null. Rephrased in a similar manner as several earlier doc comments. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:562: Token tryParseQualified(Token token) { On 2016/04/08 09:53:32, ahe wrote: > There's probably suboptimal error recovery here. For example, it's natural for > people to try to access type variables as fields of classes or instances: > > lib a; > class Foo<T> { > } > > lib b; > import 'a.dart' as a; > > main() { > m<a.Foo.T>(); > } It would be nice if they _could_ do that. ;) For improved error recovery, we will discuss in depth what to do in about a week, so I did not take any actions here at this point. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:571: /// Checks for typeName typeArguments? and returns null iff failing. On 2016/04/08 09:53:32, ahe wrote: > iff -> if, but what about: > > Returns next token after match if [token] matches `typeName typeArguments?`. > Otherwise returns null. Rephrased in a similar manner as several earlier doc comments. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:581: /// Checks for '<' type (',' type)* '>' '(' and returns null iff failing. On 2016/04/08 09:53:33, ahe wrote: > iff -> if, but what about: > > Returns last token of match if [token] matches `'<' type (',' type)* '>' '('. > Otherwise returns null. Rephrased in a similar manner as several earlier doc comments. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:602: /// Returns true iff the tokens starting from [token] match On 2016/04/08 09:53:32, ahe wrote: > iff->if. > > We normally emit "otherwise returns false." Rephrased in a similar manner as several earlier doc comments, which includes the "otherwise returns false" phrase. https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token token) { On 2016/04/08 09:53:32, ahe wrote: > Rename to "isValidMethodTypeArguments" to signal that a trailing parenthesis is > required? Makes sense, done. (This deviates from the naming of parsing methods after the grammar non-terminals---but we're beyond that anyway, because of the trailing '('). > Can I assume that you can't "curry" type arguments as part of a tear-off > operation? That is, this doesn't work (assuming m is a generic method): > > var f = m<a, b>; > f(); Right, Florian did not want to support that, and Leaf seems to be relatively willing to leave out the curried construct (a rather short lambda can be used as a work-around). https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:609: } On 2016/04/08 09:53:32, ahe wrote: > Please reverse the order of all these methods. It's easier to read code if you > start with the main entry point (isValidTypeArguments) followed by the methods > it uses. Generally, try to sort methods so that f comes before g if f calls g. Ah, that's funny. The literate programming credo (and a core design idea behind Pascal) was "Don't use anything before it is defined". You would then be able to read the code top-down, and you would never encounter an unknown "word" along the way, because all definitions appear before their first use. Of course, mutual recursion calls for an extra trick, but the main idea would be bottom up: Define "small" things first, and then go to "bigger and bigger" ones, ending in `MAIN`. Your preference is obviously top-down, possibly based on a reading where we get the overview first, and each of the smaller pieces are understood (better) when we get to them, because we know in which context they are being used. I'm not sure I've heard any specific policies on this before; various preferences about class member declaration order have been discussed, and those discussions were somewhat bottom-up oriented: fields and constructors come first (a bottom-up element), and private methods go just before the public method that uses them (also bottom-up), but the ordering of static vs instance members etc. makes the whole issue less clear-cut. My general impression was, however, that bottom-up is the most common preference. Anyway, I've transformed these methods to use the top-down style.
Response to irl discussion. I'm currently running the longer test series (will finish in about 80 minutes), but the uploaded code has already passed the test suites 'language' and 'corelib', and it produces the expected warnings when running 'dart2js_developer --generic-method-syntax ..' on the tests where generic methods are used.
floitsch@google.com changed reviewers: + floitsch@google.com
https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... File pkg/compiler/lib/src/parser/parser.dart (right): https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token token) { On 2016/04/08 17:07:10, eernst wrote: > On 2016/04/08 09:53:32, ahe wrote: > > Rename to "isValidMethodTypeArguments" to signal that a trailing parenthesis > is > > required? > > Makes sense, done. (This deviates from the naming of parsing methods after the > grammar non-terminals---but we're beyond that anyway, because of the trailing > '('). > > > Can I assume that you can't "curry" type arguments as part of a tear-off > > operation? That is, this doesn't work (assuming m is a generic method): > > > > var f = m<a, b>; > > f(); > > Right, Florian did not want to support that, and Leaf seems to be relatively > willing to leave out the curried construct (a rather > short lambda can be used as a work-around). Fwiw, this CL only adds syntactic support. The semantic meaning is independent, and both options are still open (although I would like to avoid currying).
On 2016/04/11 11:00:14, floitsch wrote: > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > File pkg/compiler/lib/src/parser/parser.dart (right): > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token > token) { > On 2016/04/08 17:07:10, eernst wrote: > > On 2016/04/08 09:53:32, ahe wrote: > > > Rename to "isValidMethodTypeArguments" to signal that a trailing parenthesis > > is > > > required? > > > > Makes sense, done. (This deviates from the naming of parsing methods after the > > grammar non-terminals---but we're beyond that anyway, because of the trailing > > '('). > > > > > Can I assume that you can't "curry" type arguments as part of a tear-off > > > operation? That is, this doesn't work (assuming m is a generic method): > > > > > > var f = m<a, b>; > > > f(); > > > > Right, Florian did not want to support that, and Leaf seems to be relatively > > willing to leave out the curried construct (a rather > > short lambda can be used as a work-around). > > Fwiw, this CL only adds syntactic support. The semantic meaning is independent, > and both options are still open (although I would like to avoid currying). Do you want to see whether it would work from a syntactic point of view, in an additional CL? I'm not sure which extra ambiguities it would create, but surely the parser needs to be adjusted to allow for the curried form.
At this point all test runs are complete, with no failures.
lgtm https://codereview.chromium.org/1863053003/diff/60001/pkg/compiler/lib/src/pa... File pkg/compiler/lib/src/parser/parser.dart (right): https://codereview.chromium.org/1863053003/diff/60001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:552: !identical(endToken.next.kind, OPEN_PAREN_TOKEN)) return null; Nit: Add braces arround `return null;`
Review response 2 https://codereview.chromium.org/1863053003/diff/60001/pkg/compiler/lib/src/pa... File pkg/compiler/lib/src/parser/parser.dart (right): https://codereview.chromium.org/1863053003/diff/60001/pkg/compiler/lib/src/pa... pkg/compiler/lib/src/parser/parser.dart:552: !identical(endToken.next.kind, OPEN_PAREN_TOKEN)) return null; On 2016/04/11 12:41:00, Johnni Winther wrote: > Nit: Add braces arround `return null;` Done.
On 2016/04/11 12:08:50, eernst wrote: > On 2016/04/11 11:00:14, floitsch wrote: > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > File pkg/compiler/lib/src/parser/parser.dart (right): > > > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token > > token) { > > On 2016/04/08 17:07:10, eernst wrote: > > > On 2016/04/08 09:53:32, ahe wrote: > > > > Rename to "isValidMethodTypeArguments" to signal that a trailing > parenthesis > > > is > > > > required? > > > > > > Makes sense, done. (This deviates from the naming of parsing methods after > the > > > grammar non-terminals---but we're beyond that anyway, because of the > trailing > > > '('). > > > > > > > Can I assume that you can't "curry" type arguments as part of a tear-off > > > > operation? That is, this doesn't work (assuming m is a generic method): > > > > > > > > var f = m<a, b>; > > > > f(); > > > > > > Right, Florian did not want to support that, and Leaf seems to be relatively > > > willing to leave out the curried construct (a rather > > > short lambda can be used as a work-around). > > > > Fwiw, this CL only adds syntactic support. The semantic meaning is > independent, > > and both options are still open (although I would like to avoid currying). > > Do you want to see whether it would work from a syntactic point of view, in an > additional CL? I'm not sure which extra ambiguities it would create, but > surely the parser needs to be adjusted to allow for the curried form. My mistake. Yes: I don't want to support "bound" tear-offs. Maybe we will have to add them, but for now I would rather not add the additional complexity. I have already seen code that would benefit from it, so I might eventually change my mind, though.
Rebased to current master on github. Includes many changes (apparently, `dartfmt` was run on all libraries in the compiler since then), but tests still run.
Description was changed from ========== Adds support for --generic-method-syntax This CL adds support for parsing and ignoring declarations of type parameters on methods and functions, and passing type arguments to method and function invocations. The type parameters thus declared do not get a representation during static analysis, so any usage will cause warnings about undefined types; hence, this CL transforms usage of generic methods from syntax errors to static warnings. A followup CL will eliminate the static warnings. ========== to ========== Adds support for --generic-method-syntax This CL adds support for parsing and ignoring declarations of type parameters on methods and functions, and passing type arguments to method and function invocations. The type parameters thus declared do not get a representation during static analysis, so any usage will cause warnings about undefined types; hence, this CL transforms usage of generic methods from syntax errors to static warnings. A followup CL will eliminate the static warnings. R=johnniwinther@google.com Committed: https://github.com/dart-lang/sdk/commit/440b17901ef07ee68ec911f0f13c9bcc72857c18 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 440b17901ef07ee68ec911f0f13c9bcc72857c18 (presubmit successful).
Message was sent while issue was closed.
On 2016/04/11 13:36:16, floitsch wrote: > On 2016/04/11 12:08:50, eernst wrote: > > On 2016/04/11 11:00:14, floitsch wrote: > > > > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > > File pkg/compiler/lib/src/parser/parser.dart (right): > > > > > > > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > > pkg/compiler/lib/src/parser/parser.dart:607: bool isValidTypeArguments(Token > > > token) { > > > On 2016/04/08 17:07:10, eernst wrote: > > > > On 2016/04/08 09:53:32, ahe wrote: > > > > > Rename to "isValidMethodTypeArguments" to signal that a trailing > > parenthesis > > > > is > > > > > required? > > > > > > > > Makes sense, done. (This deviates from the naming of parsing methods after > > the > > > > grammar non-terminals---but we're beyond that anyway, because of the > > trailing > > > > '('). > > > > > > > > > Can I assume that you can't "curry" type arguments as part of a tear-off > > > > > operation? That is, this doesn't work (assuming m is a generic method): > > > > > > > > > > var f = m<a, b>; > > > > > f(); > > > > > > > > Right, Florian did not want to support that, and Leaf seems to be > relatively > > > > willing to leave out the curried construct (a rather > > > > short lambda can be used as a work-around). > > > > > > Fwiw, this CL only adds syntactic support. The semantic meaning is > > independent, > > > and both options are still open (although I would like to avoid currying). > > > > Do you want to see whether it would work from a syntactic point of view, in an > > additional CL? I'm not sure which extra ambiguities it would create, but > > surely the parser needs to be adjusted to allow for the curried form. > > My mistake. Yes: I don't want to support "bound" tear-offs. Maybe we will have > to add them, but for now I would rather not add the additional complexity. I > have already seen code that would benefit from it, so I might eventually change > my mind, though. OK, so I won't look into parsing it now.
Message was sent while issue was closed.
On 2016/04/11 15:00:12, eernst wrote: > On 2016/04/11 13:36:16, floitsch wrote: > > On 2016/04/11 12:08:50, eernst wrote: > > > On 2016/04/11 11:00:14, floitsch wrote: > > > > > > > > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > > > File pkg/compiler/lib/src/parser/parser.dart (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1863053003/diff/20001/pkg/compiler/lib/src/pa... > > > > pkg/compiler/lib/src/parser/parser.dart:607: bool > isValidTypeArguments(Token > > > > token) { > > > > On 2016/04/08 17:07:10, eernst wrote: > > > > > On 2016/04/08 09:53:32, ahe wrote: > > > > > > Rename to "isValidMethodTypeArguments" to signal that a trailing > > > parenthesis > > > > > is > > > > > > required? > > > > > > > > > > Makes sense, done. (This deviates from the naming of parsing methods > after > > > the > > > > > grammar non-terminals---but we're beyond that anyway, because of the > > > trailing > > > > > '('). > > > > > > > > > > > Can I assume that you can't "curry" type arguments as part of a > tear-off > > > > > > operation? That is, this doesn't work (assuming m is a generic > method): > > > > > > > > > > > > var f = m<a, b>; > > > > > > f(); > > > > > > > > > > Right, Florian did not want to support that, and Leaf seems to be > > relatively > > > > > willing to leave out the curried construct (a rather > > > > > short lambda can be used as a work-around). > > > > > > > > Fwiw, this CL only adds syntactic support. The semantic meaning is > > > independent, > > > > and both options are still open (although I would like to avoid currying). > > > > > > Do you want to see whether it would work from a syntactic point of view, in > an > > > additional CL? I'm not sure which extra ambiguities it would create, but > > > surely the parser needs to be adjusted to allow for the curried form. > > > > My mistake. Yes: I don't want to support "bound" tear-offs. Maybe we will have > > to add them, but for now I would rather not add the additional complexity. I > > have already seen code that would benefit from it, so I might eventually > change > > my mind, though. > > OK, so I won't look into parsing it now. I'm not strongly dug in on these, but they do allow some code to be written with less clutter. How much this matters depends a little on other choices we make. A place this sometimes comes up is when I have a function f which expects an (int -> int) as an argument, and I happen to have a (g : <T>(T) -> T) handy that I'd like to use. This isn't that uncommon, and it's a bit unpleasant to always have to write f((x) => g(x)) (assuming best case inference). If we allow inference to fill in instantiations, (or if we get a little fancy with subtyping) then we can just support f(g). At that point though, it's a little surprising that inference can generate a "bound" tear-off, but you can't do it by hand. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
