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

Fixing rename for permitted subclasses. #7977

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 21, 2024

Having something like:

sealed interface I permits Subtype {}
final class Subtype implements I {}

Renaming Subtype will not rename the identifier in the permits clause. This is because:

a) the write model (CausalDiff) does not support permits
b) the indexing does not include the permitted subclasses.

This PR is trying to fix both.

@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests labels Nov 21, 2024
@lahodaj lahodaj added this to the NB25 milestone Nov 21, 2024
@lahodaj lahodaj requested review from mbien and sdedic November 21, 2024 08:49
@lahodaj lahodaj marked this pull request as draft November 21, 2024 09:55
@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 21, 2024

(Found a problem in the patch.)

@lahodaj lahodaj marked this pull request as ready for review November 22, 2024 06:55
@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 22, 2024

Should be fixed now, and ready for review. Please let me know what you think!

@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jan 16, 2025
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

this looks good as far as I can tell (thanks for the comments in CasualDiff).

merge conflict in apichanges.xml needs to be resolved + rebasing would be good to have a fresh CI run before integration.

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

I found a regression right after review :(

if you rename the interface, it will remove the permits section, this does not happen in NB 24

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 17, 2025

FWIW: the pre-existing methods now are a bit problematic, as if some code takes a ClassTree, and creates a new one from it using the old TreeMaker.Class/Interface method with permitted subclasses, the permitted subclasses will be lost. I don't see a good way around that.

So, I've tried to go through the whole NetBeans (production code), and fix what I could. Will push that shortly. I think I still need to make at least one pass through it, maybe add some tests, and maybe add some automated way to prevent use of the pre-existing methods. Will try to look at that as I can.

@mbien mbien self-requested a review January 17, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants