Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444843002:
Make `C()`, where `C` is a class name, a compile-time error. (Closed)
|
Created:
4 years, 1 month ago by Lasse Reichstein Nielsen Modified:
4 years, 1 month ago CC:
reviews_dartlang.org, Bob Nystrom, Leaf Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMake `C()`, where `C` is a class name, a compile-time error.
Fixers issue #27651
BUG= http://dartbug.com/27651
R=eernst@google.com, floitsch@google.com, fschneider@google.com
Committed: https://github.com/dart-lang/sdk/commit/de8e04d76a8974a5c18f34820b23b381565dec72
Patch Set 1 #
Total comments: 2
Patch Set 2 : Also prohibit calls on constructors when not a single identifier. #
Total comments: 2
Patch Set 3 : Specify existing behavior. #Patch Set 4 : Address previous comment. #
Total comments: 12
Patch Set 5 : Address comments. #Messages
Total messages: 17 (3 generated)
lrn@google.com changed reviewers: + eernst@google.com, floitsch@google.com
https://codereview.chromium.org/2444843002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2444843002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3713: If $f_{id}$ is a prefix object or a type literal, a compile-time error occurs. This case was unhandeled in the existing specification, so it fell through to `this.C()` which wasn't what any implementation actually implements. https://codereview.chromium.org/2444843002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3713: If $f_{id}$ is a prefix object or a type literal, a compile-time error occurs. We could add "or type variable", but it's not as necessary - it is handled by "a variable" below making this a function expression invocation equivalent to `T.call()` which is what is actually implemented.
LGTM. If the implementations implemented it this way anyway, go ahead and commit. If not, please talk to the teams first. Fwiw, this should be temporary until `new` is optional.
LGTM. We could consider it to be a clean-up action: It's a compile time error for now; later we can introduce `new`-less instance creation to make it OK again, but we will then know that all usages are intended (or at least fresh ;-).
It is not the current behavior - the current behavior is to treat it like `(C)()`. I want to make it a compile-time error to preserve the syntax for new-less constructor calls. Come to think of it, we treat different constructor invocations differently: - C() and prefix.C(): cannot invoke `call` on `Type` object/...call is not a function. - C.foo() and prefix.C.foo(): C does not have a static method `foo`. Both are runtime errors, neither cause a warning in dart2js. It's inconsistent. They should all act the same, and preferably they should all be compile-time errors. I'll see if I can handle the other cases too.
It is not the current behavior - the current behavior is to treat it like `(C)()`. I want to make it a compile-time error to preserve the syntax for new-less constructor calls. Come to think of it, we treat different constructor invocations differently: - C() and prefix.C(): cannot invoke `call` on `Type` object/...call is not a function. - C.foo() and prefix.C.foo(): C does not have a static method `foo`. Both are runtime errors, neither cause a warning in dart2js. It's inconsistent. They should all act the same, and preferably they should all be compile-time errors. I'll see if I can handle the other cases too.
Added clause saying prefix.C(), prefix.C.foo() and C.foo() are also compile-time errors. PTAL
On 2016/10/25 07:16:14, Lasse Reichstein Nielsen wrote: > It is not the current behavior - the current behavior is to treat it like > `(C)()`. > I want to make it a compile-time error to preserve the syntax for new-less > constructor calls. That was exactly what I meant (I should have said 'After having made this change, it's a compile-time error; later ..') > Come to think of it, we treat different constructor invocations differently: > > - C() and prefix.C(): cannot invoke `call` on `Type` object/...call is not a > function. This one is obvious: There is no other interpretation than "the developer is going to 'call' a `Type` here", so we can make this a compile-time error. > - C.foo() and prefix.C.foo(): C does not have a static method `foo`. This one is less obvious: It is a reasonable interpretation that the developer intended to call a static method here, and maybe made a spelling error. We expect this to become a compile-time error in Dart 2.0 also from the perspective where it is an invocation of a static method, so we might as well make it a compile-time error at this time. I suspect, though, that it will be more helpful for developers to tell them that there is no such static method, and then we can tell them in the future (when we have new-less instance creation) that there is no such method-or-constructor. > Both are runtime errors, neither cause a warning in dart2js. > > It's inconsistent. They should all act the same, and preferably they should all > be compile-time errors. I'll see if I can handle the other cases too. I agree that they should all be compile-time errors. Implementations may choose to use different messages in the different cases, but that shouldn't affect the spec.
On 2016/10/25 07:16:14, Lasse Reichstein Nielsen wrote: > It is not the current behavior - the current behavior is to treat it like > `(C)()`. > I want to make it a compile-time error to preserve the syntax for new-less > constructor calls. That was exactly what I meant (I should have said 'After having made this change, it's a compile-time error; later ..') > Come to think of it, we treat different constructor invocations differently: > > - C() and prefix.C(): cannot invoke `call` on `Type` object/...call is not a > function. This one is obvious: There is no other interpretation than "the developer is going to 'call' a `Type` here", so we can make this a compile-time error. > - C.foo() and prefix.C.foo(): C does not have a static method `foo`. This one is less obvious: It is a reasonable interpretation that the developer intended to call a static method here, and maybe made a spelling error. We expect this to become a compile-time error in Dart 2.0 also from the perspective where it is an invocation of a static method, so we might as well make it a compile-time error at this time. I suspect, though, that it will be more helpful for developers to tell them that there is no such static method, and then we can tell them in the future (when we have new-less instance creation) that there is no such method-or-constructor. > Both are runtime errors, neither cause a warning in dart2js. > > It's inconsistent. They should all act the same, and preferably they should all > be compile-time errors. I'll see if I can handle the other cases too. I agree that they should all be compile-time errors. Implementations may choose to use different messages in the different cases, but that shouldn't affect the spec.
LGTM again, with a question about a possible redundant wording in line 3745. https://codereview.chromium.org/2444843002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2444843002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3745: Otherwise, if $e_f$ is a property extraction expression (\ref{propertyExtraction}), then $i$ is is not a function expression invocation and is instead recognized as an ordinary method invocation (\ref{ordinaryInvocation}). It is a compile-time error if $e_f$ denotes a constructor. In which case can $e_f$ denote a constructor, which is not already covered by line 3744?
Changes since last review, PTAL. https://codereview.chromium.org/2444843002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2444843002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3745: Otherwise, if $e_f$ is a property extraction expression (\ref{propertyExtraction}), then $i$ is is not a function expression invocation and is instead recognized as an ordinary method invocation (\ref{ordinaryInvocation}). It is a compile-time error if $e_f$ denotes a constructor. Agree, it should be covered by the previous line. The syntactically possible ways to denote a constructor is: C C.name prefix.C prefix.C.name (This rules out using C<T> instead of C because type arguments are still only allowed in new-expressions) Of those, C and prefix.C are type literals (as much as "type literal" is ever defined) so all the cases are covered by line 3744. Sentence removed.
fschneider@google.com changed reviewers: + fschneider@google.com
lgtm
LGTM again, with a couple of typo-level comments. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:1241: A {\em constructor parameter list} is a parenthesized, comma-separated list of formal constructor parameters. A {\em formal constructor parameter} is either a formal parameter (\ref{formalParameters}) or an initializing formal. An {\em initializing formal} has the form \code{\THIS{}.$id$}, where $id$ is the name of an instance variable of the immediately enclosing class. It is a compile-time error if \code{id} is not an instance variable of the immediately enclosing class. It is a compile-time error if an initializing formal is used by a function other than a non-redirecting generative constructor. `\code{id}` --> `$id$` https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2382: newExpression; Funny, that line was also removed in https://codereview.chromium.org/2476613002/, which was already landed on Nov 7..? https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3157: \cd{\CLASS{} C<T> \IMPLEMENTS{} Future<C<C<T$>>>$ \ldots } For consistency, `$>>>$` --> `>>>`. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3758: If $e_f$ is a property extraction expression (\ref{propertyExtraction}), then $i$ is is not a function expression invocation and is instead recognized as an ordinary method invocation (\ref{ordinaryInvocation}) Missing period at the end. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4248: Except that iff two closurizations were created by code declared in the same class with identical bindings of \THIS{} then \cd{\SUPER$_1.m$ == \SUPER$_2.m$}. We should actually keep it at `if` rather than `iff` here, because there may be additional ways to achieve \code{\SUPER$_1$.$m$ == \SUPER$_2$.$m$}`, e.g., when it's the same closure (it is not obvious what \SUPER_1 and \SUPER_2 mean, because it should at least cover two different evaluations of the same syntactic expression `\code{\SUPER.$m$}`, as well as evaluation of two different expressions with syntax `\code{\SUPER.$m$}`). https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4414: An assignment of the form \code{\SUPER[$e_1$] = $e_2$} is equivalent to the expression \code{\SUPER.[$e_1$] = $e_2$}. The static type of the expression \code{\SUPER[$e_1$] = $e_2$} is the static type of $e_2$. Are you sure there should be a period in `\code{\SUPER.[$e_1$] = $e_2$}`?
https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:1241: A {\em constructor parameter list} is a parenthesized, comma-separated list of formal constructor parameters. A {\em formal constructor parameter} is either a formal parameter (\ref{formalParameters}) or an initializing formal. An {\em initializing formal} has the form \code{\THIS{}.$id$}, where $id$ is the name of an instance variable of the immediately enclosing class. It is a compile-time error if \code{id} is not an instance variable of the immediately enclosing class. It is a compile-time error if an initializing formal is used by a function other than a non-redirecting generative constructor. Done. Also in the next paragraph. There are more \code{id} in the spec, I'll save those for a follow-up CL. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2382: newExpression; Just goes to show that this CL is older than that, so and if you compare branches, you see all the differences between those two branches, not only the ones introduced by this CL. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3157: \cd{\CLASS{} C<T> \IMPLEMENTS{} Future<C<C<T$>>>$ \ldots } Done. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3758: If $e_f$ is a property extraction expression (\ref{propertyExtraction}), then $i$ is is not a function expression invocation and is instead recognized as an ordinary method invocation (\ref{ordinaryInvocation}) Done. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4248: Except that iff two closurizations were created by code declared in the same class with identical bindings of \THIS{} then \cd{\SUPER$_1.m$ == \SUPER$_2.m$}. That's not a change in this CL, so let's discuss this offline. https://codereview.chromium.org/2444843002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4414: An assignment of the form \code{\SUPER[$e_1$] = $e_2$} is equivalent to the expression \code{\SUPER.[$e_1$] = $e_2$}. The static type of the expression \code{\SUPER[$e_1$] = $e_2$} is the static type of $e_2$. Yes, this is Gilad's way of treating operators like methods. Maybe it should really be \code{\SUPER.[]=($e_1$, $e_2$)} to make the method calling aspect more explicit. We can discuss whether it's confusing, but it's not part of this CL.
Description was changed from ========== Make `C()`, where `C` is a class name, a compile-time error. Fixers issue #27651 BUG= http://dartbug.com/27651 ========== to ========== Make `C()`, where `C` is a class name, a compile-time error. Fixers issue #27651 BUG= http://dartbug.com/27651 R=eernst@google.com, floitsch@google.com, fschneider@google.com Committed: https://github.com/dart-lang/sdk/commit/de8e04d76a8974a5c18f34820b23b381565dec72 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as de8e04d76a8974a5c18f34820b23b381565dec72 (presubmit successful). |
