Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1190453003:
Mixin DEP changes (Closed)
|
Created:
5 years, 6 months ago by gbracha Modified:
5 years, 6 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMixin DEP changes
BUG=
R=eernst@google.com, lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/2c7234bd610bd5a99dd501da4ee59e78a33f1839
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fix review comments by lrn and eernst #Patch Set 3 : Really fix review comments #Patch Set 4 : #Patch Set 5 : retry #Patch Set 6 : Explictly disallow mixing in unsubclassable classes #Messages
Total messages: 9 (2 generated)
lrn@google.com changed reviewers: + lrn@google.com
https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. This requires the lookup-class of a method to be remembered from the time it is looked up to the time this code executes. Even if the method is extracted. So: class C {foo()=>42;} class M extends C { foo()=>super.foo(); } class A implements C {foo()=>37;} class AM = A with M; class X extends AM { foo() => 42; get superFoo => super.foo; }; Then I do: var f = new X().superFoo; // This closure knows that it was looked up in AM (not X, not M). It's a kind of hidden data channel between lookup and execution that isn't explained anywhere else. It might be easy enough to implement, but it sounds complicated :) https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3923: Let $p_1 \ldots p_h$ be the required parameters of $f$, let $p_1 \ldots p_m$ be the positional parameters of $f$ and let $p_{h+1}, \ldots, p_{h+l}$ be the optional parameters declared by $f$. This seems to assume that $f$ is found. How about: class C {foo()=>42;} class M extends C { foo()=>super.foo(); } abstract class A implements C {} class AM = A with M; Then the superclass of AM is A, and the super.foo() call will look up "foo" in "A" and fail. Then you can't talk about the required parameters yet. (That may not be related to this change though, I think it could also be absent before? Maybe this paragraph just needs to be prefixed with "If the lookup succeeded,") https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:4096: Let $S_{static}$ be the superclass of the immediately enclosing class. It is a static type warning if $S_{static}$ does not have an accessible instance method or getter named $m$. Will this method be concrete, or can an abstract method satisfy this requirement as written?
https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:2078: Let $C$ be a class declaration that includes $M_A$ in a with clause. It is a static warning if $C$ does not implement, directly or indirectly, all the direct superinterfaces of $M$. Does this need a `@proxy` exception too? That is, "... unless C, or one of its super-interfaces, is annotated with the value of the `proxy` constant in the dart:core library."
eernst@google.com changed reviewers: + eernst@google.com
A few drive-by comments added. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:2076: Let $A$ be an application of $M_A$. It is a static warning if the superclass of $A$ is not a subtype of $S$. Maybe these two occurrences of $S$ should be changed to $S_{static}$ for consistency with the other references to the declared superclass of a class used as a mixin. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3736: If $C$ declares a concrete instance method named $m$ that is accessible to $L$, then that method is the result of the lookup, and we say that the method was {\em looked up in $C$}. Otherwise, if $C$ has a superclass $S$, then the result of the lookup is the result of looking up $m$ in $S$ with respect to $L$. Otherwise, we say that the method lookup has failed. Would it be better to find all occurrences of 'superclass $S$' and similar phrases, and change the $S$ to $S_{dynamic}$ or $S_{static}$ as appropriate, or would that information always be present in the context anyway? ("Use 'dynamic' when the topic is lookup, use 'static' when the topic is type checking"). Here, it would be $S_{dynamic}$. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. On 2015/06/19 09:36:27, Lasse Reichstein Nielsen wrote: > This requires the lookup-class of a method to be remembered from the time it is > looked up to the time this code executes. > Even if the method is extracted. > > So: > class C {foo()=>42;} > class M extends C { foo()=>super.foo(); } > class A implements C {foo()=>37;} > class AM = A with M; > class X extends AM { foo() => 42; > get superFoo => super.foo; }; > Then I do: > var f = new X().superFoo; // This closure knows that it was looked up in AM > (not X, not M). > > It's a kind of hidden data channel between lookup and execution that isn't > explained anywhere else. > It might be easy enough to implement, but it sounds complicated :) Actually, the method would be 'looked up in' the class where it is found, not the one where the lookup started. This, I think, means that it is a property of the method as such ("in which class is this method implementation declared?"), and super.foo is looked up in M. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. $S_{dynamic}$ seems to imply that the given class can be chosen at runtime; but it's a compile time constant for a non-mixin, and for a mixin, for every mixin application, the result is a class which is created at compile-time, and hence the superclass is also fixed at compile-time. Maybe it would make sense to use a terminology which allows for variability without explicitly referring to runtime; how about using a plain $S$ and add commentary mentioning that $S$ can be unknown at the location where the expression \SUPER.m(..) is located? A similar consideration would apply everywhere where $S_{dynamic}$ is introduced. (It might also be helpful for readers, because $S_{dynamic}$ occurs 20 times on a range of 13 pages, and it is not immediately obvious why the 'dynamic' tag has been added to this class if the reader isn't already thinking about mixins). https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3958: Let $S_{static}$ be the superclass of the immediately enclosing class. It is a static type warning if $S_{static}$ does not have an accessible (\ref{privacy}) instance member named $m$ unless $S_{static}$ or a superinterface of $S_{static}$ is annotated with an annotation denoting a constant identical to the constant \code{@proxy} defined in \code{dart:core}. If $S_{static}.m$ exists, it is a static type warning if the type $F$ of $S_{static}.m$ may not be assigned to a function type. If $S_{static}.m$ does not exist, or if $F$ is not a function type, the static type of $i$ is \DYNAMIC{}; otherwise the static type of $i$ is the declared return type of $F$. Looking at this again, it does make a lot of sense to talk about $S_{dynamic}$ vs. $S_{static}$ (except that 'static' is terribly ambiguous) when distinguishing method lookup vs. type checking. So maybe it's the best choice after all. ;-) https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:4210: Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up. Let $S_{dynamic}$ be the superclass of $C$. Let $S_{dynamic}$ be the superclass of the $C$. Last sentence duplicated.
Responded to comments. Please ignore intermediate states. Other fixes may go in main version, as they are not specific to this DEP. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:2076: Let $A$ be an application of $M_A$. It is a static warning if the superclass of $A$ is not a subtype of $S$. On 2015/06/23 15:45:03, eernst wrote: > Maybe these two occurrences of $S$ should be changed to $S_{static}$ for > consistency with the other references to the declared superclass of a class used > as a mixin. It doesn't matter much as there is no question of a static vs. dynamic distinction in this context, but sure. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:2078: Let $C$ be a class declaration that includes $M_A$ in a with clause. It is a static warning if $C$ does not implement, directly or indirectly, all the direct superinterfaces of $M$. On 2015/06/23 08:06:23, Lasse Reichstein Nielsen wrote: > Does this need a `@proxy` exception too? That is, "... unless C, or one of its > super-interfaces, is annotated with the value of the `proxy` constant in the > dart:core library." Short answer: No. Long answer: For whatever dark historical reasons, @proxy controls warnings given on *usage* of undefined members. Missing *declarations* are subject to other controls - they are disabled if the class is abstract or has a non-trivial (i.e., no the one from Object) noSuchMethod (which an @proxy class usually does). So the question would be whether there should be language about the concreteness of C, or whether it has noSuchMethod etc. Even then, my answer is negative. This requirement is about declaring that C implements these interfaces - not about whether it has specific methods. That is (or atelast, should be) covered elsewhere, under the blanket requirement that the expansion of the class, K, should be legal. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3736: If $C$ declares a concrete instance method named $m$ that is accessible to $L$, then that method is the result of the lookup, and we say that the method was {\em looked up in $C$}. Otherwise, if $C$ has a superclass $S$, then the result of the lookup is the result of looking up $m$ in $S$ with respect to $L$. Otherwise, we say that the method lookup has failed. On 2015/06/23 15:45:03, eernst wrote: > Would it be better to find all occurrences of 'superclass $S$' and similar > phrases, and change the $S$ to $S_{dynamic}$ or $S_{static}$ as appropriate, or > would that information always be present in the context anyway? ("Use 'dynamic' > when the topic is lookup, use 'static' when the topic is type checking"). Here, > it would be $S_{dynamic}$. Lasse made a similar comment for one specific case in the mixin text. I changed it, even though I felt it was not really necessary because it is clear from context. It does seem to get out of control if we start fretting over this everywhere. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. On 2015/06/19 09:36:27, Lasse Reichstein Nielsen wrote: > This requires the lookup-class of a method to be remembered from the time it is > looked up to the time this code executes. > Even if the method is extracted. > > So: > class C {foo()=>42;} > class M extends C { foo()=>super.foo(); } > class A implements C {foo()=>37;} > class AM = A with M; > class X extends AM { foo() => 42; > get superFoo => super.foo; }; > Then I do: > var f = new X().superFoo; // This closure knows that it was looked up in AM > (not X, not M). > > It's a kind of hidden data channel between lookup and execution that isn't > explained anywhere else. > It might be easy enough to implement, but it sounds complicated :) It's easy if the implementation copies down methods with supercalls (as in the VM, or Strongtalk). It's a bit harder if the super lookup is done dynamically (you need as special lookup) and the real problem is when you have repeated applications of the same mixin in the same superclass chain. BTW, f is not a closure in your example, but var g = new X()#superFoo is. But I don't see that it makes a difference. The key point is that the method needs to (conceptually) know what *mixin* it was defined in and what *class* it was looked up in. See the issue raised by sra for DDC on the github repo for this DEP. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. On 2015/06/23 15:45:03, eernst wrote: > $S_{dynamic}$ seems to imply that the given class can be chosen at runtime; but > it's a compile time constant for a non-mixin, and for a mixin, for every mixin > application, the result is a class which is created at compile-time, and hence > the superclass is also fixed at compile-time. > > Maybe it would make sense to use a terminology which allows for variability > without explicitly referring to runtime; how about using a plain $S$ and add > commentary mentioning that $S$ can be unknown at the location where the > expression \SUPER.m(..) is located? > > A similar consideration would apply everywhere where $S_{dynamic}$ is > introduced. (It might also be helpful for readers, because $S_{dynamic}$ occurs > 20 times on a range of 13 pages, and it is not immediately obvious why the > 'dynamic' tag has been added to this class if the reader isn't already thinking > about mixins). It's constant until you have dynamic mixin application :-) https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3923: Let $p_1 \ldots p_h$ be the required parameters of $f$, let $p_1 \ldots p_m$ be the positional parameters of $f$ and let $p_{h+1}, \ldots, p_{h+l}$ be the optional parameters declared by $f$. On 2015/06/19 09:36:27, Lasse Reichstein Nielsen wrote: > This seems to assume that $f$ is found. > How about: > class C {foo()=>42;} > class M extends C { foo()=>super.foo(); } > abstract class A implements C {} > class AM = A with M; > > Then the superclass of AM is A, and the super.foo() call will look up "foo" in > "A" and fail. Then you can't talk about the required parameters yet. > (That may not be related to this change though, I think it could also be absent > before? Maybe this paragraph just needs to be prefixed with "If the lookup > succeeded,") Yes, this assumes that other lookup failures are handled somehow. It should probably be prefixed as you say. There are probably other such cases. None of which relate to this CL in particular. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3958: Let $S_{static}$ be the superclass of the immediately enclosing class. It is a static type warning if $S_{static}$ does not have an accessible (\ref{privacy}) instance member named $m$ unless $S_{static}$ or a superinterface of $S_{static}$ is annotated with an annotation denoting a constant identical to the constant \code{@proxy} defined in \code{dart:core}. If $S_{static}.m$ exists, it is a static type warning if the type $F$ of $S_{static}.m$ may not be assigned to a function type. If $S_{static}.m$ does not exist, or if $F$ is not a function type, the static type of $i$ is \DYNAMIC{}; otherwise the static type of $i$ is the declared return type of $F$. On 2015/06/23 15:45:03, eernst wrote: > Looking at this again, it does make a lot of sense to talk about $S_{dynamic}$ > vs. $S_{static}$ (except that 'static' is terribly ambiguous) when > distinguishing method lookup vs. type checking. So maybe it's the best choice > after all. ;-) Not sure if you're agreeing with me or not. I'll assume you are, as that is always the right thing to do :-) https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:4096: Let $S_{static}$ be the superclass of the immediately enclosing class. It is a static type warning if $S_{static}$ does not have an accessible instance method or getter named $m$. On 2015/06/19 09:36:27, Lasse Reichstein Nielsen wrote: > Will this method be concrete, or can an abstract method satisfy this requirement > as written? (1) This has nothing to do with this CL; it has to do with tear-offs in general. (2) This is where we should be thinking about @proxy! (3) Abstract methods are fine (to finally answer the question). Of course, the abstract method will not be found during lookup, but for typechecking purposes we assume that a concrete method should be introduced. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:4210: Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up. Let $S_{dynamic}$ be the superclass of $C$. Let $S_{dynamic}$ be the superclass of the $C$. On 2015/06/23 15:45:03, eernst wrote: > Last sentence duplicated. It's idempotent :-). Fixed.
Aha, now I'm a reviewer, too. In that case I'll add an LGTM.
LGTM too. https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1190453003/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3922: First, the argument list $(a_1, \ldots , a_n, x_{n+1}: a_{n+1}, \ldots , x_{n+k}: a_{n+k})$ is evaluated yielding actual argument objects $o_1, \ldots , o_{n+k}$. Let $g$ be the method currently executing, and let $C$ be the class in which $g$ was looked up (\ref{methodLookup}). Let $S_{dynamic}$ be the superclass of $C$, and let $f$ be the result of looking up method (\ref{methodLookup}) $m$ in $S_{dynamic}$ with respect to the current library $L$. The result of doing super.foo in the X.superFoo getter is the closurization of AM.foo, so I'm fairly sure it's an extracted closure and it was found in AM (M is not in the superclass chain of X at all). I guess the implementation is simple - a closure contains the instance, the class that the method was found on and the method declaration. In this case that's an X object, the AM class and the M.foo method declaration. The class is used only for "super.*" lookups, the instance for "this" and the method declaration is the code.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 2c7234bd610bd5a99dd501da4ee59e78a33f1839 (presubmit successful). |
