[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Committed patchset #15 (id:270001) https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group an

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Patchset 15 (id:??) landed as https://crrev.com/ddd3f318c7f3f04ceb151430bdd67cf634224aab Cr-Commit-Position: refs/heads/master@{#28032} https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this messa

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

thanks for review! https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc#newcode1191 src/scopes.cc:1191: class_var = class_var->corresponding_outer_class_variable(); On 2015/04/23 13:

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1060913005/270001 https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subsc

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

lgtm https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc#newcode1191 src/scopes.cc:1191: class_var = class_var->corresponding_outer_class_variable(); Nit: you could switch the two

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc#newcode1207 src/scopes.cc:1207: if (class_var->class_declaration_group_start() == On 2015/04/23 12:20:53, rossberg wrote: Can't c

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/23 12:38:25, marja wrote: Afaics, we need to store the initializer positions for all variables, since we can e.g., parse a lazy function and want to check its variables against the outer scopes, no? You're right, I was thinking of the declaration group start. https://codereview

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Afaics, we need to store the initializer positions for all variables, since we can e.g., parse a lazy function and want to check its variables against the outer scopes, no? (Anyway, that's being worked on in a separate CL.) https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/23 09:52:37, marja wrote: As mentioned (offline?), it's a bug (also in bleeding_edge) that the variable Kinds and initializer positions are not stored correctly in the ScopeInfo. I was wondering why it doesn't cause visible problems, and turns out it's because e.g., the initialize

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mutually-recursive-classes.js File test/mjsunit/strong/mutually-recursive-classes.js (right): https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mutually-recursive-classes.js#newcode151 test/mjsunit/

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Alright, here's a new try. As mentioned (offline?), it's a bug (also in bleeding_edge) that the variable Kinds and initializer positions are not stored correctly in the ScopeInfo. I was wondering why it doesn't cause visible problems, and turns out it's because e.g., the initializer position

Re: [v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 21 April 2015 at 16:01, 'Erik Arvidsson' via v8-dev < v8-dev@googlegroups.com> wrote: > I've been following along... and it seems like this is getting pretty > messy. How are we going to spec this? The spec isn't particularly complicated, I'll work on it. > I still want to know how we plan

Re: [v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

I've been following along... and it seems like this is getting pretty messy. How are we going to spec this? I still want to know how we plan for this to work with mutually recursive modules? On Tue, Apr 21, 2015 at 9:33 AM, wrote: > PS: Might also try to store the link to the outer var in class

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

PS: Might also try to store the link to the outer var in class scopes. (And don't we need to serialize it?) https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscrib

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

https://codereview.chromium.org/1060913005/diff/190001/src/ast.h File src/ast.h (right): https://codereview.chromium.org/1060913005/diff/190001/src/ast.h#newcode577 src/ast.h:577: int consecutive_declaration_group_start() const { Nit: also drop the "consecutive" (here and elsewhere) https://cod

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Alright, I tried to fix all the cases you pointed out. ptal. I'm adding extra stuff to Variable though and that's sad :( https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because y

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Alright, I tried to fix all the cases you pointed out. ptal. I'm adding extra stuff to Variable though and that's sad :( https://codereview.chromium.org/1060913005/diff/90001/src/ast.h File src/ast.h (right): https://codereview.chromium.org/1060913005/diff/90001/src/ast.h#newcode577 src/ast.

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/20 14:35:46, marja wrote: > > but the cycle detection logic I proposed earlier doesn't cover it. > > Why not? Meh, it does, I was just confused. B has an init-time dependency to A, so doesn't matter which type of dependency A has to B. ... it just means we need to collect the refe

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

> but the cycle detection logic I proposed earlier doesn't cover it. Why not? Meh, it does, I was just confused. B has an init-time dependency to A, so doesn't matter which type of dependency A has to B. https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegro

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/20 14:21:13, marja wrote: Oh my, this is exponentially more complicated than I thought. I would think it's actually simpler. It certainly is more regular, which means no special hacks. All we need is a generic method that, given two scopes S1 and S2, finds the binding in S1 that

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Oh my, this is exponentially more complicated than I thought. Then this should be forbidden: class A { static sm() { class C extends B { } ... } } class B { [A.sm()]() { ... } } but the cycle detection logic I proposed earlier doesn't cover it. https://codereview.chromium.org/1060913005/

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/20 13:59:46, marja wrote: I don't think this should work though: class A { m() { class C extends B {} } } class B {} since we should allow init-time references only to already declared things. No, that should definitely work. Any example of the form class A { m() { ...B.

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Yup, I'm still unsure whether the inability to find a class variable in the following cases causes any practical problems: let A = class { ... } let A = class B { ... } I don't think this should work though: class A { m() { class C extends B {} } } class B {} since we should allow init-time

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/20 12:37:30, marja wrote: omg, turns out the counterexample let A = class { m(){B} } class B {} was working (as in, we were producing an error), but it was working by accident: when looking for a class variable for m() we didn't find A. In fact we didn't find anything. Idk

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

omg, turns out the counterexample let A = class { m(){B} } class B {} was working (as in, we were producing an error), but it was working by accident: when looking for a class variable for m() we didn't find A. In fact we didn't find anything. Idk how to fix that though... it just looks li

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

On 2015/04/20 11:13:18, marja wrote: Alright, I uploaded an alternative Scope-based (instead of FunctionState-based) solution, patch set 8. I think that's cleaner.. ptal. Sounds good, should be mostly orthogonal to my comments. https://codereview.chromium.org/1060913005/ -- -- v8-dev mailin

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

There may be a kind of off-by-one-declaration bug in there, if I understand the code correctly. To ease terminology, I suggest saying "declaration group" instead of "consecutive declaration batch". That is terminology used in the context of some other languages with similar mechanisms (e.g.

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Alright, I uploaded an alternative Scope-based (instead of FunctionState-based) solution, patch set 8. I think that's cleaner.. ptal. https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this messa

[v8-dev] Re: [strong] Stricter check for referring to other classes inside methods. (issue 1060913005 by ma...@chromium.org)

Hmm, another possibility would be to put the "batch start position" thing to Scope instead of FunctionState... not sure if that would work better. https://codereview.chromium.org/1060913005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received