Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-12 Thread Archie L . Cobbs
On Thu, 12 Jan 2023 15:10:19 GMT, Maurizio Cimadamore wrote: > Interesting example - I thought you might have been referring to a case where > the class being analyzed was itself an exception. Yes - although that example doesn't compile (oops!). Just replace `catch (RuntimeException e)` with

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-12 Thread Maurizio Cimadamore
On Thu, 12 Jan 2023 14:59:12 GMT, Archie L. Cobbs wrote: >>> * On the Java stack >>> (a) The current 'this' instance >>> (b) A method parameter >>> (c) A local variable >>> (d) A temporary value that is part of the current expression being >>> evaluated >>> (e) The return value from a

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-12 Thread Archie L . Cobbs
On Thu, 12 Jan 2023 09:57:00 GMT, Maurizio Cimadamore wrote: > I'm not sure what you mean by (1f). You mean this can be embedded in an > exception being thrown? Is that different from (2)? Yes, this would be a different case from any other that you'd have to handle in the code if you wanted t

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-12 Thread Maurizio Cimadamore
On Thu, 12 Jan 2023 02:14:10 GMT, Archie L. Cobbs wrote: >>> >>> D'oh, you're right. But if you made `returnMe()` static or private then the >>> argument would still hold. >>> >>> > And, if the method is static, same story - you are passing ref3 somewhere >>> > else, and ref3 potentially cont

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Archie L . Cobbs
On Thu, 12 Jan 2023 00:15:08 GMT, Maurizio Cimadamore wrote: > So, for static methods, it could go down two ways: either we don't even look > at referenced method bodies, give up and just declare "sorry, escaping". Or, > if we look into method bodies, and see that the relationship between inne

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Maurizio Cimadamore
On Wed, 11 Jan 2023 22:40:43 GMT, Archie L. Cobbs wrote: > > D'oh, you're right. But if you made `returnMe()` static or private then the > argument would still hold. > > > And, if the method is static, same story - you are passing ref3 somewhere > > else, and ref3 potentially contains this. >

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Archie L . Cobbs
On Wed, 11 Jan 2023 21:51:45 GMT, Maurizio Cimadamore wrote: > So, in this example though you are calling an instance method before the > object is initialized, which would seem to me like a leak D'oh, you're right. But if you made `returnMe()` static or private then the argument would still

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Maurizio Cimadamore
On Wed, 11 Jan 2023 21:45:20 GMT, Maurizio Cimadamore wrote: >> Regarding the assignment graph approach, I think that would work if the >> references are bouncing around strictly between variables, but what if the >> chain includes any of the more complicated stuff that is currently being >>

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Maurizio Cimadamore
On Wed, 11 Jan 2023 19:10:04 GMT, Archie L. Cobbs wrote: >> Good idea. Looks like the difference is in the noise, at least on my Macbook: >> >> Builds of master (jdk-21+3-69-gc6588d5bb3f) >> == >> >> Build times: >> >> real 2m24.650s >> user 13m4

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Archie L . Cobbs
On Wed, 11 Jan 2023 18:44:20 GMT, Archie L. Cobbs wrote: >> Also, looking at the loop test more closely, it seems to me that what the >> compiler needs to do is to prove that there can be possible paths by which a >> `this` can land into ref4. >> >> If we build a graph of all the assignments,

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Archie L . Cobbs
On Wed, 11 Jan 2023 16:14:24 GMT, Maurizio Cimadamore wrote: >> True - probably 3 * 3 can be achieved if this: >> >> >> ThisEscapeLoop ref21 = ref14; >> >> Is replaced with >> >> >> ThisEscapeLoop ref21 = this; >> >> >> In which case the inner loop won't converge immediately (a

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Maurizio Cimadamore
On Wed, 11 Jan 2023 14:12:46 GMT, Maurizio Cimadamore wrote: >> Actually I think it would take 1 + 1 + 3 iterations. >> >> During the first two iterations of the outer loop, nothing changes after the >> first go round of the inner loop - i.e., the total set of possible >> references in existe

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-11 Thread Maurizio Cimadamore
On Wed, 11 Jan 2023 00:22:03 GMT, Archie L. Cobbs wrote: >> So, if the code was be like this: >> >> >> ThisEscapeLoop ref11 = this; >> ThisEscapeLoop ref12 = null; >> ThisEscapeLoop ref13 = null; >> ThisEscapeLoop ref14 = null; >> for (int i = 0; i < 100; i++) { >> ref14 = ref13; >> ref

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Archie L . Cobbs
On Tue, 10 Jan 2023 23:38:14 GMT, Maurizio Cimadamore wrote: >> OK I'm glad you pointed that out because I'm a little unclear on the best >> way to do this bit. >> >> Just to confirm, you are saying that this: >> >> `if (erasure(type).equalsIgnoreMetadata(outerType)) {` >> >> should be repla

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Archie L . Cobbs
On Wed, 11 Jan 2023 00:04:14 GMT, Maurizio Cimadamore wrote: >> Yes, because the 'this' reference can bounce around through different >> variables in scope each time around the loop. So we have to repeat the loop >> until all 'this' references have "flooded" into all the nooks and crannies. >>

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Archie L . Cobbs
On Tue, 10 Jan 2023 23:45:59 GMT, Maurizio Cimadamore wrote: >> It's slightly different from that. >> >> Considering any of the various things in scope that can point to an object >> (these are: the current 'this' instance, the current outer 'this' instance, >> method parameter/variable, meth

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Maurizio Cimadamore
On Tue, 10 Jan 2023 19:20:35 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java >> line 1098: >> >>> 1096: private void visitLooped(T tree, Consumer >>> visitor) { >>> 1097: this.visitScoped(tree, false, t -> { >>> 1098:

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Maurizio Cimadamore
On Tue, 10 Jan 2023 19:18:04 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java >> line 85: >> >>> 83: * >>> 84: * >>> 85: * When tracking references, we distinguish between direct references >>> and indirect references, >> >> I'

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Maurizio Cimadamore
On Tue, 10 Jan 2023 19:42:06 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2345: >> >>> 2343: if (!innerType.hasTag(CLASS) || !outerType.hasTag(CLASS)) >>> 2344: return false; >>> 2345: innerType = erasure(inner

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Archie L . Cobbs
On Mon, 9 Jan 2023 14:23:47 GMT, Maurizio Cimadamore wrote: >> Archie L. Cobbs has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix incorrect @bug numbers in unit tests. > > src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.j

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Archie L . Cobbs
On Mon, 9 Jan 2023 15:03:10 GMT, Maurizio Cimadamore wrote: >> Archie L. Cobbs has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix incorrect @bug numbers in unit tests. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEsc

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-10 Thread Maurizio Cimadamore
On Sat, 7 Jan 2023 21:08:07 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 gener

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-09 Thread Archie L . Cobbs
On Mon, 9 Jan 2023 06:37:22 GMT, David Holmes wrote: > All your new files need a copyright and GPL header. Sorry if I'm being blind but I'm not seeing it. Which file(s) are you referring to? The `@test /nodynamiccopyright/` files don't get one per [this](https://openjdk.org/groups/compiler/te

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-08 Thread David Holmes
On Sat, 7 Jan 2023 21:08:07 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 gener

Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

2023-01-07 Thread Archie L . Cobbs
> 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