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

DRAFT: Make AST map pre 1.8 versions to 1.8 #3533

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akurtakov
Copy link
Contributor

As the compiler mandates 1.8 already it makes sense for the AST to do the same.

@akurtakov akurtakov marked this pull request as draft January 8, 2025 16:28
@akurtakov
Copy link
Contributor Author

@iloveeclipse What do you think?

@iloveeclipse
Copy link
Member

@iloveeclipse What do you think?

Jenkins is not accessible, how many test fails are there? I'm not AST expert, I can only remember I've tried to use latest AST in JDT debug and that failed with tons of errors...

@akurtakov
Copy link
Contributor Author

Jenkins issue is https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5498 . My question is more about the goal not that much about the work involved ( I know it will require quite some).
Btw, can you point me what you have tried in jdt.debug so I can use/test it with this one?

@iloveeclipse
Copy link
Member

My question is more about the goal not that much about the work involved ( I know it will require quite some).

I assume working with "old" level AST for Java version we don't support anymore should be more of academic interest? I don't know if consumers like Xtext use AST at some minimal JLS level internally and expect us to keep it going forever. @szarnekow ?

Btw, can you point me what you have tried in jdt.debug so I can use/test it with this one?

I see I've finally managed to fix it: eclipse-jdt/eclipse.jdt.debug#109, so that should be fine.

As the compiler mandates 1.8 already it makes sense for the AST to do
the same.
@akurtakov
Copy link
Contributor Author

akurtakov commented Jan 9, 2025

@iloveeclipse FYI, there are no test failures, presumably because the maps changes make tests run with 1.8 level which all the tests are supposedly succeeding with.

@iloveeclipse
Copy link
Member

Cool.

@akurtakov
Copy link
Contributor Author

May I assume that if @szarnekow (or someone else ) doesn't raise serious concern this approach if fine to proceed with?

@iloveeclipse
Copy link
Member

@mpalat, @srikanth-sankaran, @jarthana , @noopur2507 : any objections?

@iloveeclipse
Copy link
Member

@akurtakov : looking at the code, you only touched the mapping. But the AST constructor still uses JLS4_INTERNAL , JLS2_INTERNAL etc and only has the check below:

if (level < JLS2_INTERNAL && level > JLS_Latest) {
   throw new IllegalArgumentException("Unsupported JLS level : " + level); //$NON-NLS-1$
}

Do you plan to update this as well?

@akurtakov
Copy link
Contributor Author

Yes, I do. Also cleaning the tests to not run old compliance e.g jls2 compliance (

return runJLS3Conversion(unit, resolveBindings, checkJLS2, false);
leading to ) which significantly complicates, slows down and prevent the suite to be useful to verify "supported" JDT features are implemented proper (a good number of ast failures in https://ci.eclipse.org/ls/job/jdt-core-incubator/job/dom-with-javac/lastSuccessfulBuild/testReport/ ).

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.

2 participants