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

Remove AbstractUnnamedTypeDeclaration #2483

Merged

Conversation

datho7561
Copy link
Contributor

@datho7561 datho7561 commented May 22, 2024

What it does

See #2481

  • Rework hierarchy so that ImplicitTypeDeclaration depends on AbstractTypeDeclaration
    • Use an empty SimpleName node for ImplicitTypeDeclaration as suggested by Stephen
    • Implement internalResolveBinding() for ImplicitTypeDeclaration
  • Revert AbstractTypeDeclaration to how it was before ImplicitTypeDeclaration was added
  • Reincorporate Jörg's fix that makes typeName volatile
  • Delete AbstractUnnamedTypeDeclaration

How to test

See #2481, it should fix pasting snippets into the package explorer (it doesn't yet, it needs work, currently it throws an "Invalid identifier" exception).

Author checklist

@iloveeclipse
Copy link
Member

@jarthana , @stephan-herrmann : tomorrow is RC1 build. It would be very helpful to review (and merge?) this PR ASAP to avoid last minute fixes in RC2.

@srikanth-sankaran
Copy link
Contributor

@jarthana , @stephan-herrmann : tomorrow is RC1 build. It would be very helpful to review (and merge?) this PR ASAP to avoid last minute fixes in RC2.

I am told @jarthana is on leave till Monday. I will study this and share my comments. @mpalat has also offered to look into this - Thanks!

Copy link
Contributor

@stephan-herrmann stephan-herrmann left a comment

Choose a reason for hiding this comment

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

Basically looks good, the issue of calling preLazyInit(), postLazyInit() has priority among my remarks.

Let's wait for at least one more review.

@stephan-herrmann
Copy link
Contributor

See #2481, it should fix pasting snippets into the package explorer (it doesn't yet, it needs work, currently it throws an "Invalid identifier" exception).

That requires some reverts in JDT/UI, right? Should those be prepared already?

@iloveeclipse
Copy link
Member

That requires some reverts in JDT/UI, right? Should those be prepared already?

Not reverts but careful changes, as some logic changed too => eclipse-jdt/eclipse.jdt.ui#1420

@datho7561
Copy link
Contributor Author

Sorry I should have prepared a PR with the required changes to jdt.ui, my bad I'll have that up soon too

datho7561 added a commit to datho7561/eclipse.jdt.ui that referenced this pull request May 23, 2024
@iloveeclipse
Copy link
Member

Sorry I should have prepared a PR with the required changes to jdt.ui, my bad I'll have that up soon too

No problem, but you can review eclipse-jdt/eclipse.jdt.ui#1420

@datho7561
Copy link
Contributor Author

Looking into test failures

@datho7561
Copy link
Contributor Author

They are legitimate, working on a fix

@datho7561
Copy link
Contributor Author

I can't reproduce the failure (JavaSearchBugsTests2.testBug483650_0002) locally from within Eclipse. I'm going to squash to one commit and clean up the commit message.

@datho7561 datho7561 force-pushed the remove-abstract-unnamed-class-decl branch from 69c2f23 to 106a134 Compare May 23, 2024 18:54
@datho7561 datho7561 changed the title [WIP] Remove AbstractUnnamedTypeDeclaration Remove AbstractUnnamedTypeDeclaration May 23, 2024
@datho7561 datho7561 marked this pull request as ready for review May 23, 2024 18:54
- rework hierarchy so ImplicitTypeDeclaration depends directly on AbstractTypeDeclaration
  - use subclass of `SimpleName` "EmptyName" as the `typeName` for
    `ImplicitTypeDeclaration`,
    since `SimpleName` cannot have the empty string as the type name
- delete AbstractUnnamedTypeDeclaration
- revert AbstractTypeDeclaration to pre-ImplicitTypeDeclaration
  - reincorporate Jörg's fix in AbstractTypeDeclaration that makes `typeName` volatile

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the remove-abstract-unnamed-class-decl branch from 106a134 to 709b674 Compare May 23, 2024 21:19
Copy link
Contributor

@stephan-herrmann stephan-herrmann left a comment

Choose a reason for hiding this comment

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

If build succeeds, this is good to go from my p.o.v.
thanks

@iloveeclipse
Copy link
Member

I'm going to sleep now.

If you are going to merge this, please also merge eclipse-jdt/eclipse.jdt.ui#1420 at same time, even if it doesn't compile yet on jenkins, it compiles with this PR. Merging only this PR will break the SDK build.

@mpalat
Copy link
Contributor

mpalat commented May 24, 2024

If build succeeds, this is good to go from my p.o.v. thanks

@stephan-herrmann @datho7561 Since this is still a preview feature, an "@noreference" to the ImplicitTypeDeclaration is the norm we follow - until it becomes a standard feature. So I would request to add that in the JavaDoc before merging

@iloveeclipse
Copy link
Member

@mpalat : I think David can add annotation for the next build. I would like to have all ready for RC1 now, so we can run all tests etc.

@iloveeclipse iloveeclipse merged commit 70560b9 into eclipse-jdt:master May 24, 2024
8 of 9 checks passed
@mpalat
Copy link
Contributor

mpalat commented May 24, 2024

@mpalat : I think David can add annotation for the next build. I would like to have all ready for RC1 now, so we can run all tests etc.

@iloveeclipse - yes, that's sounds fine.

@datho7561 datho7561 deleted the remove-abstract-unnamed-class-decl branch May 27, 2024 12:52
datho7561 added a commit to datho7561/eclipse.jdt.core that referenced this pull request May 27, 2024
See eclipse-jdt#2483 (comment)

Signed-off-by: David Thompson <davthomp@redhat.com>
jarthana pushed a commit that referenced this pull request May 28, 2024
See #2483 (comment)

Signed-off-by: David Thompson <davthomp@redhat.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.

None yet

5 participants