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

Reuse quick fixes from LocalCorrectionsBaseSubProcessor. #3368

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Jan 23, 2025

New quick fixes

Note that most of these diagnostics already existed, although there are some cases where the diagnostics might be new as well. What's new is the quick fixes for those diagnostics. Some of these new quick fixes do require a compiler setting to be set to warning/error in order for the problem reporting to diagnose them as they're ignored by default. Most are diagnosed by default though.

  • UnqualifiedFieldAccess -> addUnqualifiedFieldAccessProposal
    • org.eclipse.jdt.core.compiler.problem.unqualifiedFieldAccess=error
UnqualifiedFieldAccess.mp4
  • UnnecessaryInstanceof -> addUnnecessaryInstanceofProposal
    • org.eclipse.jdt.core.compiler.problem.unnecessaryTypeCheck=error
UnnecessaryInstanceof.mp4
  • IllegalQualifiedEnumConstantLabel -> addIllegalQualifiedEnumConstantLabelProposal
IllegalQualifiedEnumConstantLabel.mp4
  • UnnecessaryElse -> addUnnecessaryElseProposals
    • org.eclipse.jdt.core.compiler.problem.unnecessaryElse=error
UnnecessaryElse.mp4
  • SuperclassMustBeAClass -> addInterfaceExtendsClassProposals
SuperclassMustBeAClass.mp4
  • AssignmentHasNoEffect -> addAssignmentHasNoEffectProposals
AssignmentHasNoEffect.mp4
  • FallthroughCase, IllegalFallthroughToPattern -> addFallThroughProposals
    • org.eclipse.jdt.core.compiler.problem.fallthroughCase=error
FallthroughCase.mp4
  • UnsafeTypeConversion, RawTypeReference, UnsafeRawMethodInvocation -> addDeprecatedFieldsToMethodsProposals
UnsafeRawMethodInvocation.mp4
  • IllegalTotalPatternWithDefault -> addRemoveDefaultCaseProposal
IllegalTotalPatternWithDefault.mp4
  • SuperfluousSemicolon -> addSuperfluousSemicolonProposal
SuperfluousSemicolon.mp4
  • NonStaticFieldFromStaticInvocation, NonStaticFieldFromStaticInvocation -> addObjectReferenceProposal, addVariableReferenceProposal, addNewObjectProposal
NonStaticFieldFromStaticInvocation.mp4
  • AbstractServiceImplementation, ProviderMethodOrConstructorRequiredForServiceImpl, ServiceImplDefaultConstructorNotPublic -> addServiceProviderConstructorProposals
ProviderMethodOrConstructorRequiredForServiceImpl.mp4
  • Fix the JDT-LS QuickFixProcessor to better match upstream (by preventing fall-throughs, with 'break' where appropriate)
  • Preserve non-upstreamed behaviour of surrounding all selected statements when "Surrounding with try/catch"
  • Based on https://eclip.se/545163 the "Surround with try-catch" quick-fix is not valid for a 'throw' statement so we should adjust those testcases to use methods with throw declarations instead
  • Ensure the current compilation unit primary type is never used when it is null

TODO (prior to merging)

  • Basically all changes I noticed are things upstream introduced since we copied over code, and never got reintegrated, so switching to upstream's version should be a step in the right direction. There is only 1 exception I found. Wrong range of QuickFix: surround with try/multi-catch  #1189 . I need to verify whether upstream continues to fail to do this, and potentially upstream our one-line fix. I've preserved the behaviour on our end (hacked a custom ProblemLoation that return the context's covered node when a selection exists)
    • Update: Upstream JDT doesn't have this issue because the UI gets the full selection through SurroundWithTryCatchAction. Our implementation goes through LocalCorrectionsBaseSubProcessor.getUncaughtExceptionProposals(..) whether it's a quick-fix or just a plain refactoring so we need to make sure the selection is correct with the extra code.
  • AbstractServiceImplementation, ProviderMethodOrConstructorRequiredForServiceImpl, ServiceImplDefaultConstructorNotPublic -> addServiceProviderConstructorProposals appears to have some issues and isn't working correctly. Need to check if it's an upstream issue, in which case I think we can just add it and get it fixed there

@rgrunber rgrunber added this to the End February 2025 milestone Jan 23, 2025
@rgrunber rgrunber self-assigned this Jan 23, 2025
@rgrunber rgrunber force-pushed the upstream-local-corrections-subprocessor branch from c6c39dc to 8f76c30 Compare January 24, 2025 16:40
- New quick fixes :
  - UnqualifiedFieldAccess -> addUnqualifiedFieldAccessProposal
  - UnnecessaryInstanceof -> addUnnecessaryInstanceofProposal
  - IllegalQualifiedEnumConstantLabel -> addIllegalQualifiedEnumConstantLabelProposal
  - UnnecessaryElse -> addUnnecessaryElseProposals
  - SuperclassMustBeAClass -> addInterfaceExtendsClassProposals
  - AssignmentHasNoEffect -> addAssignmentHasNoEffectProposals
  - FallthroughCase, IllegalFallthroughToPattern
     -> addFallThroughProposals
  - UnsafeTypeConversion, RawTypeReference, UnsafeRawMethodInvocation
    -> addDeprecatedFieldsToMethodsProposals
  - IllegalTotalPatternWithDefault -> addRemoveDefaultCaseProposal
  - SuperfluousSemicolon -> addSuperfluousSemicolonProposal
  - AbstractServiceImplementation,
       ProviderMethodOrConstructorRequiredForServiceImpl,
       ServiceImplDefaultConstructorNotPublic
    -> addServiceProviderConstructorProposals
  - NonStaticFieldFromStaticInvocation, NonStaticFieldFromStaticInvocation
    -> addObjectReferenceProposal, addVariableReferenceProposal,
       addNewObjectProposal
- Fix the JDT-LS QuickFixProcessor to better match upstream (by
  preventing fall-throughs, with 'break' where appropriate)
- Preserve non-upstreamed behaviour of surrounding all selected
  statements when "Surrounding with try/catch"
- Based on https://eclip.se/545163 the "Surround with try-catch"
  quick-fix is not valid for a 'throw' statement so we should adjust
  those testcases to use methods with throw declarations instead
- Ensure the current compilation unit primary type is never used when it
  is null

Signed-off-by: Roland Grunberg <[email protected]>
@rgrunber rgrunber force-pushed the upstream-local-corrections-subprocessor branch from 8f76c30 to 86c0a6e Compare January 24, 2025 18:16
Copy link
Contributor Author

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

I intend to merge this soon. The test cases within JDT upstream should be sufficient.

@rgrunber rgrunber merged commit cc0946b into eclipse-jdtls:master Jan 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant