Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 00:57:14 GMT, Archie L. Cobbs wrote:
>> Ok - I thought false negative was the thing to absolutely avoid - and that
>> was the no. 1 concern. I think a possible approach to keep both the
>> filtering and the code more or less similar to what you have, is to save the
>> type of the expression in the ExprRef. Then, I believe that, at the end of
>> scan() you can just get whatever type is there, and check that it's the
>> correct one, if not drop it.
>
>> Ok - I thought false negative was the thing to absolutely avoid - and that
>> was the no. 1 concern.
>
> You're right. I think at the time I reasoned that it would be unusual enough
> for the type of an expression to start as an instanceof X, then change to not
> being an instanceof X, and then change back, that it was worth it to go ahead
> and filter these out. But admittedly that was based on intuition, not science.
>
>> I think a possible approach to keep both the filtering and the code more or
>> less similar to what you have, is to save the type of the expression in the
>> ExprRef. Then, I believe that, at the end of scan() you can just get
>> whatever type is there, and check that it's the correct one, if not drop it.
>
> That's a nice idea... thanks. I'll play around with it some.
I was curious how much of a difference this type filtering makes, so I built
the JDK with and without it.
The results were identical except for one case:
package java.lang.invoke;
...
public abstract sealed class CallSite permits ConstantCallSite,
MutableCallSite, VolatileCallSite {
...
CallSite(MethodType targetType, MethodHandle createTargetHook) throws
Throwable {
this(targetType); // need to initialize target to make CallSite.type()
work in createTargetHook
ConstantCallSite selfCCS = (ConstantCallSite) this;
MethodHandle boundTarget = (MethodHandle)
createTargetHook.invokeWithArguments(selfCCS);
setTargetNormal(boundTarget); // ConstantCallSite doesn't publish
CallSite.target
UNSAFE.storeStoreFence(); // barrier between target and isFrozen updates
}
When we do type filtering, `(ConstantCallSite) this` causes the 'this'
reference to be discarded so no leak is reported on the next line for
`invokeWithArguments(selfCCS)`. Just a side note, there is also a leak on the
next line because final method `setTargetNormal()` invokes
`MethodHandleNatives.setCallSiteTargetNormal(this, newTarget)`, so a leak does
get reported anyway even with type filtering.
When type filtering is disabled, we report a leak at
`invokeWithArguments(selfCCS)` - which is accurate.
So what did we learn?
First it looks like type filtering has very minimal effect. I think this is
because it requires some version of two casts in a row in and out of type
compatibility, and this is probably very rare.
But looking at the one data point we do have, the type filtering did in fact
cause a false negative.
And when building the entire JDK, it causes zero net new false positives.
So to me this is evidence that we should just remove the type filtering
altogether...
@vicente-romero-oracle your thoughts?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 16:06:04 GMT, Maurizio Cimadamore wrote: >>> Something seems to be up with the lint description for this-escape - >>> compare this: >> >> Oops, will fix - thanks. > >> The decision was to go with drawing the "warning boundary" at the >> compilation unit. The reasoning is that (a) this aligns with the compiler's >> "knowledge boundary", i.e., we can know for sure from code inspection, and >> also (b) focuses the warning on the particularly pernicious aspect of these >> bugs, which is that they arise from the non-obvious interaction among two or >> more files > > Sorry for being picky - you mention this "compilation unit" boundary before, > but this is not really the reason here. Note that in my example B constructor > calls out to a static method that is "outside" the boundary. The reason as to > why my example is not flagged is simply that "escaping" is defined as > "escaping into a subclass method", not just "escaping from the constructor > (into some other compilation unit)". Oops, you're right, I answered the wrong question so to speak. The "must involve a subclass" requirement is another dimension on which a "boundary" was declared. This was also part of the original discussion. So yes the requirement is: "requires involvement of a subclass" **AND** "that subclass lives in a separate compilation unit". - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 15:08:59 GMT, Archie L. Cobbs wrote: >>> I guess I was confused because, while subclasses are a particularly sneaky >>> case where uninitialized values can show up, the above leak seems >>> potentially dangerous as well... >> >> Yes - and this very question did come up in the discussions around this >> warning (see amber-dev). >> >> The decision was to go with drawing the "warning boundary" at the >> compilation unit. The reasoning is that (a) this aligns with the compiler's >> "knowledge boundary", i.e., we can know for sure from code inspection, and >> also (b) focuses the warning on the particularly pernicious aspect of these >> bugs, which is that they arise from the non-obvious interaction among two or >> more files - even when looking at any single one of those files, there >> doesn't seem to be any apparent problem. In other words, we decided "not to >> try to save any single source code from itself". >> >> But I think it's still an interesting question. Maybe experience will >> provide more guidance over time. > >> Something seems to be up with the lint description for this-escape - compare >> this: > > Oops, will fix - thanks. > The decision was to go with drawing the "warning boundary" at the compilation > unit. The reasoning is that (a) this aligns with the compiler's "knowledge > boundary", i.e., we can know for sure from code inspection, and also (b) > focuses the warning on the particularly pernicious aspect of these bugs, > which is that they arise from the non-obvious interaction among two or more > files Sorry for being picky - you mention this "compilation unit" boundary before, but this is not really the reason here. Note that in my example B constructor calls out to a static method that is "outside" the boundary. The reason as to why my example is not flagged is simply that "escaping" is defined as "escaping into a subclass method", not just "escaping from the constructor (into some other compilation unit)". - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 15:08:43 GMT, Archie L. Cobbs wrote: >> Something seems to be up with the lint description for this-escape - compare >> this: >> >> >> serial Warn about Serializable classes that do not have a >> serialVersionUID field. >> Also warn about other suspect declarations in >> Serializable and Externalizable classes and interfaces. >> >> with this: >> >> >> this-escape Warn when a constructor invokes a method that could >> be overriden in a subclass; >> such a method would execute before the subclass constructor completes its >> initialization. >> >> >> Indentation seems to be missing, which causes readability issues in the >> `--help-lint` output. > >> I guess I was confused because, while subclasses are a particularly sneaky >> case where uninitialized values can show up, the above leak seems >> potentially dangerous as well... > > Yes - and this very question did come up in the discussions around this > warning (see amber-dev). > > The decision was to go with drawing the "warning boundary" at the compilation > unit. The reasoning is that (a) this aligns with the compiler's "knowledge > boundary", i.e., we can know for sure from code inspection, and also (b) > focuses the warning on the particularly pernicious aspect of these bugs, > which is that they arise from the non-obvious interaction among two or more > files - even when looking at any single one of those files, there doesn't > seem to be any apparent problem. In other words, we decided "not to try to > save any single source code from itself". > > But I think it's still an interesting question. Maybe experience will provide > more guidance over time. > Something seems to be up with the lint description for this-escape - compare > this: Oops, will fix - thanks. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 11:08:33 GMT, Maurizio Cimadamore wrote: >> Perhaps my confusion might come from the name `this-escape` of the lint >> warning - which seems overpromising in this respect. But I looked at the >> description of the lint warning using `javac --help-lint` and I got this: >> >> >> this-escape Warn when a constructor invokes a method that could >> be overriden in a subclass; >> >> >> Which indeed aligns well with what this PR is doing. So that's ok. > > Something seems to be up with the lint description for this-escape - compare > this: > > > serial Warn about Serializable classes that do not have a > serialVersionUID field. > Also warn about other suspect declarations in > Serializable and Externalizable classes and interfaces. > > with this: > > > this-escape Warn when a constructor invokes a method that could be > overriden in a subclass; > such a method would execute before the subclass constructor completes its > initialization. > > > Indentation seems to be missing, which causes readability issues in the > `--help-lint` output. > I guess I was confused because, while subclasses are a particularly sneaky > case where uninitialized values can show up, the above leak seems potentially > dangerous as well... Yes - and this very question did come up in the discussions around this warning (see amber-dev). The decision was to go with drawing the "warning boundary" at the compilation unit. The reasoning is that (a) this aligns with the compiler's "knowledge boundary", i.e., we can know for sure from code inspection, and also (b) focuses the warning on the particularly pernicious aspect of these bugs, which is that they arise from the non-obvious interaction among two or more files - even when looking at any single one of those files, there doesn't seem to be any apparent problem. In other words, we decided "not to try to save any single source code from itself". But I think it's still an interesting question. Maybe experience will provide more guidance over time. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 10:58:33 GMT, Maurizio Cimadamore
wrote:
>> Caring about the proper initialization of any class in the _current_
>> compilation unit is an explicit non-goal.
>>
>> We only care about bugs where a superclass and subclass are in separate
>> compilation units.
>
> So, to clarify, in this case:
>
>
> import java.util.*;
>
> class B {
> final Object ref;
>
> private B(Object ref) {
> Foo.consume(this);
> this.ref = ref;
> }
> }
>
>
> Even though `this` leaks to a method clearly before it is fully initialized,
> we do not care because there can be no subclass involved observing this. I
> guess I was confused because, while subclasses are a particularly sneaky case
> where uninitialized values can show up, the above leak seems potentially
> dangerous as well - we're effectively leaking a class whose final field has
> not been initialized!
>
> That said, if that was discussed, and it was decided for the warning not to
> deal with this case, that's ok.
Perhaps my confusion might come from the name `this-escape` of the lint warning
- which seems overpromising in this respect. But I looked at the description of
the lint warning using `javac --help-lint` and I got this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
Which indeed aligns well with what this PR is doing. So that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 11:05:51 GMT, Maurizio Cimadamore
wrote:
>> So, to clarify, in this case:
>>
>>
>> import java.util.*;
>>
>> class B {
>> final Object ref;
>>
>> private B(Object ref) {
>> Foo.consume(this);
>> this.ref = ref;
>> }
>> }
>>
>>
>> Even though `this` leaks to a method clearly before it is fully initialized,
>> we do not care because there can be no subclass involved observing this. I
>> guess I was confused because, while subclasses are a particularly sneaky
>> case where uninitialized values can show up, the above leak seems
>> potentially dangerous as well - we're effectively leaking a class whose
>> final field has not been initialized!
>>
>> That said, if that was discussed, and it was decided for the warning not to
>> deal with this case, that's ok.
>
> Perhaps my confusion might come from the name `this-escape` of the lint
> warning - which seems overpromising in this respect. But I looked at the
> description of the lint warning using `javac --help-lint` and I got this:
>
>
> this-escape Warn when a constructor invokes a method that could
> be overriden in a subclass;
>
>
> Which indeed aligns well with what this PR is doing. So that's ok.
Something seems to be up with the lint description for this-escape - compare
this:
serial Warn about Serializable classes that do not have a
serialVersionUID field.
Also warn about other suspect declarations in
Serializable and Externalizable classes and interfaces.
with this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
such a method would execute before the subclass constructor completes its
initialization.
Indentation seems to be missing, which causes readability issues in the
`--help-lint` output.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:04:09 GMT, Archie L. Cobbs wrote:
>> but what if `m` is a static method in a separate compilation unit? Should it
>> be able to observe a partially initialized Foo?
>
> Caring about the proper initialization of any class in the _current_
> compilation unit is an explicit non-goal.
>
> We only care about bugs where a superclass and subclass are in separate
> compilation units.
So, to clarify, in this case:
import java.util.*;
class B {
final Object ref;
private B(Object ref) {
Foo.consume(this);
this.ref = ref;
}
}
Even though `this` leaks to a method clearly before it is fully initialized, we
do not care because there can be no subclass involved observing this. I guess I
was confused because, while subclasses are a particularly sneaky case where
uninitialized values can show up, the above leak seems potentially dangerous as
well - we're effectively leaking a class whose final field has not been
initialized!
That said, if that was discussed, and it was decided for the warning not to
deal with this case, that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:47:28 GMT, Maurizio Cimadamore wrote: > Ok - I thought false negative was the thing to absolutely avoid - and that > was the no. 1 concern. You're right. I think at the time I reasoned that it would be unusual enough for the type of an expression to start as an instanceof X, then change to not being an instanceof X, and then change back, that it was worth it to go ahead and filter these out. But admittedly that was based on intuition, not science. > I think a possible approach to keep both the filtering and the code more or > less similar to what you have, is to save the type of the expression in the > ExprRef. Then, I believe that, at the end of scan() you can just get whatever > type is there, and check that it's the correct one, if not drop it. That's a nice idea... thanks. I'll play around with it some. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:12:27 GMT, Archie L. Cobbs wrote:
>> Uhm. Turns out I probably did not understand the filter correctly, and now
>> I'm more dubious about what it actually does. Say you have this hierarchy:
>>
>>
>> interface A { }
>> class B {
>> B() {
>> A a = (A)this;
>> ...
>> }
>> }
>> class C extends B implements A { }
>> ```
>>
>> Pathological case, I know. But the filtering will end up dropping the
>> expression Ref on the floor, right? (because B and A are unrelated).
>
>> But the filtering will end up dropping the expression Ref on the floor,
>> right? (because B and A are unrelated).
>
> Ah, I see what you mean.
>
> Here's a more complete example:
>
> public class CastLeak {
>
> public CastLeak() {
> ((CastLeak)(Runnable)this).mightLeak();
> }
>
> public void mightLeak() {
> }
> }
>
> That would be a leak for any subclass that implements `Runnable`. Yet no
> warning is generated.
>
> So the filtering by expression type is going to potentially create false
> negatives. But it also eliminates a bunch of false positives. And the false
> negatives are probably all somewhat pathological like the example above.
>
> So I still think it's worth doing.
Ok - I thought false negative was the thing to absolutely avoid - and that was
the no. 1 concern. I think a possible approach to keep both the filtering and
the code more or less similar to what you have, is to save the type of the
expression in the ExprRef. Then, I believe that, at the end of scan() you can
just get whatever type is there, and check that it's the correct one, if not
drop it.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:28:12 GMT, Archie L. Cobbs wrote:
>> 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.
> ```java
> ```java
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
> ```
>
>
>
>
>
>
>
>
>
>
>
> with something like this:
> ```java
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> ```
> ```
This sounds like a good idea, that idiom is quite widespread, so it should help
avoiding repetition.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:01:10 GMT, Maurizio Cimadamore
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
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:24:50 GMT, Archie L. Cobbs wrote:
>> This patch passes all tests:
>>
>>
>> diff --git
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>>
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> index 9d35c2fbc0a..e755c54d0c8 100644
>> ---
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> +++
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> @@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // If the expression type is incompatible with 'this', discard
>> it
>> -if (type != null && !this.isSubtype(this.targetClass.sym.type,
>> type))
>> +if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
>> types))
>> this.refs.remove(ExprRef.direct(this.depth));
>> }
>> }
>> @@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> if (explicitOuterThis != null) {
>> this.scan(explicitOuterThis);
>> this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
>> OuterRef(direct)));
>> -} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
>> +} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
>> if (this.refs.contains(ThisRef.direct()))
>> outerRefs.add(OuterRef.direct());
>> if (this.refs.contains(ThisRef.indirect()))
>> @@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> // Explicit outer 'this' reference?
>> final Type selectedType = this.types.erasure(tree.selected.type);
>> if (selectedType.hasTag(CLASS)) {
>> -final Type.ClassType selectedClassType =
>> (Type.ClassType)selectedType;
>> if (tree.name == this.names._this &&
>> -this.types.hasOuterClass(currentClassType,
>> selectedClassType)) {
>> +
>> currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> if (this.refs.contains(OuterRef.indirect()))
>> @@ -895,9 +894,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> final MethodSymbol sym = (MethodSymbol)tree.sym;
>>
>> // Check for implicit 'this' reference
>> -final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> -final Type methodOwnerType = sym.owner.type;
>> -if (this.isSubtype(currentClassType, methodOwnerType)) {
>> +if (methodClass.sym.isSubClass(sym.owner, types)) {
>> if (this.refs.contains(ThisRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> if (this.refs.contains(ThisRef.indirect()))
>> @@ -906,7 +903,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // Check for implicit outer 'this' reference
>> -if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>> +if (methodClass.sym.isEnclosedBy((ClassSymbol)sym.owner)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>>
>>
>> Btw, I believe a similar trick can be used in
>> TreeInfo.isExplicitThisReference. If I'm correct, `hasOuterClass` should
>> probably be removed as it duplicates already existing functionality.
>>
>> Since I'm bringing this up, as TreeInfo.isExplicitThisReference is only used
>> by the new analyzer, it would be cleaner if it was defined there, at least
>> until it's clear it might be needed by some other client.
>
> Weird. I don't get that build failure.
>
> Neither does github... I have been relying on the github build "Actions"
> succeeding to determine if "the build works" and according to that it is
> building.
>
> I will add the `DISABLED_WARNINGS` in any case.
Thanks for the patch!
The semantics of `hasOuterClass()` returns false if A and B are the same class,
while `isEnclosedBy()` returns true if A and B are the same class.
However, I don't think it would actually matter in practice...
Regardless, I'll add the extra equality comparison and apply your patch and
also the suggestions to integrate `isExplicitThisReference()` and eliminate
`hasOuterClass()`.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:48:25 GMT, Maurizio Cimadamore wrote: >>> I guess what I'm thinking about: >> >> No leak is possible in that example. >> * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`) >> therefore `m()` is not overridden >> * Any subclass of `Foo` that may exist in the outside world cannot use the >> `Foo()` constructor that leaks because it's private > > but what if `m` is a static method in a separate compilation unit? Should it > be able to observe a partially initialized Foo? Caring about the proper initialization of any class in the _current_ compilation unit is an explicit non-goal. We only care about bugs where a superclass and subclass are in separate compilation units. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:40:38 GMT, Maurizio Cimadamore
wrote:
>> This patch:
>>
>>
>> diff --git a/make/test/BuildMicrobenchmark.gmk
>> b/make/test/BuildMicrobenchmark.gmk
>> index 1c89328a388..7c3f0293edc 100644
>> --- a/make/test/BuildMicrobenchmark.gmk
>> +++ b/make/test/BuildMicrobenchmark.gmk
>> @@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
>> TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
>> SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
>> INCLUDE_FILES := indify/Indify.java, \
>> -DISABLED_WARNINGS := rawtypes serial options, \
>> +DISABLED_WARNINGS := this-escape rawtypes serial options, \
>> BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
>> JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
>> ))
>> @@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation,
>> BUILD_JDK_MICROBENCHMARK, \
>> TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
>> SMALL_JAVA := false, \
>> CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
>> -DISABLED_WARNINGS := processing rawtypes cast serial preview, \
>> +DISABLED_WARNINGS := this-escape processing rawtypes cast serial
>> preview, \
>> SRC := $(MICROBENCHMARK_SRC), \
>> BIN := $(MICROBENCHMARK_CLASSES), \
>> JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
>> diff --git
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>>
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> index 9d35c2fbc0a..4e2b1e558e7 100644
>> ---
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> +++
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> @@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> // Check for implicit 'this' reference
>> final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> final Type methodOwnerType = sym.owner.type;
>> +//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
>> if (this.isSubtype(currentClassType, methodOwnerType)) {
>> if (this.refs.contains(ThisRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> @@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // Check for implicit outer 'this' reference
>> +// if
>> (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner)) {
>> if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>>
>>
>> Fixes the build failure.
>
> This patch passes all tests:
>
>
> diff --git
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> index 9d35c2fbc0a..e755c54d0c8 100644
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> @@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> }
>
> // If the expression type is incompatible with 'this', discard it
> -if (type != null && !this.isSubtype(this.targetClass.sym.type,
> type))
> +if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
> types))
> this.refs.remove(ExprRef.direct(this.depth));
> }
> }
> @@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> if (explicitOuterThis != null) {
> this.scan(explicitOuterThis);
> this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
> OuterRef(direct)));
> -} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
> +} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
> if (this.refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (this.refs.contains(ThisRef.indirect()))
> @@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
> // Explicit outer 'this' reference?
> final Type selectedType = this.types.erasure(tree.selected.type);
> if (selectedType.hasTag(CLASS)) {
> -final Type.ClassType selectedClassType =
> (Type.ClassType)selectedType;
> if (tree.name == this.names._this &&
> -this.types.hasOuterClass(currentClassType,
> selectedClassType)) {
> +
> currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
> if (this.refs.contains(OuterRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
> if (this.refs.conta
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:40:36 GMT, Maurizio Cimadamore
wrote:
> But the filtering will end up dropping the expression Ref on the floor,
> right? (because B and A are unrelated).
Ah, I see what you mean.
Here's a more complete example:
public class CastLeak {
public CastLeak() {
((CastLeak)(Runnable)this).mightLeak();
}
public void mightLeak() {
}
}
That would be a leak for any subclass that implements `Runnable`. Yet no
warning is generated.
So the filtering by expression type is going to potentially create false
negatives. But it also eliminates a bunch of false positives. And the false
negatives are probably all somewhat pathological like the example above.
So I still think it's worth doing.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:33:48 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 875:
>>
>>> 873: // Reference to this?
>>> 874: if (tree.name == names._this || tree.name == names._super) {
>>> 875: if (this.refs.contains(ThisRef.direct()))
>>
>> This idiom occurs quite a lot. If I'm correct, this basically amounts at
>> asking as to whether the receiver of the method we're currently evaluating
>> is direct or not (which is an invariant, given a method body - e.g. for a
>> given method this "fact" should stay the same). If that's the case, perhaps
>> capturing this in a flag could be better - then you could have just have a
>> single method e.g. `XYZRef.create(boolean direct)`, and remove the branching
>> (here and elsewhere).
>
> 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.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:29:22 GMT, Maurizio Cimadamore
wrote:
>> I put it there because of switch expressions and `yeild`... ?
>
> Well, yield can... yield a value - `case` doesn't. So I'm confused. Also
> because the variable is called `referenceExpressionNode` and `CASE` is not an
> expression. Can `CASE` leave anything on the stack? YIELD does, but CASE?
It's just an artifact of the way switch expressions are handled:
@Override
public void visitSwitchExpression(JCSwitchExpression tree) {
visitScoped(tree, true, t -> {
scan(t.selector);
refs.discardExprs(depth);
RefSet combinedRefs = new RefSet<>();
for (List cases = t.cases; cases.nonEmpty(); cases =
cases.tail) {
scan(cases.head);
combinedRefs.addAll(refs.removeExprs(depth));
}
refs.addAll(combinedRefs);
});
}
@Override
public void visitCase(JCCase tree) {
scan(tree.stats); // no need to scan labels
}
After scanning a switch expression case, the `yield`'ed value will be on the
top of the stack.
Then `visitCase` does NOT remove it, so that it can be picked up and removed by
`visitSwitchExpression()`.
But this can be cleaned up a little bit, by having `visitSwitchExpression()`
not delegate to `visitCase()` but rather iterate through the case statements
itself directly. Then we can remove the `CASE` as you suggest.
Also I realized we need to handle `yield`'ed values like `return` values, i.e.,
collect and remember them until the entire case is complete.
I'll fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:37:06 GMT, Archie L. Cobbs wrote:
>> I guess what I'm thinking about:
>>
>> class Foo {
>> private Foo() {
>> m(this);
>> }
>>
>> public void m() { ... } // overridable
>>
>> static Foo makeFoo() { return new Foo(); }
>> }
>
>> I guess what I'm thinking about:
>
> No leak is possible in that example.
> * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`)
> therefore `m()` is not overridden
> * Any subclass of `Foo` that may exist in the outside world cannot use the
> `Foo()` constructor that leaks because it's private
but what if `m` is a static method in a separate compilation unit? Should it be
able to observe a partially initialized Foo?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:18:38 GMT, Maurizio Cimadamore
wrote:
>> I can't seem to be able to run tests - I get failures in the build:
>>
>>
>> * For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
>
> This patch:
>
>
> diff --git a/make/test/BuildMicrobenchmark.gmk
> b/make/test/BuildMicrobenchmark.gmk
> index 1c89328a388..7c3f0293edc 100644
> --- a/make/test/BuildMicrobenchmark.gmk
> +++ b/make/test/BuildMicrobenchmark.gmk
> @@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
> TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
> SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
> INCLUDE_FILES := indify/Indify.java, \
> -DISABLED_WARNINGS := rawtypes serial options, \
> +DISABLED_WARNINGS := this-escape rawtypes serial options, \
> BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
> JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
> ))
> @@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation,
> BUILD_JDK_MICROBENCHMARK, \
> TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
> SMALL_JAVA := false, \
> CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
> -DISABLED_WARNINGS := processing rawtypes cast serial preview, \
> +DISABLED_WARNINGS := this-escape processing rawtypes cast serial
> preview, \
> SRC := $(MICROBENCHMARK_SRC), \
> BIN := $(MICROBENCHMARK_CLASSES), \
> JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
> diff --git
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> index 9d35c2fbc0a..4e2b1e558e7 100644
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> @@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> // Check for implicit 'this' reference
> final Type.ClassType currentClassType =
> (Type.ClassType)this.methodClass.sym.type;
> final Type methodOwnerType = sym.owner.type;
> +//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
> if (this.isSubtype(currentClassType, methodOwnerType)) {
> if (this.refs.contains(ThisRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
> @@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> }
>
> // Check for implicit outer 'this' reference
> +// if
> (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner)) {
> if (this.types.hasOuterClass(currentClassType, methodOwnerType))
> {
> if (this.refs.contains(OuterRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
>
>
> Fixes the build failure.
This patch passes all tests:
diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index 9d35c2fbc0a..e755c54d0c8 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
// If the expression type is incompatible with 'this', discard it
-if (type != null && !this.isSubtype(this.targetClass.sym.type,
type))
+if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
types))
this.refs.remove(ExprRef.direct(this.depth));
}
}
@@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
if (explicitOuterThis != null) {
this.scan(explicitOuterThis);
this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
OuterRef(direct)));
-} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
+} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
if (this.refs.contains(ThisRef.direct()))
outerRefs.add(OuterRef.direct());
if (this.refs.contains(ThisRef.indirect()))
@@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Explicit outer 'this' reference?
final Type selectedType = this.types.erasure(tree.selected.type);
if (selectedType.hasTag(CLASS)) {
-final Type.ClassType selectedClassType =
(Type.ClassType)selectedType;
if (tree.name == this.names._this &&
-this.types.hasOuterClass(currentClassType, selectedClassType))
{
+
currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
if (this.refs.contains(OuterRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
if (this.refs.cont
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 16:40:33 GMT, Maurizio Cimadamore wrote: > I guess what I'm thinking about: No leak is possible in that example. * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`) therefore `m()` is not overridden * Any subclass of `Foo` that may exist in the outside world cannot use the `Foo()` constructor that leaks because it's private - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:11:01 GMT, Maurizio Cimadamore
wrote:
>> Same comment as previous: I don't quite know what I'm doing and I'm loathe
>> to break what is already working. Do you have a suggested patch?
>
> I can't seem to be able to run tests - I get failures in the build:
>
>
> * For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
This patch:
diff --git a/make/test/BuildMicrobenchmark.gmk
b/make/test/BuildMicrobenchmark.gmk
index 1c89328a388..7c3f0293edc 100644
--- a/make/test/BuildMicrobenchmark.gmk
+++ b/make/test/BuildMicrobenchmark.gmk
@@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
INCLUDE_FILES := indify/Indify.java, \
-DISABLED_WARNINGS := rawtypes serial options, \
+DISABLED_WARNINGS := this-escape rawtypes serial options, \
BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
))
@@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation, BUILD_JDK_MICROBENCHMARK,
\
TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
SMALL_JAVA := false, \
CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
-DISABLED_WARNINGS := processing rawtypes cast serial preview, \
+DISABLED_WARNINGS := this-escape processing rawtypes cast serial preview, \
SRC := $(MICROBENCHMARK_SRC), \
BIN := $(MICROBENCHMARK_CLASSES), \
JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index 9d35c2fbc0a..4e2b1e558e7 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Check for implicit 'this' reference
final Type.ClassType currentClassType =
(Type.ClassType)this.methodClass.sym.type;
final Type methodOwnerType = sym.owner.type;
+//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
if (this.isSubtype(currentClassType, methodOwnerType)) {
if (this.refs.contains(ThisRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
@@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
// Check for implicit outer 'this' reference
+// if (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner))
{
if (this.types.hasOuterClass(currentClassType, methodOwnerType)) {
if (this.refs.contains(OuterRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
Fixes the build failure.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:28:12 GMT, Maurizio Cimadamore wrote: > This might also be related with the fact that we deal with return values in > different ways than with e.g. values returned from a nested scope (where we > just pop, and then copy all pending expression to the outer depth). Yes, a method return value that represents a reference is an `ExprRef` just before the `return` is actually executed, but then it turns into a `ReturnRef`. A `ReturnRef` is really just a yellow sticky note that reminds us "Hey whenever you finish 'executing' this method, remember that there is a reference in its return value". > This might also be related with the fact that we deal with return values in > different ways than with e.g. values returned from a nested scope (where we > just pop, and then copy all pending expression to the outer depth). Exactly. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:48:37 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 909:
>>
>>> 907:
>>> 908: // Check for implicit outer 'this' reference
>>> 909: if (this.types.hasOuterClass(currentClassType,
>>> methodOwnerType)) {
>>
>> Similarly here - look for `Symbol.isEnclosedBy`
>
> Same comment as previous: I don't quite know what I'm doing and I'm loathe to
> break what is already working. Do you have a suggested patch?
I can't seem to be able to run tests - I get failures in the build:
* For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:39:05 GMT, Vicente Romero wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 200:
>
>> 198: //
>> 199:
>> 200: public void analyzeTree(Env env) {
>
> nit: this method could be removed in favor of the overloaded version below
Thanks - will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 204:
>
>> 202: }
>> 203:
>> 204: public void analyzeTree(Env env, JCTree tree) {
>
> nit: `env` parameter doesn't seem to be used could be dropped I think
Thanks - will fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:26:27 GMT, Maurizio Cimadamore wrote: > Do we really need a set for this? There are surely other ways to model things. But I got myself really confused trying to build more complicated models. What I ended up with is this simple model that works: * There is a set of `Ref` subclasses that model the various types of 'this' references possible: `OuterRef`, `ExprRef`, etc. * There is a singleton `this.refs`, which just is a `Set`, containing the 'this' references that are alive at the current moment * As we "execute" code, all we need to do is update the `this.refs` based on what the current bit of code does E.g When a variable is assigned a value that has a reference, we add a `VarRef` for that variable. When we leave a scope, we remove any `Ref`'s that are no longer in scope. Before executing a method, we add `Ref`'s for the method receiver and parameter. When we return from a non-void method, we convert any `ReturnRef` into a `ExprRef`. Etc. THAT I can understand. I don't see converting the above into a bitmap or whatever as worth the additional complexity. We're not programming in perl here. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:17:32 GMT, Maurizio Cimadamore wrote: > There is a concept of push/popScope and then there's a separate concept of > call stack (which is just a list of diagnostic position up to the point). I > wonder if this could be better modeled by using a single class e.g. > Scope/Frame which has a diagnostic position, plus other useful things. I think that would be more confusing, because they're really two different animals. Scopes only have meaning within a single method. They simply serve to bracket the lifetimes of `VarRef` references, following Java curly brace scope. When you "invoke" a method, you put aside the current stack of scopes and start over with an empty scope stack. They don't bridge between methods. Call stacks are just a list of method+code position therein, and they exist outside of any single method. They have nothing to do with Java scopes defined by curly braces. > Perhaps it might even be helpful to have a ref set on each scope, so that you > don't have to attach a "depth" to each ref - the depth of the ref would be > determined by the "scope" in which it appears. That's another way to skin the same cat... in fact I had a design like that before but then realized it was a lot simpler to just carry around one `RefSet`. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:15:17 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 175:
>
>> 173: private DiagnosticPosition[] pendingWarning;
>> 174:
>> 175: // These fields are scoped to the CONSTRUCTOR OR INVOKED METHOD BEING
>> ANALYZED
>
> Watch out for "all caps"
Will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 900:
>
>> 898: final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> 899: final Type methodOwnerType = sym.owner.type;
>> 900: if (this.isSubtype(currentClassType, methodOwnerType)) {
>
> I believe what you need here is not subtyping but subclassing - see
> `Symbol.isSubclass`
Hmm, I tried using `Symbol.isSubclass()` but it caused test failures. Obviously
I don't quite know what I'm doing and I'm loathe to break what is already
working. Do you have a suggested patch?
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 909:
>
>> 907:
>> 908: // Check for implicit outer 'this' reference
>> 909: if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>
> Similarly here - look for `Symbol.isEnclosedBy`
Same comment as previous: I don't quite know what I'm doing and I'm loathe to
break what is already working. Do you have a suggested patch?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 11:09:35 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 444:
>
>> 442: if (referenceExpressionNode) {
>> 443:
>> 444: // We treat instance methods as having a "value" equal to
>> their instance
>
> The comment is slightly misleading - e.g. I'd suggest clarifying "having a
> "value" whose type is the same as that of the class in which the method is
> defined"
Agreed - will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 454:
>
>> 452:
>> 453: // If the expression type is incompatible with 'this',
>> discard it
>> 454: if (type != null &&
>> !this.isSubtype(this.targetClass.sym.type, type))
>
> Instead of adding the direct reference, and then having to check if the
> reference needs to be removed, would it be possible not to add the reference
> in the first place if the types mismatch?
No because (for example) what if you cast?
The thing you're casting might be compatible, but after the cast it might
become incompatible.
> This will then determine how to interpret this in the context of that method
> analysis - e.g. when we see a JCIdent for this, we create a direct/indirect
> ExprRef based on what the receiver kind was. Correct?
Yes, exactly.
When we "invoke" a method, we "preload" the set of current references that the
method is going to have when it starts. That "preload" step includes any
references from (a) the method receiver (non-static methods only) and (b)
method parameters.
To the extent the method receiver has a direct/indirect reference, that turns
into a direct/indirect `ThisRef` during method execution. You can see this
happen in the very next lines after what you quoted. But of course the method
invocation (the "apply") could have been applied to any arbitrary expression as
the receiver.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 817:
>
>> 815: // Methods - the "value" of a non-static method is a reference
>> to its instance
>> 816: final Symbol sym = tree.sym;
>> 817: if (sym.kind == MTH) {
>
> This is perhaps where filtering based on the declaring class could make sense
> (to avoid having to filter later) ? Perhaps this could also be centralized -
> e.g. whenever you create an ExprRef you also pass the type for it, and if the
> type matches that for the current class you create it and add to the list,
> otherwise you skip it.
Yes but see previous comment regarding casting.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 875:
>
>> 873: // Reference to this?
>> 874: if (tree.name == names._this || tree.name == names._super) {
>> 875: if (this.refs.contains(ThisRef.direct()))
>
> This idiom occurs quite a lot. If I'm correct, this basically amounts at
> asking as to whether the receiver of the method we're currently evaluating is
> direct or not (which is an invariant, given a method body - e.g. for a given
> method this "fact" should stay the same). If that's the case, perhaps
> capturing this in a flag could be better - then you could have just have a
> single method e.g. `XYZRef.create(boolean direct)`, and remove the branching
> (here and elsewhere).
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.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:13:55 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 411:
>>
>>> 409: final boolean referenceExpressionNode;
>>> 410: switch (tree.getTag()) {
>>> 411: case CASE:
>>
>> surprised to see `CASE` here - as that's not an expression
>
> I put it there because of switch expressions and `yeild`... ?
Well, yield can... yield a value - `case` doesn't. So I'm confused. Also
because the variable is called `referenceExpressionNode` and `CASE` is not an
expression. Can `CASE` leave anything on the stack? YIELD does, but CASE?
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 454:
>>
>>> 452:
>>> 453: // If the expression type is incompatible with 'this',
>>> discard it
>>> 454: if (type != null &&
>>> !this.isSubtype(this.targetClass.sym.type, type))
>>
>> Instead of adding the direct reference, and then having to check if the
>> reference needs to be removed, would it be possible not to add the reference
>> in the first place if the types mismatch?
>
> No because (for example) what if you cast?
>
> The thing you're casting might be compatible, but after the cast it might
> become incompatible.
Uhm. Turns out I probably did not understand the filter correctly, and now I'm
more dubious about what it actually does. Say you have this hierarchy:
interface A { }
class B {
B() {
A a = (A)this;
...
}
}
class C extends B implements A { }
```
Pathological case, I know. But the filtering will end up dropping the
expression Ref on the floor, right? (because B and A are unrelated).
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 200:
> 198: //
> 199:
> 200: public void analyzeTree(Env env) {
nit: this method could be removed in favor of the overloaded version below
src/jdk.compiler/share/clas
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:56:53 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 411:
>
>> 409: final boolean referenceExpressionNode;
>> 410: switch (tree.getTag()) {
>> 411: case CASE:
>
> surprised to see `CASE` here - as that's not an expression
I put it there because of switch expressions and `yeild`... ?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:48:49 GMT, Maurizio Cimadamore wrote: >> Archie L. Cobbs has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Use the more appropriate Type comparison method Types.isSameType(). >> - Add some more comments to clarify how the analysis works. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java > line 227: > >> 225: final boolean privateOuterPrev = this.privateOuter; >> 226: final Lint lintPrev = this.lint; >> 227: this.lint = this.lint.augment(tree.sym); > > general stylistic comment - I see here and everywhere in this class > `this.xyz` - this is not the norm in the rest of the javac codebase, and it > would be better to replace it with just `xyz` OK. I will reluctantly remove... `` > Why on earth wouldn't you want to make it clear which one of N outer classes > a field comes from, and that it's a field, and not a variable declared > somewhere off screen?? > > IMHO omitting 'this' qualifiers is effectively accepting the grave offense of > code obfuscation in exchange for a tiny smidgen of brevity. > > It's definitely made it harder for me to read and understand the compiler, > with all the levels of nesting it has. > `` I readily admit I'm probably in the minority on this and anyway it's a bikeshed thing so there's no point in debating it. I will go with the flow :) though I feel a little sorry for the next person who has to read this code. > From a documentation perspective it might carry a bit of value Yes, that's the purpose - the `final` is for the human viewer, not the compiler. Just trying to be helpful to the next person. But you're right it's inconsistent so I'll remove. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 16:20:12 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 218:
>>
>>> 216: new TreeScanner() {
>>> 217:
>>> 218: private Lint lint = ThisEscapeAnalyzer.this.lint;
>>
>> On a first look I'm not sure about the granularity of suppression here. I
>> believe that suppressing at the class level, or at the constructor level is
>> enough. Allowing to further annotate var declarations and non-constructor
>> methods, while doable, might actually be counter productive - in the sense
>> that the annotation does not occur in the constructor (where you'd want to
>> see it) but in some other method. I think the fact that a constructor is
>> escaping (willingly) should be a visible thing. Something to consider.
>
> Two things...
>
> We need to support annotations on field declarations because their
> initializers are effectively mini-constructors. But we don't need it on local
> variables, parameters, etc. - too confusing.
>
> For annotations on methods, yes it's debatable. It can be handy if you have
> e.g. an `init()` method that all of your constructors invoke. However, I
> agree it's also confusing, so I will remove.
>
> But we should keep the notion that if a constructor invokes `this()`, and the
> invoked constructor has annotations suppressed, then we skip over the
> constructor invocation.
>
> I will make these updates.
Yep - I guess there's a general theme of "where do we want the warnings to be
reported". My general feeling is that reporting a warning on the constructor
might be enough - but I see that you try to generate a warning in the exact
spot where the leakage happens - which I'm not sure if it's ok, or too clever.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 270:
>>
>>> 268: final boolean analyzable =
>>> this.currentClassIsExternallyExtendable() &&
>>> 269: TreeInfo.isConstructor(tree) &&
>>> 270: !tree.sym.isPrivate() &&
>>
>> Why aren't private constructors analyzed? If we have a class with a private
>> constructor and public static factory invoking said constructor, and the
>> constructor makes `this` escape, isn't that an issue we should detect?
>
>> If we have a class with a private constructor and public static factory
>> invoking said constructor, and the constructor makes this escape, isn't that
>> an issue we should detect?
>
> A static factory method will not create a subclassed instance, so there's no
> 'this' escape problem.
>
> But I admit I completely missed factory methods as a potential thing to worry
> about.
>
> Is it possible for a leak to be missed due to the use of a factory method?
>
> Hmm. I can't immediately think of how, but if you can come up with an example
> please share.
I guess what I'm thinking about:
class Foo {
private Foo() {
m(this);
}
public void m() { ... } // overridable
static Foo makeFoo() { return new Foo(); }
}
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 294:
>>
>>> 292: !(this.currentClass.sym.isSealed() &&
>>> this.currentClass.permitting.isEmpty()) &&
>>> 293: !(this.currentClass.sym.owner.kind == MTH) &&
>>> 294: !this.privateOuter;
>>
>> Here, note that is the owner of the current class symbol is a method, that
>> covers anonymous classes too, which is a part of `privateOuter`. So the only
>> think we need to check here is whether "currentClass" is private, which is a
>> simple predicate. No need to carry `privateOuter` I believe
>
> Unless you explicitly declare a nested class `private`, it won't have the
> `ACC_PRIVATE` flag, even though it is "implicitly private" because it has a
> `private` enclosing class.
>
> Example:
>
> $ cat PrivateOuter.java
> public class PrivateOuter {
> private static class Inner1 {
> static class Inner2 {
> }
> }
> }
> $ javap -v PrivateOuter$Inner1$Inner2
> Classfile
> /Users/archie/proj/jdk/flex-test/classes/PrivateOuter$Inner1$Inner2.class
> Last modified Jan 12, 2023; size 408 bytes
> SHA-256 checksum
> 51ba6d39a5e66df2a078761d6424acbea7a8e32b8451f6ca7d2af49889673b2c
> Compiled from "PrivateOuter.java"
> class PrivateOuter$Inner1$Inner2
> minor version: 0
> major version: 63
> flags: (0x0020) ACC_SUPER
> this_class: #7 // PrivateOuter$Inner1$Inner2
> super_class: #2 // java/lang/Object
> ...
>
>
> So we have to keep track of this "implicit privateness" manually, which is
> what the `privateOuter` flag is for.
D'oh - missed the `|=` - so this keeps updating...
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:32:19 GMT, Maurizio Cimadamore wrote: > If we have a class with a private constructor and public static factory > invoking said constructor, and the constructor makes this escape, isn't that > an issue we should detect? A static factory method will not create a subclassed instance, so there's no 'this' escape problem. But I admit I completely missed factory methods as a potential thing to worry about. Is it possible for a leak to be missed due to the use of a factory method? Hmm. I can't immediately think of how, but if you can come up with an example please share. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:25:27 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 218:
>
>> 216: new TreeScanner() {
>> 217:
>> 218: private Lint lint = ThisEscapeAnalyzer.this.lint;
>
> On a first look I'm not sure about the granularity of suppression here. I
> believe that suppressing at the class level, or at the constructor level is
> enough. Allowing to further annotate var declarations and non-constructor
> methods, while doable, might actually be counter productive - in the sense
> that the annotation does not occur in the constructor (where you'd want to
> see it) but in some other method. I think the fact that a constructor is
> escaping (willingly) should be a visible thing. Something to consider.
Two things...
We need to support annotations on field declarations because their initializers
are effectively mini-constructors. But we don't need it on local variables,
parameters, etc. - too confusing.
For annotations on methods, yes it's debatable. It can be handy if you have
e.g. an `init()` method that all of your constructors invoke. However, I agree
it's also confusing, so I will remove.
But we should keep the notion that if a constructor invokes `this()`, and the
invoked constructor has annotations suppressed, then we skip over the
constructor invocation.
I will make these updates.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 304:
>
>> 302:
>> 303: // We are looking for analyzable constructors only
>> 304: final Symbol sym = entry.getKey();
>
> This seems unused. And, if so, perhaps we only need a `Set`, not
> a map.
Thanks, you're right, the map keys are not used here. I'll clean up the loop.
But we still need a map, not a set, because the map is used elsewhere.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:18:27 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 294:
>
>> 292: !(this.currentClass.sym.isSealed() &&
>> this.currentClass.permitting.isEmpty()) &&
>> 293: !(this.currentClass.sym.owner.kind == MTH) &&
>> 294: !this.privateOuter;
>
> Here, note that is the owner of the current class symbol is a method, that
> covers anonymous classes too, which is a part of `privateOuter`. So the only
> think we need to check here is whether "currentClass" is private, which is a
> simple predicate. No need to carry `privateOuter` I believe
Unless you explicitly declare a nested class `private`, it won't have the
`ACC_PRIVATE` flag, even though it is "implicitly private" because it has a
`private` enclosing class.
Example:
$ cat PrivateOuter.java
public class PrivateOuter {
private static class Inner1 {
static class Inner2 {
}
}
}
$ javap -v PrivateOuter$Inner1$Inner2
Classfile
/Users/archie/proj/jdk/flex-test/classes/PrivateOuter$Inner1$Inner2.class
Last modified Jan 12, 2023; size 408 bytes
SHA-256 checksum
51ba6d39a5e66df2a078761d6424acbea7a8e32b8451f6ca7d2af49889673b2c
Compiled from "PrivateOuter.java"
class PrivateOuter$Inner1$Inner2
minor version: 0
major version: 63
flags: (0x0020) ACC_SUPER
this_class: #7 // PrivateOuter$Inner1$Inner2
super_class: #2 // java/lang/Object
...
So we have to keep track of this "implicit privateness" manually, which is what
the `privateOuter` flag is for.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 13:01:44 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 334:
>
>> 332: // of the stack trace of B. For example, if constructor Foo(int
>> x) has a leak, and constructor
>> 333: // Foo() invokes this(0), then emitting a warning for Foo()
>> would be redundant.
>> 334: final BiPredicate
>> extendsAsPrefix = (warning1, warning2) -> {
>
> Another strategy would be to give a single warning per leaking constructor.
We already impose a limit of at most one warning per constructor.
But for this limit what is technically meant by "per constructor" is "At most
one warning per constructor, assuming that constructor is the one being
analyzed (i.e., the one invoked by the subclass and not via `this()`)."
So that limitation doesn't stop a constructor from generating the same warning
multiple times due to it being invoked indirectly by other constructors via
`this()`. Those duplicate warnings are what the code above eliminates.
So this code only generates one warning, and it's reported for the second
constructor:
public class DupWarn1 {
public DupWarn1() {
this(0);
}
public DupWarn1(int x) {
this.mightLeak();
}
public void mightLeak() {
}
}
DupWarn1.java:8: warning: [this-escape] possible 'this' escape before subclass
is fully initialized
this.mightLeak();
^
This is appropriate. The leak is really in the second constructor; the first
constructor has nothing to do with it. Reporting a leak for both constructors
would be redundant.
An interesting side question: Is it possible to come up with an example where
constructor A has a 'this' leak, and some other constructor B invokes `this()`
to delegate to A, but when B delegates to A the leak occurs differently, or not
at all?
Because of the "flood" analysis, and the fact that you can't pass 'this'
references to `this()` or `super()` (these are static contexts), I don't think
it's possible.
So in fact the deduplication will always apply whenever `this()` is involved.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 334:
> 332: // of the stack trace of B. For example, if constructor Foo(int
> x) has a leak, and constructor
> 333: // Foo() invokes this(0), then emitting
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 163:
> 161: * invoked from the target constructor; if empty, we're still in
> the constructor.
> 162: */
> 163: private final ArrayDeque callStack = new
> A
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
FWIW, the make changes look good. I have nothing to say about the actual lint
code itself.
-
Marked as reviewed by ihse (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
Per the discussion in "Effective Java" on calling overridable methods in a
constructor, future refinements of the check under review here could be
extended to examine the bodies of clone and readObject methods.
-
PR: https://git.openjdk.org/jdk
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 15:59:29 GMT, Maurizio Cimadamore wrote: > What I'm interested in though is what incremental improvement is brought by > the more complex analysis you have in this PR? It's a good question. Here are some thoughts... One meta-goal is that this analysis be conservative. In other words, if your code does NOT generate any warnings, then you should feel confident that there is a high probability that there are no leaks. This is analogous to how you can be confident that you won't get any `ClassCastExceptions` at runtime if your code doesn't generate any `unchecked` warnings at compile time. I think this point is generally agreed upon. If so, then we would want any simpler analysis to err on the side of generating more false positives rather than more false negatives. Assuming that, then the question comes down to a trade-off between code complexity vs. rate of false positives. >From my casual looking over the JDK, the current algorithm generates very few >false positives - almost all of the warnings represent actual leaks (I'd be >interested in any false positives you do see). (Of course, an irony is that most of these leaks have no real-world effect. For example package-private classes in the JDK (a) have already been debugged long ago, so any intra-package bugs due to 'this' escapes have already been fixed; and (b) are unlikely to be subclassed by anyone. And the ones that have a real-world effect (e.g., `HashSet(Collection)`) can't be fixed because of backward compatibility concerns. So this warning is most useful when writing new code.) So the current code is clearly "complex enough" already. FWIW that's ~975 lines of code excluding blank lines and comments. Now to answer your question about a theoretical simpler analysis: > let's say that we don't care about tracking where `this` ends up going > exactly (e.g. if it's aliased by another variable) - and perhaps also let's > say we don't care about inspecting the method to which `this` is leaked too > closely - e.g. we treat any early escape of this as a possible issue. I'm not sure I completely understand the semantics. But are you saying that if a constructor invokes a private or static method, then this simpler analysis would always declare a leak? If that's the case then I think there are a lot of new false positives, because this is common in constructors. I would then worry that if we dilute the warnings with a bunch of new false positives people are just going to get discouraged and turn the warning off completely. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
More questions. Let's say that we only tracked escaping of direct references to
`this`. Do you have any sense of how many "less" warnings would be reported in
real code (e.g. JDK) ? What I mean by this - let's say that we don't care about
tracking _where_ `
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
> This PR adds a new lint warning category `this-escape`.
>
> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
> allow the JDK to continue to compile with `-Xlint:all`.
>
> A 'this' escape warning is generated for a constructor `A()` in a class `A`
> when the compiler detects that the following situation is _in theory
> possible:_
> * Some subclass `B extends A` exists, and `B` is defined in a separate source
> file (i.e., compilation unit)
> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
> * During the execution of `A()`, some non-static method of `B.foo()` could
> get invoked, perhaps indirectly
>
> In the above scenario, `B.foo()` would execute before `A()` has returned and
> before `B()` has performed any initialization. To the extent `B.foo()`
> accesses any fields of `B` - all of which are still uninitialized - it is
> likely to function incorrectly.
>
> Note, when determining if a 'this' escape is possible, the compiler makes no
> assumptions about code outside of the current compilation unit. It doesn't
> look outside of the current source file to see what might actually happen
> when a method is invoked. It does follow method and constructors within the
> current compilation unit, and applies a simplified
> union-of-all-possible-branches data flow analysis to see where 'this' could
> end up.
>
> From my review, virtually all of the warnings generated in the various JDK
> modules are valid warnings in the sense that a 'this' escape, as defined
> above, is really and truly possible. However, that doesn't imply that any
> bugs were found within the JDK - only that the possibility of a certain type
> of bug exists if certain superclass constructors are used by someone,
> somewhere, someday.
>
> For several "popular" classes, this PR also adds `@implNote`'s to the
> offending constructors so that subclass implementors are made aware of the
> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>
> More details and a couple of motivating examples are given in an included
> [doc
> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
> for some background.
>
> Ideally, over time the owners of the various modules would review their
> `@SuppressWarnings("this-escape")` annotations and determine which other
> constructors also warranted such an `@implNote`.
>
> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch of
> different JDK modules. My apologies for that. Adding these annotations was
> determined to be the more conservative approach, as compared to just
> excepting `this-escape` from various module builds globally.
>
> **Patch Navigation Guide**
>
> * Non-trivial compiler changes:
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>
> * Javadoc additions of `@implNote`:
>
> * `src/java.base/share/classes/java/io/PipedReader.java`
> * `src/java.base/share/classes/java/io/PipedWriter.java`
> * `src/java.base/share/classes/java/lang/Throwable.java`
> * `src/java.base/share/classes/java/util/ArrayDeque.java`
> * `src/java.base/share/classes/java/util/EnumMap.java`
> * `src/java.base/share/classes/java/util/HashSet.java`
> * `src/java.base/share/classes/java/util/Hashtable.java`
> * `src/java.base/share/classes/java/util/LinkedList.java`
> * `src/java.base/share/classes/java/util/TreeMap.java`
> * `src/java.base/share/classes/java/util/TreeSet.java`
>
> * New unit tests
> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>
> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
Archie L. Cobbs has updated the pull request incrementally with two additional
commits since the last revision:
- Use the more appropriate Type comparison method Types.isSameType().
- Add some more comments to clarify how the analysis works.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11874/files
- new: https://git.openjdk.org/jdk/pull/11874/files/d70d12f4..6e96a7d7
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=05-06
Stats: 31 lines in 2 files changed: 16 ins; 10 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/11874.diff
Fetc
