Skip to content

Propagate refactoring problems to LSP client#8968

Merged
lahodaj merged 1 commit intoapache:masterfrom
shivam71:lsp_refactoring_warnings_fix
Nov 10, 2025
Merged

Propagate refactoring problems to LSP client#8968
lahodaj merged 1 commit intoapache:masterfrom
shivam71:lsp_refactoring_warnings_fix

Conversation

@shivam71
Copy link
Contributor

@shivam71 shivam71 commented Oct 29, 2025

Issue

  • LSP Server didn't inform LSP Client about refactoring changes that can lead to broken code.
  • For example
    • Trying to move a non static data member to another class often breaks code.
    • Changing method parameters by misspelling type of the parameter like 'string'
  • Such problems are found by Refactoring Plugins.
  • Prior to the current change such problems (if not deemed fatal) were not propagated to the LSP Client.
  • In Netbeans IDE such warnings are shown and confirmation is asked for proceedings with the warnings.
  • With the current change such non fatal problems would be shown as warning message to LSP Client.
  • Further LSP Server would ask for confirmation to proceed despite the warnings.

Example: Warnings in Netbeans

Netbeans_Refactoring_Warnings

After this change same warnings propagated to LSP Client (VSCode).

VSCode_Warnings
Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

  1. Was this PR correctly labeled, did the right tests run? When did they run?
  2. Is this PR squashed?
  3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
  4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

@mbien mbien 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 Oct 30, 2025
@apache apache locked and limited conversation to collaborators Oct 30, 2025
@apache apache unlocked this conversation Oct 30, 2025
@lahodaj
Copy link
Contributor

lahodaj commented Nov 3, 2025

Overall +1 on reporting the warnings to the user.

But, please:

  • there's a lot of code duplication in each refactoring class that handles the warnings, etc. Is there a chance that would be factored out into CodeRefactoring? Maybe it would also allow to reduce churn in that class?
  • in java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/refactoring/MoveRefactoring.java, it seems there's a huge amount of changes, which are probably mostly formatting(?) - I would suggest to undo that, it makes it very hard to review that part of the PR.

@shivam71
Copy link
Contributor Author

shivam71 commented Nov 4, 2025

Sorry for the MoveRefactoring formatting related diffs , will revert those .
Okay I go over the code and try to refactor to reduce duplication .

@shivam71 shivam71 force-pushed the lsp_refactoring_warnings_fix branch 2 times, most recently from 13f5b2a to da0c904 Compare November 4, 2025 10:35
@shivam71
Copy link
Contributor Author

shivam71 commented Nov 4, 2025

@lahodaj I have tried to refactor a bit , please review again . Thank you !

@lahodaj lahodaj mentioned this pull request Nov 7, 2025
Merged
@lahodaj
Copy link
Contributor

lahodaj commented Nov 7, 2025

Functionally seems OK to me. There are some formatting/cleanup that I would suggest to do, shown here:
shivam71#1
(reducing access, removing stuff no longer needed, keeping spaces consistent with the file).

If I was writing this, I would probably inline CodeRefactoring.warningsMessageParams and CodeRefactoring.prepare into their callers, but I don't think that is strictly necessary.

@shivam71 shivam71 force-pushed the lsp_refactoring_warnings_fix branch from 7e36aa1 to aab5a8c Compare November 7, 2025 15:05
@shivam71 @lahodaj
Propagate refactoring problems to lsp client and ask for confirmation…
ebc27e1 
… to proceed with warnings.

Co-authored-by: Jan Lahoda <jan.lahoda@oracle.com>
@shivam71 shivam71 force-pushed the lsp_refactoring_warnings_fix branch from aab5a8c to ebc27e1 Compare November 7, 2025 15:07
@mbien mbien added this to the NB29 milestone Nov 7, 2025
lahodaj
lahodaj approved these changes Nov 7, 2025
View reviewed changes
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

I'll integrate on Monday, unless there are objections.

@lahodaj lahodaj merged commit 6401e56 into apache:master Nov 10, 2025
35 checks passed
This was referenced Nov 11, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lahodaj lahodaj lahodaj approved these changes

Assignees

No one assigned

Labels

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

NB29

Development

Successfully merging this pull request may close these issues.

3 participants

@shivam71 @lahodaj @mbien