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

Fix for visit(PatternInstanceofExpression node) in NaiveASTFlattener #2285

Merged

Conversation

twasik81
Copy link
Contributor

@twasik81 twasik81 commented Apr 5, 2024

What it does

The change resolves an issue with the "MISSING" string printed that is a result of parsing PatternInstanceofExpression.

How to test

Author checklist

@gayanper gayanper force-pushed the fb_patternInstanceOfExpression_JDK21 branch from 8f17328 to 195299d Compare April 7, 2024 10:38
@gayanper
Copy link
Contributor

gayanper commented Apr 7, 2024

@twasik81 do you have issue describing where we can observe this behavior ?

@twasik81
Copy link
Contributor Author

twasik81 commented Apr 8, 2024

Here is the result of debugging:

debug

on code snippet:

static boolean checkCollisionIfs(Object p1, Object p2) {
if (p1 instanceof Point(int x1, int y1) && p2 instanceof Point(int x2, int y2)) {
return x1 == x2 && y1 == y2;
}
if (p1 instanceof Point(int x1, int y1) && p2 instanceof ColoredPoint(Point(int x2, int y2), Color c)) {
return x1 == x2 && y1 == y2;
}
if (p1 instanceof ColoredPoint(Point(int x1, int y1), Color c) && p2 instanceof Point(int x2, int y2)) {
return x1 == x2 && y1 == y2;
}
if (p1 instanceof ColoredPoint(Point(int x1, int y1), Color c1)
&& p2 instanceof ColoredPoint(Point(int x2, int y2), Color c2)) {
return x1 == x2 && y1 == y2;
}
throw new IllegalArgumentException("Invalid type");
}

@iloveeclipse
Copy link
Member

@twasik81
Copy link
Contributor Author

twasik81 commented Apr 15, 2024

@iloveeclipse : The ECA is already signed, but I'm not a committer to run the ECA validation tool again.

image

@iloveeclipse
Copy link
Member

The ECA is already signed, but I'm not a committer to run the ECA validation tool again.

I did it and it still failing.

image

Please make sure the mail used in the commit matches the mail used for your Eclipse account.
See hints in https://api.eclipse.org/git/eca/status/gh/eclipse-jdt/eclipse.jdt.core/2285

@twasik81
Copy link
Contributor Author

@iloveeclipse here is eclipse account view:
image
here is git command execution:
image
image

@twasik81
Copy link
Contributor Author

@iloveeclipse: So if there is a problem with ECA validation of my account can someone else create the pull request with the same content? From my point of view is irrelevant who will be a contributor to this fix.

@iloveeclipse
Copy link
Member

From my point of view is irrelevant who will be a contributor to this fix.

But not from IP point of view, as long as commit is not identified to be from a person that signed ECA we cannot accept patches. I've created https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4542 ticket, let see what IT guys say.

@iloveeclipse
Copy link
Member

I've created https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4542 ticket, let see what IT guys say.

@twasik81 : see https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4542#note_2086634.
Please check if you have linked your github account at Eclipse.org profile page.

@twasik81
Copy link
Contributor Author

@iloveeclipse : I've added this:
image

@iloveeclipse
Copy link
Member

@twasik81 : while the ECA check seem to be fixed now (see ticket), could you please add a test case or at least provide a simple snippet that shows the issue?

@twasik81
Copy link
Contributor Author

Here is the code of test:

import org.eclipse.jdt.core.dom.AST;
import org.eclipse.jdt.core.dom.ASTParser;
import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.PatternInstanceofExpression;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertFalse;

public class PatternInstanceOfExpressionTest {
    @Test
    public void testPatternInstanceOfExpression() {

        String content = "public class Foo {" +
                "static boolean checkCollisionIfs(Object p1, Object p2) {" +
                "if (p1 instanceof Point(int x1, int y1)) {" +
                "return x1 == x2 && y1 == y2;" +
                "}}" +
                "record Point(int x, int y) {}" +
                "record ColoredPoint(Point T, String color) {}" +
                "record Decorator(Object obj) {}" +
                "}";


        ASTParser parser = ASTParser.newParser(AST.JLS21);
        parser.setSource(content.toCharArray());
        CompilationUnit cunit = (CompilationUnit) parser.createAST(null);
        NodeFinder astVisitor = new NodeFinder();
        cunit.accept(astVisitor);
        assertFalse(astVisitor.patternInstanceofExpression.toString().contains("MISSING"));
    }
    class NodeFinder extends ASTVisitor {
        PatternInstanceofExpression patternInstanceofExpression;
        public boolean visit(PatternInstanceofExpression node) {
            patternInstanceofExpression = node;
            return super.visit(node);
        }
    }

}

@iloveeclipse
Copy link
Member

Here is the code of test:

Could you please add it in org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/dom - either as single test or to some of the already existing tests? If it will be a dedicated new class, it should be also added to the test suite org.eclipse.jdt.core.tests.dom.RunAllTests.

While doing so, please amend existing commit, rebase it on latest master state & force push to github.
This should also solve the problem with the ECA check.

@twasik81
Copy link
Contributor Author

I've tried but I've failed to run a test under the specified package.
Could you please create a test based on the data that we already provided?

@jarthana jarthana force-pushed the fb_patternInstanceOfExpression_JDK21 branch from 195299d to c4e27a8 Compare July 1, 2024 10:07
@jarthana jarthana merged commit 104a635 into eclipse-jdt:master Jul 1, 2024
9 checks passed
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this pull request Jul 18, 2024
…ipse-jdt#2285)

eclipse-jdt#2285

Author: twasik <twasik@parasoft.com>
Also-by: Jay Arthanareeswaran <jarthana@in.ibm.com>
gayanper pushed a commit to gayanper/eclipse.jdt.core that referenced this pull request Sep 7, 2024
…ipse-jdt#2285)

eclipse-jdt#2285

Author: twasik <twasik@parasoft.com>
Also-by: Jay Arthanareeswaran <jarthana@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants