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

2023-01-16 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

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

2023-01-16 Thread Maurizio Cimadamore
On Mon, 16 Jan 2023 16:29:31 GMT, Archie L. Cobbs wrote: > It looks like you might be counting the "here via invocation" lines as > separate warnings. These are really part of the previous `possible 'this' > escape` line, e.g.: yes, I really did the simplest possible thing (e.g. just counting

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

2023-01-16 Thread Archie L . Cobbs
On Mon, 16 Jan 2023 12:51:49 GMT, Maurizio Cimadamore wrote: >> The number of times around any single loop is bounded by the number of new >> references that can possibly be created during the analysis of that loop. >> >> That number is at most 2 * (1 + 1 + 1 + 1 + N) where N is the number of

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

2023-01-16 Thread Archie L . Cobbs
On Mon, 16 Jan 2023 11:53:40 GMT, Maurizio Cimadamore wrote: >> Archie L. Cobbs has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Fix bug where field initializer warnings could be incorrectly suppressed. >> - Consolidate all the unit

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

2023-01-16 Thread Maurizio Cimadamore
On Fri, 13 Jan 2023 21:28:51 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java >> line 1088: >> >>> 1086: private void visitLooped(T tree, Consumer >>> visitor) { >>> 1087: visitScoped(tree, false, t -> { >>> 1088:

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

2023-01-16 Thread Maurizio Cimadamore
On Fri, 13 Jan 2023 22:48:59 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

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

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

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

2023-01-13 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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 21:33:02 GMT, Archie L. Cobbs wrote: >> Sounds good - thanks. > > Ah. I just realized that we need to do it your way because of the following > bug: > > Suppose you have a constructor and a field with initializer that both leak, > but you have

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 16:20:41 GMT, Archie L. Cobbs wrote: >> I'm OK either way we can revisit this later either as part of this PR or in >> a future one. I let it to your consideration > > Sounds good - thanks. Ah. I just realized that we need to do it your way because of the following bug:

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 20:21:24 GMT, Vicente Romero wrote: >> Archie L. Cobbs has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Use more idiomatic test for java.lang.Object. >> - Revert 27cb30129; the error was actually fixed in

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 20:16:25 GMT, Vicente Romero wrote: >> Archie L. Cobbs has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Use more idiomatic test for java.lang.Object. >> - Revert 27cb30129; the error was actually fixed in

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 19:09:54 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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 19:09:54 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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 17:49:05 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java >> line 685: >> >>> 683: >>> 684: @Override >>> 685: public void visitDoLoop(JCDoWhileLoop tree) { >> >> I was thinking, code can also loop

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

2023-01-13 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

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 17:35:08 GMT, Vicente Romero wrote: >> Archie L. Cobbs has updated the pull request incrementally with 16 >> additional commits since the last revision: >> >> - Fix bug where all but the last yeild statement were being ignored. >> - Add method RefSet.mapInto() and use to

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 04:04:36 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

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 16:12:50 GMT, Vicente Romero wrote: >> Yes... I did it that way is so that it doesn't require any adaptation >> if/when JDK-8194743 ever gets implemented. And it keeps the code a little >> simpler in exchange for a little redundancy. >> >> I'm happy to fix this if you

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

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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 15:14:05 GMT, Archie L. Cobbs wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java >> line 516: >> >>> 514: Name name = TreeInfo.name(invoke.meth); >>> 515: if (name == names._super) { >>> 516:

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

2023-01-13 Thread Maurizio Cimadamore
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

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

2023-01-13 Thread Archie L . Cobbs
On Fri, 13 Jan 2023 12:42:24 GMT, Vicente Romero wrote: >> Archie L. Cobbs has updated the pull request incrementally with 16 >> additional commits since the last revision: >> >> - Fix bug where all but the last yeild statement were being ignored. >> - Add method RefSet.mapInto() and use to

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

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

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

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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 04:04:36 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

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

2023-01-13 Thread Vicente Romero
On Fri, 13 Jan 2023 04:04:36 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

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

2023-01-13 Thread Maurizio Cimadamore
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

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

2023-01-13 Thread Maurizio Cimadamore
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; >> } >> } >> >> >>

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

2023-01-13 Thread Maurizio Cimadamore
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

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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;

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

2023-01-12 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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

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

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

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

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

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

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

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

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

2023-01-12 Thread Archie L . Cobbs
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() {

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

2023-01-12 Thread Maurizio Cimadamore
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:

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

2023-01-12 Thread Maurizio Cimadamore
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 >

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

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

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

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

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

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

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

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

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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: >> >>

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

2023-01-12 Thread Vicente Romero
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

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

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

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

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

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

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

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

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

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 [v7]

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

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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

2023-01-12 Thread Maurizio Cimadamore
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

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

2023-01-12 Thread Magnus Ihse Bursie
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

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

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

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

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

2023-01-11 Thread Joe Darcy
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

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

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

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

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

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

2023-01-11 Thread Maurizio Cimadamore
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

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; >>

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

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

2023-01-10 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

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 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 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,

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, >> >>

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 =

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

2023-01-10 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

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. > >

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

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

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

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

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

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

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

2023-01-07 Thread Archie L . Cobbs
On Sat, 7 Jan 2023 18:03:04 GMT, Alan Bateman wrote: > I don't think the implementation notes should be included as part of the > adding the lint warning as I think it creates an attractive nuisance. I agree with you - not only for that reason, but also because as others have pointed out the

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

2023-01-07 Thread Alan Bateman
On Fri, 6 Jan 2023 23:08:27 GMT, Archie L. Cobbs wrote: > I've moved the `@SuppressWarnings` annotations onto a new branch > `ThisEscapeAnnotations`. This is just for future reference in case somebody > wants to add them back someday and doesn't want to start from scratch. > > > I suspect

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

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

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

2023-01-07 Thread ExE Boss
On Fri, 6 Jan 2023 23:13:09 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

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

2023-01-06 Thread Archie L . Cobbs
On Fri, 6 Jan 2023 15:38:31 GMT, Alan Bateman wrote: >>> The associated JBS issue has been dormant for 6+ years and this is a very >>> intrusive change affecting many, many files. Has the resurrection of this >>> project previously been discussed somewhere? >> >> Hi @dholmes-ora, >> >> The

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

2023-01-06 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

  1   2   >