Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8015831: Add lint check for calling overridable methods from a constructor #11874

Closed
wants to merge 91 commits into from

Conversation

archiecobbs
Copy link
Contributor

@archiecobbs archiecobbs commented Jan 6, 2023

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 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")


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8299995 to be approved
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

  • JDK-8015831: Add lint check for calling overridable methods from a constructor
  • JDK-6557145: Warn about calling abstract methods in constructors
  • JDK-8299995: Add lint check for calling overridable methods from a constructor (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11874/head:pull/11874
$ git checkout pull/11874

Update a local copy of the PR:
$ git checkout pull/11874
$ git pull https://git.openjdk.org/jdk pull/11874/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11874

View PR using the GUI difftool:
$ git pr show -t 11874

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11874.diff

Warning B duplicates warning A if the stack trace of A is a prefix of
the stack trace of B. For example, if constructor Foo(int x) has a
leak, and constructor Foo() invokes this(0), then emitting a warning
for Foo() would be redundant.
@archiecobbs
Copy link
Contributor Author

I agree with this conclusion. Also, even if a class is public, our question should be: is this class in a non-exported package? Because, if so, even a public class can be "implementation specific".

Agreed - I originally thought this was harder but actually the containing module is easy to access.

I've prototyped this now in a new ThisEscapeModule branch - relative diff is here.

@archiecobbs
Copy link
Contributor Author

FYI, based on discussions & feedback I'm making the warning less aggressive. We'll now only warn when a subclass could exist outside of the current module, instead of outside the current compilation unit (i.e., source file).

FWIW this reduces the warning count in the JDK itself from 2093 to 1334 (36% reduction).

In the future we can consider tightening this up, but for now this will be a more conservative way to introduce the new warning to the world.

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the last version, @archiecobbs thanks a lot for the patience and continuous effort

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the latest version too with improved boundary detection.
Moving forward, if we plan to add other complex analyses like this one, we should maybe think to add them as a separate compilation step, rather than adding them in Flow (which might be problematic as parts of Flow are also used as part of Attr. e.g. to infer lambda thrown types).

@archiecobbs
Copy link
Contributor Author

Thanks for the careful reviews.

Moving forward, if we plan to add other complex analyses like this one, we should maybe think to add them as a separate compilation step, rather than adding them in Flow (which might be problematic as parts of Flow are also used as part of Attr. e.g. to infer lambda thrown types).

Agreed. Perhaps there could be a new phase of compliation ANALYZE_SOURCE (?) after FLOW where code can assume that all source code is already resolved, validated, attributed, etc. This phase would be the place for read-only tasks like this one, i.e., source-level analyses that generate warnings or perhaps generate some kind of derivative output like a cross-reference file or dependency report.

@archiecobbs
Copy link
Contributor Author

The CSR also needs a review from a compiler-dev engineer as well if anyone is interested... thanks.

@openjdk
Copy link

openjdk bot commented Jan 23, 2023

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@vicente-romero-oracle
Copy link
Contributor

vicente-romero-oracle commented Jan 24, 2023

The CSR also needs a review from a compiler-dev engineer as well if anyone is interested... thanks.

some comments on the CSR:

  • I think the value for field Compatibility Risk could be low, instead of medium. Even for code that uses -Xlint:all users will mostly see more warnings, they can change this setting or add @sw annotations to the code if available
  • The Implementation section I think could be moved to a JIRA comment
  • I think that the Specification section has text that probably is describing the solution, to me the specification section should start in this line: Changes to the module-info.java Javadoc for module jdk.compiler:

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 21, 2023

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@archiecobbs
Copy link
Contributor Author

/pingbot

@openjdk
Copy link

openjdk bot commented Feb 21, 2023

@archiecobbs Unknown command pingbot - for a list of valid commands use /help.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 2, 2023
@openjdk
Copy link

openjdk bot commented Mar 2, 2023

@archiecobbs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8015831: Add lint check for calling overridable methods from a constructor
6557145: Warn about calling abstract methods in constructors

Reviewed-by: ihse, vromero, mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 896 new commits pushed to the master branch:

  • b51ea42: 8303354: addCertificatesToKeystore in KeystoreImpl.m needs CFRelease call in early potential CHECK_NULL return
  • fb13063: 8303409: Add Windows AArch64 ABI support to the Foreign Function & Memory API
  • c9afd55: 8302820: Remove costs for NMTPreInit when NMT is off
  • 72de24e: 8303457: Introduce convenience test library APIs for creating test servers for tests in test/jdk/java/net/httpclient
  • 3091744: 8303418: Improve parameter and variable names in BitMap
  • dbb562d: 8303355: The Depend plugin does fully recompile when primitive type changes
  • 4619e8b: 8297587: Upgrade JLine to 3.22.0
  • 99f5687: 8302144: Move ZeroTLABTest.java to tier3
  • dc08216: 8303186: Missing Classpath exception from Continuation.c
  • d10d40a: 8303077: JFR: Add example usage to jdk.jfr
  • ... and 886 more: https://git.openjdk.org/jdk/compare/c6588d5bb3f778806c9112e86dbfba964c0636fd...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @vicente-romero-oracle, @mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 2, 2023
@archiecobbs
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 2, 2023
@openjdk
Copy link

openjdk bot commented Mar 2, 2023

@archiecobbs
Your change (at version 86e26a7) is now ready to be sponsored by a Committer.

@vicente-romero-oracle
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Mar 17, 2023

Going to push as commit 8f5bb53.
Since your change was applied there have been 1120 commits pushed to the master branch:

  • b085ab9: 8180387: com.sun.source.util.JavacTask should have a protected constructor.
  • bfb812a: 8292818: replace 96-bit representation for field metadata with variable-sized streams
  • 932be35: 8298469: Obsolete legacy parallel class loading workaround for non-parallel-capable class loaders
  • 02a4ee2: 8303921: serviceability/sa/UniqueVtableTest.java timed out
  • 4486f1b: 8304367: jlink --include-locales=* attempts to parse non .class resource files with classfile reader
  • 8d2ebf2: 8303697: ProcessTools doesn't print last line of process output
  • d5a1507: 8304314: StackWalkTest.java fails after CODETOOLS-7903373
  • 384a8b8: 8303069: Memory leak in CompilerOracle::parse_from_line
  • 6dd6c15: 8301684: Fix test code to not get finalizer deprecation warnings
  • cb4ae19: 8292059: Do not inline InstanceKlass::allocate_instance()
  • ... and 1110 more: https://git.openjdk.org/jdk/compare/c6588d5bb3f778806c9112e86dbfba964c0636fd...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 17, 2023
@openjdk openjdk bot closed this Mar 17, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 17, 2023
@openjdk
Copy link

openjdk bot commented Mar 17, 2023

@vicente-romero-oracle @archiecobbs Pushed as commit 8f5bb53.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org compiler compiler-dev@openjdk.org integrated Pull request has been integrated
10 participants