Skip to content

Fixing rename for permitted subclasses.#7977

Merged
lahodaj merged 1 commit intoapache:masterfrom
lahodaj:sealed-type-rename
Apr 4, 2025
Merged

Fixing rename for permitted subclasses.#7977
lahodaj merged 1 commit intoapache:masterfrom
lahodaj:sealed-type-rename

Conversation

@lahodaj
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
mbien
mbien approved these changes Jan 16, 2025
View reviewed changes
Copy link
Copy Markdown
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.

mbien
mbien requested changes Jan 16, 2025
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
@mbien
Copy link
Copy Markdown
Member

mbien commented Jan 23, 2025

@lahodaj Unfortunately I didn't get to taking a look why tests are failing and tomorrow is freeze - probably better to move this to NB 26?

@lahodaj lahodaj modified the milestones: NB25, NB26 Jan 23, 2025
@lahodaj
Copy link
Copy Markdown
Contributor Author

lahodaj commented Jan 23, 2025

@lahodaj Unfortunately I didn't get to taking a look why tests are failing and tomorrow is freeze - probably better to move this to NB 26?

I apparently broke something in the CasualDiff. I hope I'll cycle back to this sometime soon.

@lahodaj lahodaj force-pushed the sealed-type-rename branch from d87c35c to 1fad9d2 Compare February 4, 2025 16:46
@lahodaj
Copy link
Copy Markdown
Contributor Author

lahodaj commented Feb 10, 2025

I think this is ready for review. I've updated, squashed, went through the changes.

The "old" TreeMaker.Class/Interface methods will create the AST trees without any permitted subclasses, which could lead to a loss of those permitted subclasses. I don't think there's a reasonable way around that in the generator - the generator needs to follow what is in the new trees. So these old methods are now deprecated and there's an AP that prevents use of the deprecated method inside NB sources.

mbien
mbien approved these changes Feb 10, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mbien mbien left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good to me. Ran a quick manual test and everything worked.

Deprecating the method was a good idea - wanted to suggest that too so that it isn't used by accident again.

Comment thread java/lib.nbjavac/src/org/netbeans/lib/nbjavac/CheckProcessor.java Outdated
@lahodaj
Copy link
Copy Markdown
Contributor Author

lahodaj commented Apr 1, 2025

Any issue with merging this sometime soon?

@matthiasblaesing
Copy link
Copy Markdown
Contributor

matthiasblaesing commented Apr 1, 2025

Given, the broad changes, I think it would be good to get this in early, so I suggest to merge now. That way people running nightlies will test and report problems early.

@mbien
Copy link
Copy Markdown
Member

mbien commented Apr 1, 2025

i reviewed it and tested it - have no concerns regarding integration.

@lahodaj lahodaj force-pushed the sealed-type-rename branch from 1fad9d2 to 71f656d Compare April 2, 2025 07:54
mbien
mbien approved these changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
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.

still approved ;)

@lahodaj lahodaj merged commit b203b72 into apache:master Apr 4, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbien mbien mbien approved these changes

@sdedic sdedic Awaiting requested review from sdedic

Assignees

No one assigned

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

Milestone

NB26

Development

Successfully merging this pull request may close these issues.

3 participants

@lahodaj @mbien @matthiasblaesing