Skip to content

[WIP] Fix variable declaration in JFacePlugin to resolve CI test failures#595

Merged
carstenartur merged 2 commits into
copilot/cleanup-subprogressmonitor-usagefrom
copilot/fix-jface-plugin-declaration
Feb 4, 2026
Merged

[WIP] Fix variable declaration in JFacePlugin to resolve CI test failures#595
carstenartur merged 2 commits into
copilot/cleanup-subprogressmonitor-usagefrom
copilot/fix-jface-plugin-declaration

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 4, 2026

Fix JFace Cleanup Test Failures - Plan

  • Add missing imports for VariableDeclarationStatement and VariableDeclarationFragment to JFacePlugin.java
  • Replace SingleVariableDeclaration with VariableDeclarationFragment + VariableDeclarationStatement (lines 414-432)
  • Change replacement target from minv to minv.getParent() to replace the entire ExpressionStatement
  • Run tests to verify the fix resolves all 14 test failures
  • Validate the generated code matches expected output format
Original prompt

Problem

The CI tests in PR #568 are failing with 14 test failures in sandbox_jface_cleanup_test. The root cause is that JFacePlugin.java uses SingleVariableDeclaration instead of VariableDeclarationStatement when replacing monitor.beginTask() with SubMonitor subMonitor = SubMonitor.convert(...).

Root Cause

In sandbox_jface_cleanup/src/org/sandbox/jdt/internal/corext/fix/helper/JFacePlugin.java, lines 414-432, the code creates a SingleVariableDeclaration:

SingleVariableDeclaration newVariableDeclarationStatement = ast.newSingleVariableDeclaration();
newVariableDeclarationStatement.setName(ast.newSimpleName(identifier));
newVariableDeclarationStatement.setType(ast.newSimpleType(addImport(SubMonitor.class.getCanonicalName(), cuRewrite, ast)));
// ...
newVariableDeclarationStatement.setInitializer(staticCall);
ASTNodes.replaceButKeepComment(rewrite, minv, newVariableDeclarationStatement, group);

However, SingleVariableDeclaration is only valid inside method parameter lists or catch clauses - it cannot stand alone as a statement. For a local variable declaration statement like:

SubMonitor subMonitor = SubMonitor.convert(monitor, "Task", 100);

You need to use VariableDeclarationStatement with a VariableDeclarationFragment.

Required Fix

Change the code to use VariableDeclarationStatement instead:

// Create the variable declaration fragment (name + initializer)
VariableDeclarationFragment fragment = ast.newVariableDeclarationFragment();
fragment.setName(ast.newSimpleName(identifier));
fragment.setInitializer(staticCall);

// Create the variable declaration statement (type + fragment)
VariableDeclarationStatement varDeclStmt = ast.newVariableDeclarationStatement(fragment);
varDeclStmt.setType(ast.newSimpleType(addImport(SubMonitor.class.getCanonicalName(), cuRewrite, ast)));

// Replace the entire ExpressionStatement (parent of beginTask), not just the MethodInvocation
ASTNodes.replaceButKeepComment(rewrite, minv.getParent(), varDeclStmt, group);

Key changes:

  1. Use VariableDeclarationFragment + VariableDeclarationStatement instead of SingleVariableDeclaration
  2. Replace minv.getParent() (the ExpressionStatement) instead of just minv (the MethodInvocation)

Files to Modify

  • sandbox_jface_cleanup/src/org/sandbox/jdt/internal/corext/fix/helper/JFacePlugin.java

Expected Result

After this fix, the transformation should produce valid Java code that matches the test expectations, and all 14 failing tests should pass.

This pull request was created from Copilot chat.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…VariableDeclaration

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
@carstenartur carstenartur marked this pull request as ready for review February 4, 2026 04:46
@carstenartur carstenartur merged commit aacb76e into copilot/cleanup-subprogressmonitor-usage Feb 4, 2026
@carstenartur carstenartur deleted the copilot/fix-jface-plugin-declaration branch February 4, 2026 05:39
carstenartur added a commit that referenced this pull request Feb 5, 2026
)

* Implement SubProgressMonitor enhancements: standalone conversion, flag mapping, and documentation updates

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix standalone SubProgressMonitor detection and transformation

- Refactored find() method to collect all SubProgressMonitor instances first
- Added post-processing to identify standalone monitors (not associated with beginTask)
- Create separate operations for standalone monitors
- Removed debug statements
- This should fix the CI test failure for StandaloneSubProgressMonitor test case

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Address code review feedback: document limitations and add test coverage

- Added test case for standalone SubProgressMonitor with flags (shows flags are dropped)
- Updated README.md to document flag dropping behavior for standalone cases
- Added documentation about bitwise OR limitation in flag mapping
- Improved inline comments to clarify flag handling
- Fixed duplicate comment in mapSubProgressMonitorFlags method

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Add done() call removal feature (partial implementation)

- Added doneInvocations tracking to MonitorHolder
- Implemented done() call visitor in find() method
- Added removal logic in rewrite() method
- Added DoneCallRemoval and MultipleDoneCalls test cases
- Updated README: added feature to Key Features, added example, removed limitation
- Tests show feature is detected but removal not yet working correctly

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix done() removal - iterate through all holders to find matches

The previous implementation only checked the last holder, which missed done() calls.
Now searches through all MonitorHolders to properly associate done() calls with their
corresponding beginTask transformations.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Temporarily disable done() removal to fix test failures

