On Thu, 12 Jan 2023 19:01:10 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> The code you quoted has nothing specifically to do with method invocations.
>> This code is simply handing the evaluation of the expressions `this` and
>> `super`. For example, `this` could just be a parameter we're passing to
>> another method.
>>
>> When `this` or `super` expressions are evaluated, the thing left on top of
>> the stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when
>> the there is a direct/indirect reference of type `ThisRef` in the current
>> reference set.
>>
>> In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top
>> of Java stack) becomes the method receiver, and when the method is "invoked"
>> it turns back into a `ThisRef` (see earlier question).
>>
>> Regarding the optimization you mention, in light of the above I'm not sure
>> it would still work.
>
> My point is about who puts ThisRef in the set to begin with. It seems to me
> that ThisRef is put there at the start of a method analysis. After which,
> there's several code points where we say "if there's a direct ThisRef in the
> set, do this, otherwise, if there's an indirect ThisRef in the set, do that".
> But the ThisRef (whether indirect or direct) seems set once and for all (when
> we start the analysis, and then inside visitApply).
>
> There is also this bit in `visitReference`:
>
>
> case SUPER:
> if (this.refs.contains(ThisRef.direct()))
> receiverRefs.add(ThisRef.direct());
> if (this.refs.contains(ThisRef.indirect()))
> receiverRefs.add(ThisRef.indirect());
> break;
>
>
> But this doesn't change what I'm saying - there seems to be a general
> property when we execute this analysis which says whether the current
> execution context has a direct `this` or not. This seems to me better
> captured with a boolean, which is then fed back to all the other various
> downstream ref creation.
>
> The main observation, at least for me, is that the code unifies everything
> under refs, when in reality there are different aspects:
>
> * the set of variables that can point to this, whether directly or indirectly
> (this is truly a set)
> * whether the current context has a direct or indirect this (this seems more
> a flag to me)
> * whether the expression on top of stack is direct/indirect this reference or
> not (again, 3 possible values here) - but granted, there `depth` here to take
> into account, so you probably end up with a set (given that we don't want to
> model a scope with its own set)
>
> When reading the code, seeing set expression like containment checks or
> removals for things that doesn't seem to be truly sets (unless I'm missing
> something) was genuinely confusing me.
I get what you're saying - it seems silly to model what is essentially a fixed,
boolean property with the membership of a singleton in a set field, rather than
with a simple boolean field.
There is a conceptual trade-off though... a lot of the code relates to
converting `Ref`'s from one type to another. For example, as pointed out above,
a method invocation might convert a `ExprRef` to a `ThisRef`, then to a
`ReturnRef`'s, etc. Having these things all be considered part of the same
family helps conceptually. The fact that a direct `ThisRef` is a singleton is
just a coincidence in this way of looking at things.
The benefit is the simplicity of being able to think of the data model as "just
a set of references".
For example, methods like `RefSet.replaceExprs()` become less elegant (or
basically impossible) if there have to be special cases for each type of
reference, whereas currently we can do clean stuff like this:
@Override
public void visitReturn(JCReturn tree) {
scan(tree.expr);
refs.replaceExprs(depth, ReturnRef::new);
}
But I'm also realizing now that several places can be cleaned up by taking this
event further. E.g., we should replace this code:
if (refs.contains(ThisRef.direct()))
outerRefs.add(OuterRef.direct());
if (refs.contains(ThisRef.indirect()))
outerRefs.add(OuterRef.indirect());
with something like this:
refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
I will go through and refactor to do that and clean things up a little more.
> When reading the code, seeing set expression like containment checks or
> removals for things that doesn't seem to be truly sets (unless I'm missing
> something) was genuinely confusing me.
Probably my fault for not providing better documentation of the overall "set of
references" conceptual approach. FWIW I added a little bit more in f83a9cf0.
-------------
PR: https://git.openjdk.org/jdk/pull/11874
