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

8240658: Code completion not working for lambdas in method invocations that require type inference #50

Closed
wants to merge 4 commits into from

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Sep 7, 2020

When a method invocation requires type inference, and the user is in the process of typing of a (block) lambda that is a parameter to the method invocation, javac may not, in some cases, perform the type inference, which then may lead to non working code completion in JShell (and possibly other tools).

For example (in JShell):
Arrays.stream(new Integer[]{1}).forEach(v -> { System.err.println(v.

and press - this leads to no proposals currently, as the type of "v" is not inferred.

The idea of the proposed patch is to improve the recovery in cases significant for cases where the text is being typed, and allow type inference in these cases. The type of "v" is then inferred, and the code completion works.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8240658: Code completion not working for lambdas in method invocations that require type inference

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/50/head:pull/50
$ git checkout pull/50

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 7, 2020

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 7, 2020
@openjdk
Copy link

openjdk bot commented Sep 7, 2020

@lahodaj The following labels will be automatically applied to this pull request: compiler kulla.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added compiler compiler-dev@openjdk.org kulla kulla-dev@openjdk.org labels Sep 7, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 7, 2020

Webrevs

rollback.append(() -> {
lambda.body = new TreeTranslator() {
@SuppressWarnings("unchecked")
public <T extends JCTree> T translate(T t) {

Choose a reason for hiding this comment

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

Jackpot:
warning: Add @OverRide Annotation

}

protected AttrRecover(Context context) {
context.put(attrRepairKey, this);

Choose a reason for hiding this comment

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

Jackpot:
warning: Leaking this in constructor

* questions.
*/

package test;

Choose a reason for hiding this comment

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

Jackpot:
warning: Incorrect Package

* questions.
*/

package test;

Choose a reason for hiding this comment

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

Jackpot:
warning: Incorrect Package


private static void test() {
Test.of(1).convert(c1 -> {Object o = c1/*getTypeMirror:DECLARED:java.lang.Integer*/;});
Test.of(1).consume(c2 -> {Object o = c2/*getTypeMirror:DECLARED:java.lang.Integer*/; return null;});

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

Test.of(1).consumeWithParam(c3 -> {Object o = c3/*getTypeMirror:DECLARED:java.lang.Integer*/;});
convert(0, c4 -> {Object o = c4/*getTypeMirror:DECLARED:java.lang.Integer*/;});
consume(0, c5 -> {Object o = c5/*getTypeMirror:DECLARED:java.lang.Integer*/;});
convertVarArgs(0, c6 -> {Object o = c6/*getTypeMirror:DECLARED:java.lang.Integer*/;}, 1, 2, 3, 4);

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

public class TestGetTypeMirrorReferenceData {

private static void test() {
Test.of(1).convert(c1 -> {Object o = c1/*getTypeMirror:DECLARED:java.lang.Integer*/;});

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

convert(0, c4 -> {Object o = c4/*getTypeMirror:DECLARED:java.lang.Integer*/;});
consume(0, c5 -> {Object o = c5/*getTypeMirror:DECLARED:java.lang.Integer*/;});
convertVarArgs(0, c6 -> {Object o = c6/*getTypeMirror:DECLARED:java.lang.Integer*/;}, 1, 2, 3, 4);
consumeVarArgs(0, c7 -> {Object o = c7/*getTypeMirror:DECLARED:java.lang.Integer*/;}, 1, 2, 3, 4);

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

Test.of(1).consume(c2 -> {Object o = c2/*getTypeMirror:DECLARED:java.lang.Integer*/; return null;});
Test.of(1).consumeWithParam(c3 -> {Object o = c3/*getTypeMirror:DECLARED:java.lang.Integer*/;});
convert(0, c4 -> {Object o = c4/*getTypeMirror:DECLARED:java.lang.Integer*/;});
consume(0, c5 -> {Object o = c5/*getTypeMirror:DECLARED:java.lang.Integer*/;});

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

Test.of(1).convert(c1 -> {Object o = c1/*getTypeMirror:DECLARED:java.lang.Integer*/;});
Test.of(1).consume(c2 -> {Object o = c2/*getTypeMirror:DECLARED:java.lang.Integer*/; return null;});
Test.of(1).consumeWithParam(c3 -> {Object o = c3/*getTypeMirror:DECLARED:java.lang.Integer*/;});
convert(0, c4 -> {Object o = c4/*getTypeMirror:DECLARED:java.lang.Integer*/;});

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read


public class TestGetTypeMirrorReferenceData {

private static void test() {

Choose a reason for hiding this comment

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

Jackpot:
warning: test is never used

private static void test() {
Test.of(1).convert(c1 -> {Object o = c1/*getTypeMirror:DECLARED:java.lang.Integer*/;});
Test.of(1).consume(c2 -> {Object o = c2/*getTypeMirror:DECLARED:java.lang.Integer*/; return null;});
Test.of(1).consumeWithParam(c3 -> {Object o = c3/*getTypeMirror:DECLARED:java.lang.Integer*/;});

Choose a reason for hiding this comment

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

Jackpot:
warning: Variable o is never read

JCMethodInvocation mit = (JCMethodInvocation) todo.env.tree;
if ((todo.candSym.flags() & Flags.VARARGS) == 0 &&
mit.args.length() > todo.candSym.type.getParameterTypes().length()) {
break RECOVER; //too many actual parameters, skip
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 9, 2020

Choose a reason for hiding this comment

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

question: why not covering varargs too?

Copy link
Contributor Author

@lahodaj lahodaj Sep 10, 2020

Choose a reason for hiding this comment

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

Note this is a case where the invoked method is not a varargs method, and there are too many actual parameters. But it is true the varags handling could be improved, which I tried to do in a recent patch (32845dd).

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 10, 2020

Choose a reason for hiding this comment

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

I like the patch. I think it is great. My only comment would be about test: TestGetTypeMirrorReference.java which does a golden file like comparison but I understand that there are not many options. I would suggest though to place this test along with file: TestGetTypeMirrorReferenceData.java in a separate folder and add some doc to the test so that it would be easier to modify it in the future

Copy link
Contributor Author

@lahodaj lahodaj Sep 11, 2020

Choose a reason for hiding this comment

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

Thanks!

I've tried to improve the text (moved to a separate directory, added comment) in 988f85b.

//do not touch nested lambdas
}
@Override
public void visitClassDef(JCClassDecl tree) {
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 9, 2020

Choose a reason for hiding this comment

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

just wondering, shouldn't anonymous classes be avoided too?

Copy link
Contributor Author

@lahodaj lahodaj Sep 10, 2020

Choose a reason for hiding this comment

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

Anonymous classes (JCNewClass) contain JCClassDecl representing the actual anonymous class, so this should avoid anonymous classes as well.

JCMethodInvocation mit = (JCMethodInvocation) todo.env.tree;
if ((todo.candSym.flags() & Flags.VARARGS) == 0 &&
mit.args.length() > todo.candSym.type.getParameterTypes().length()) {
break RECOVER; //too many actual parameters, skip
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 10, 2020

Choose a reason for hiding this comment

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

I like the patch. I think it is great. My only comment would be about test: TestGetTypeMirrorReference.java which does a golden file like comparison but I understand that there are not many options. I would suggest though to place this test along with file: TestGetTypeMirrorReferenceData.java in a separate folder and add some doc to the test so that it would be easier to modify it in the future

@openjdk
Copy link

openjdk bot commented Sep 10, 2020

@lahodaj This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8240658: Code completion not working for lambdas in method invocations that require type inference

Reviewed-by: vromero
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 32 commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate b05290aaeaf7cd96692893264d77a9ab0857aabf.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 10, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 10, 2020

Mailing list message from Robert Field on kulla-dev:

The code changes are in the javac code, so an active compiler team
member would be the appropriate review.

The jshell test looks good.

Thanks for addressing this,
Robert

On 2020-09-07 04:35, Jan Lahoda wrote:

@vicente-romero-oracle
Copy link
Contributor

vicente-romero-oracle commented Sep 11, 2020

I like the changes done at (988f85b) thanks. I think the patch is ready to go

@lahodaj
Copy link
Contributor Author

lahodaj commented Sep 14, 2020

/test

@openjdk
Copy link

openjdk bot commented Sep 14, 2020

A test job has been started with id: github.com-149121954-50-691848798

@openjdk
Copy link

openjdk bot commented Sep 14, 2020

@lahodaj your test job with id github.com-149121954-50-691848798 for commits up until 988f85b has finished.

@lahodaj
Copy link
Contributor Author

lahodaj commented Sep 14, 2020

/integrate

@openjdk openjdk bot closed this Sep 14, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 14, 2020
@openjdk
Copy link

openjdk bot commented Sep 14, 2020

@lahodaj Since your change was applied there have been 32 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 68da63d.

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

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Late to the party; I like the fact that the recovery logic now seats neatly in its own class. In fact, I think we should probably push this even further, and explore the possibility of decoupling regular attribution from recovery attribution completely - e.g. have recovery attribution run in its separate compiler pass. Probably a bit tricky to pull off in full, but I think it will significantly simplify the maintenance of the attr code going forward.

@lahodaj
Copy link
Contributor Author

lahodaj commented Sep 15, 2020

@mcimadamore - sure, I'll look into that!

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