The done() removal feature is causing 14 out of 16 tests to fail. Disabling it
temporarily to restore functionality while investigating the root cause of the
failures. The infrastructure (doneInvocations tracking) remains in place for
future re-enablement once the issue is resolved.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix standalone SubProgressMonitor conversion and done() removal in JFacePlugin (#590)

* [WIP] Fix SubProgressMonitor to SubMonitor transformation regression (#593)

* Plan: Fix JFace cleanup transformation not being applied

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix JFace cleanup: remove scope functions from SubProgressMonitor and done() visitors

The ClassInstanceCreation and done() method visitors had scope functions that prevented
them from accessing the holders created by the beginTask visitor. When multiple visitors
use different scope functions, they get separate holder instances and can't share data.

Removed the scope functions from:
1. callClassInstanceCreationVisitor for SubProgressMonitor detection
2. callMethodInvocationVisitor for done() call detection

This allows these visitors to access the global holder populated by beginTask visitor,
enabling proper SubProgressMonitor transformation.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* [WIP] Fix variable declaration in JFacePlugin to resolve CI test failures (#595)

* Restore scope functions to enable Block-level grouping of progress monitor elements

The scope functions `s -> ASTNodes.getTypedAncestor(s, Block.class)` are intentional
and necessary. They group related progress monitor elements (beginTask, SubProgressMonitor,
done) that belong together within the same Block scope.

Restored scope functions to:
- callClassInstanceCreationVisitor for SubProgressMonitor detection
- callMethodInvocationVisitor for done() call detection

This ensures all three visitors (beginTask, SubProgressMonitor, done) operate on the
same Block-scoped holder, allowing proper matching of related monitor operations.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix holder access in JFace cleanup and document pattern detection issue (#596)

* Remove standalone SubProgressMonitor and done() removal from JFace cleanup (#597)

* [WIP] Update expected output for JFace cleanup tests (#598)

* Fix JFace cleanup idempotence tests - align given/expected for already-converted code (#599)

* fix formatting

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
Co-authored-by: Carsten Hammer <carsten.hammer@t-online.de>
carstenartur added a commit that referenced this pull request Feb 5, 2026
)

* Implement SubProgressMonitor enhancements: standalone conversion, flag mapping, and documentation updates

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix standalone SubProgressMonitor detection and transformation

- Refactored find() method to collect all SubProgressMonitor instances first
- Added post-processing to identify standalone monitors (not associated with beginTask)
- Create separate operations for standalone monitors
- Removed debug statements
- This should fix the CI test failure for StandaloneSubProgressMonitor test case

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Address code review feedback: document limitations and add test coverage

- Added test case for standalone SubProgressMonitor with flags (shows flags are dropped)
- Updated README.md to document flag dropping behavior for standalone cases
- Added documentation about bitwise OR limitation in flag mapping
- Improved inline comments to clarify flag handling
- Fixed duplicate comment in mapSubProgressMonitorFlags method

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Add done() call removal feature (partial implementation)

- Added doneInvocations tracking to MonitorHolder
- Implemented done() call visitor in find() method
- Added removal logic in rewrite() method
- Added DoneCallRemoval and MultipleDoneCalls test cases
- Updated README: added feature to Key Features, added example, removed limitation
- Tests show feature is detected but removal not yet working correctly

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix done() removal - iterate through all holders to find matches

The previous implementation only checked the last holder, which missed done() calls.
Now searches through all MonitorHolders to properly associate done() calls with their
corresponding beginTask transformations.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Temporarily disable done() removal to fix test failures

The done() removal feature is causing 14 out of 16 tests to fail. Disabling it
temporarily to restore functionality while investigating the root cause of the
failures. The infrastructure (doneInvocations tracking) remains in place for
future re-enablement once the issue is resolved.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix standalone SubProgressMonitor conversion and done() removal in JFacePlugin (#590)

* [WIP] Fix SubProgressMonitor to SubMonitor transformation regression (#593)

* Plan: Fix JFace cleanup transformation not being applied

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix JFace cleanup: remove scope functions from SubProgressMonitor and done() visitors

The ClassInstanceCreation and done() method visitors had scope functions that prevented
them from accessing the holders created by the beginTask visitor. When multiple visitors
use different scope functions, they get separate holder instances and can't share data.

Removed the scope functions from:
1. callClassInstanceCreationVisitor for SubProgressMonitor detection
2. callMethodInvocationVisitor for done() call detection

This allows these visitors to access the global holder populated by beginTask visitor,
enabling proper SubProgressMonitor transformation.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* [WIP] Fix variable declaration in JFacePlugin to resolve CI test failures (#595)

* Restore scope functions to enable Block-level grouping of progress monitor elements

The scope functions `s -> ASTNodes.getTypedAncestor(s, Block.class)` are intentional
and necessary. They group related progress monitor elements (beginTask, SubProgressMonitor,
done) that belong together within the same Block scope.

Restored scope functions to:
- callClassInstanceCreationVisitor for SubProgressMonitor detection
- callMethodInvocationVisitor for done() call detection

This ensures all three visitors (beginTask, SubProgressMonitor, done) operate on the
same Block-scoped holder, allowing proper matching of related monitor operations.

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

* Fix holder access in JFace cleanup and document pattern detection issue (#596)

* Remove standalone SubProgressMonitor and done() removal from JFace cleanup (#597)

* [WIP] Update expected output for JFace cleanup tests (#598)

* Fix JFace cleanup idempotence tests - align given/expected for already-converted code (#599)

* Initial plan

* Fix formatting in Java8CleanUpTest expected outputs - revert to match actual cleanup output

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
Co-authored-by: Carsten Hammer <carsten.hammer@t-online.de>
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.

2 participants