I think this comment should explain that super/this invocation is included because its arguments are restricted too.

Probably yes, I think our agreement last time is that local/anonymous classes' captures, including the "outer this", are synthetic.

this test along with EarlyAssigmentNoPreview 2 and 3 are failing. But the idea is to integrate this PR after the code for flexible constructor bodies have been merged into lworld, which should make these 3 tests pass

I understand that you wanted to simplify the visitor -- but doing a linear pass on the constructor and creating a new list of statements is also kind of expensive -- maybe when we're done with this change we can see if there's a way to set a flag on the visitor to shortcircuit the analysis after the super call is found.

I think I like the trade-off, we could try to infer if a constructor invocation corresponds to the class we are interested in. Like for example analyzing the symbol associated to a super or this invocation. But for erroneous invocations the symbol could be null. So what to do when we find a null symbol? We would have no clues I think.

let's wait until we address the other comments first. What I had in mind (but can be addressed in a separate PR) was maybe have a general visitor for prologue -- e.g. an helper visitor class that only visits things inside the prologue. Then you can extend that helper visitor here, to do what you need to do.

Is the use of j.u.List deliberate here?

yes, I think the addAll method is faster than our list

Can you provide some examples on what this method is supposed to do? E.g. example input and output?

Does this look for early references in all the symbols referenced inside the class?

Also, why do we use symbolsFor instead of recursing this this visitor?

Why doesn't this call analyze symbol?

if you have a sym, in order to understand if something is a method parameter (not argument?) don't you need to check if sym.owner == MTH ?

this is for cases when we have an argument that is for example of the same type as the current class so like:

class Test {
    String s;
    
    Test(Test t) {
        // the owner of s is Test not MTH so we need to check what is the qualifier for s which at the end is the argument
        // `t` so we ignore it
        String s1 = t.s;
        super();
    }
}

yes, so isn't just checking owner.kind != TYP enough? (e.g. "not a field")

I tried to replace this with:

if (!sym.isStatic() && sym.kind == VAR && sym.owner.kind == TYP) { ... }

And no tests seem to fail.

Do we always report an error when seeing Foo.this ? What if we're not inside the prologue of Foo ?

all the code we analyze in this visitor is in the prologue, this is why we pre-select what code we will see

I understand -- but in the prologue of Foo we can have a Bar.this where Bar is some enclosing class?

class Foo {
        class Bar {
            Bar() { Object o = Foo.this; super();}
        }
}

yep I see, there seems to be a bug here, thanks

What has fixed this exactly? It seems like this was already working as expected because TreeInfo.isExplicitThisReference already checks for possible enclosing types?

What has fixed this exactly? It seems like this was already working as expected because TreeInfo.isExplicitThisReference already checks for possible enclosing types?

yes that could be the case

Nice reuse

Can't we cut recursion here (instead of using analyzingSelect ? That's also what the new TreeInfo.symbolsFor does. In general it seems like these two visitors are trying to do similar things but are not 100% aligned?

if one has a complex select like for example: new SuperInitFails(){}.x it is still necessary to look inside and see if there are some forbidden accesses

Ok, the issue might then be that, while looking inside SuperInitFails you find a plain early access to a field (e.g. an ident) and we end up skipping it because analyzingSelect is set. At the very least we should unset inside classes (maybe lambdas too).

Example:

class Test {

    int x = 4;

    static String m(Runnable r) { return null; }

    Test() {
        m(() -> System.out.println(x)).toString();
        super();
    }

    public static void main(String[] args) {
        new Test();
    }
}

This seems to compile, but then fails with verifier error.

(e.g. we really need to make sure that analyzeSelect is not applied too broadly)

true, now that we removed the visitor at TreeInfo that is a problem

Actually -- this is enough:

class Test {

    int x = 4;

    static String m(Object r) { return null; }

    Test() {
        m(x).toString();
        super();
    }

    public static void main(String[] args) {
        new Test();
    }
}

No lambda. So probably was an issue even before, with TreeInfo.

yep :(

Is this still needed?

yes no for this patch, but I was wondering if it could be useful for future use. But I will remove it

I'm not entirely convinced about these checks. They seem to lead to very strange asymmetries:

import java.util.function.*;

class Test3 {

    int x = 4;
    int y;

    Test3() {
        System.out.println(x); //error
        Supplier<Integer> s1 = () -> x; // error
        y = 2;
        System.out.println(y); // ok
        Supplier<Integer> s2 = () -> y; // error
        super();
    }
}

I understand that references to x are invalid here -- x is not a strict field, so it will be initialized after the prologue. So the first couple of references are errors, fine.

But in the last couple, we have that print(x) is good, but the reference from the lambda is flagged as an error. I'm not sure what's the rationale here? After all the lambda is defined after y has been assigned, so what are we trying to protect against?

I think that the idea of restricting the access from lambdas and local classes is that they will capture this in order to access the field(s)

Ah! Forgot about that one -- but... with proxy locals that's no longer the case, no?

(or, do we only do local proxies for strict fields? If so, should we be more uniform here?)

we do local proxies for every field read in the prologue, a getField with a larval this will be an error in the VM

ok, so I guess I still don't get whether this must be an error. In principle y could have a local proxy, in which case the lambda could be thought of as accessing that proxy, so no need to capture this ?

I wonder what is the mental model supposed to be here.

@mcimadamore what is your opinion on whether this should compile?

class A {
    int y;
    A() {
        y = 1;
        class B {
            static void m() {  // static context
                System.out.println(y);
            }
        }
        super();
    }
}

If your answer is "No" then aren't you then implying that y shouldn't be available whenever A.this is not available? In which case doesn't that answer your question?

If your answer is "Yes", then doesn't that imply that this should also compile...

class A {
    int y;
    A() {
        y = 1;
        class B {
            static void m() {  // static context
                System.out.println(A.this.y);
            }
        }
        super();
    }
}

even though this doesn't:

class A {
    int y;
    A() {
        y = 1;
        class B {
            static void m() {  // static context
                System.out.println(A.this);
            }
        }
        super();
    }
}

I suppose what I'm saying is: I understand why the code doesn't compile in today's world. But as we relax more restrictions and we resort to more complex translation strategies, I do wonder if some of these rules that prevent reads from lambdas will feel too tight. E.g. imagine the case of a final field -- that is written only once. If we already saw a write for that field, what stops us from being able to reference it from a lambda -- through a local proxy?

I don't buy the argument that A.this.y working implies A.this. This is already not the case in the code added by this PR, where reading an already written field in a prologue is fine, even through A.this.y -- but accessing this of a class from the prologue is never ok (if it was you could pass such a larval this to another method).

(in your example with static context, my answer is that no, it should not compile. A static context can't access instance fields from an enclosing class)

(in your example with static context, my answer is that no, it should not compile. A static context can't access instance fields from an enclosing class)

Yep, sorry that was a dumb example - I should have replaced the static context with a lambda.

I'm all for increased flexibility, it's just that it would be nice if that came with a clean mental model.

For example, one possible mental model could be "works like effectively final" - which I think is what you're advocating - but for that we'd have to break the equivalence between y and A.this.y (probably worth it).

I guess that we should discuss access from a lambda with Dan, and if we decide the rules should be relaxed then do it but I think as part of a future PR. I'm also all in for relaxing restrictions

this method, isEarlyReference, is only used in Attr, I moved it here

this method, isReceiverParameter, is not used any more, I removed it

moved this one to Attr