-
Notifications
You must be signed in to change notification settings - Fork 882
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
base: master
Are you sure you want to change the base?
Conversation
(Found a problem in the patch.) |
a77b372
to
d14199a
Compare
Should be fixed now, and ready for review. Please let me know what you think! |
There was a problem hiding this 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.
There was a problem hiding this 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
FWIW: the pre-existing methods now are a bit problematic, as if some code takes a 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. |
@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 |
d87c35c
to
1fad9d2
Compare
I think this is ready for review. I've updated, squashed, went through the changes. The "old" |
There was a problem hiding this comment.
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.
((QualifiedNameable) el.getEnclosingElement()).getQualifiedName().contentEquals("org.netbeans.api.java.source.TreeMaker") && | ||
(el.getSimpleName().contentEquals("Class") || el.getSimpleName().contentEquals("Interface")) && | ||
processingEnv.getElementUtils().isDeprecated(el)) { | ||
trees.printMessage(Diagnostic.Kind.ERROR, "Use of the deprecated TreeMaker.Class/Interface method is not permitted inside the NetBeans sources.", node, selectPath.getCompilationUnit()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is interesting. Will this run while nb-javac is used within the NB editor too or only during build?
I suppose this is picked up when -proc:full
is set + module dependency to nb-javac lib is declared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for belated reply.
(AFAIK) This will run every time this module is on the appropriate path (and the AP is enabled). I.e. it will run in the editor when the NetBeans modules that depend on this are open (like java.source.base
). It will not (should not) run for ordinary unrelated user code.
Any issue with merging this sometime soon? |
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. |
Having something like:
Renaming
Subtype
will not rename the identifier in thepermits
clause. This is because:a) the write model (
CausalDiff
) does not supportpermits
b) the indexing does not include the permitted subclasses.
This PR is trying to fix both.