Jakarta EE: Inconsistent behaviour for @Dependent quick fix.

[mrglavas]

While reviewing the Jakarta EE quick fixes, I noticed a difference in behaviour in how the "Replace current scope with @Dependent" quick fix is applied when resolving a diagnostic on a field.

Current LT for IntelliJ behaviour:
Applying "Replace current scope with @Dependent" to a field adds the annotation to the field.

LT for VS Code 24.0.3 behaviour:
Applying "Replace current scope with @Dependent" to a field replaces the scope annotation on the class.

After applying the quick fix in VS Code it continues to report a diagnostic which cannot be resolved by the quick fix. We should investigate which of these tools (IntelliJ vs. VS Code) has the correct behaviour, assuming either of them are correct.

[scottkurz]

Seems like the IntelliJ behavior is wrong. The LT in VSCode and Eclipse both will update the class-level annotation.

I haven't looked at the whole history or discussion in LSP4Jakarta and am not a CDI expert, but it seems unlikely that the field-level annotation was intended to be updated. Though the field-level annotation can hold a scope annotation, that's more for things like Producer use cases so not sure it applies here.

I assume the logic of linking the field which has the problem/diagnostic to the class which needs the annotation replaced is somewhere in the org.eclipse.lsp4jakarta.jdt.internal.cdi;ManagedBeanQuickFix but I'm not fluent enough to easily make sense of that.

Anyway hopefully that was a bit of help to mention.

[mrglavas]

Seems like the IntelliJ behavior is wrong. The LT in VSCode and Eclipse both will update the class-level annotation.

I haven't looked at the whole history or discussion in LSP4Jakarta and am not a CDI expert, but it seems unlikely that the field-level annotation was intended to be updated. Though the field-level annotation can hold a scope annotation, that's more for things like Producer use cases so not sure it applies here.

I assume the logic of linking the field which has the problem/diagnostic to the class which needs the annotation replaced is somewhere in the org.eclipse.lsp4jakarta.jdt.internal.cdi;ManagedBeanQuickFix but I'm not fluent enough to easily make sense of that.

Anyway hopefully that was a bit of help to mention.

@scottkurz Not sure if you caught my observation but updating the class-level annotation does not resolve the diagnostic, so in VSCode when the user applies the quick fix they still get an error. If the quick fix is correct in VSCode then it would seem that the diagnostic is wrong. If I manually add @Dependent to the field then VSCode stops reporting the error.

[TrevCraw]

For more context, the changes in LT IntelliJ were based on the following issue and PR in LSP4Jakarta: eclipse/lsp4jakarta#444 and eclipse/lsp4jakarta#506 (these changes are NOT included in the current 0.2.1 release of LSP4Jakarta)

[scottkurz]

@scottkurz Not sure if you caught my observation but updating the class-level annotation does not resolve the diagnostic, so in VSCode when the user applies the quick fix they still get an error. If the quick fix is correct in VSCode then it would seem that the diagnostic is wrong. If I manually add @dependent to the field then VSCode stops reporting the error.

@mrglavas no I didn't notice this before. I just tried in Eclipse and I see the same thing.

So, I think it's clear that what should happen is that the class-level annotation gets replaced with @Dependent. As far as what went wrong, well, you and @TrevCraw might know more than me.

I can imagine the opportunity for confusion since we have a field that accepts a certain annotation, but when we look at this field we want to change the annotation elsewhere in the scope (the class). I'm just recognizing it seems open for possibilities for error but not really claiming to help get us further there.

Seems likely if both VSCode and Eclipse envs show the same error then something could be wrong in LSP4Jakarta (stating the obvious).

[dessina-devasia]

The behaviour of @Dependent quickfix in different IDEs are attached below:

IntelliJ

intellij_dependent_quickfix_issue.mov

Eclipse

eclipse_dependent_quickfix_issue.mov

VSCode

vscode_dependent_quickfix_issue.mov

[TrevCraw]

Hi @dessina-devasia , for the IntelliJ example, what is the diagnostic on the public int a field before and after the quick fix adding @Dependent is added to the ManagedBean class?

[scottkurz]

Looking at the lsp4jakarta v0.2.1 code in Eclipse, this seems wrong:

This code shouldn't be checking if the field scope is dependent in order to not produce the diagnostic.... it should be looking at the scope of the managed bean itself:

                if (isManagedBean && Flags.isPublic(fieldFlags) && !Flags.isStatic(fieldFlags)
                    && (fieldScopes.size() != 1 || !fieldScopes.get(0).equals(Constants.DEPENDENT_FQ_NAME))) {
                    Range range = PositionUtils.toNameRange(field, context.getUtils());
                    diagnostics.add(context.createDiagnostic(uri,
                                                             Messages.getMessage("ManagedBeanWithNonStaticPublicField"), range,
                                                             Constants.DIAGNOSTIC_SOURCE, null,
                                                             ErrorCode.InvalidManagedBeanWithNonStaticPublicField, DiagnosticSeverity.Error));
                }

[scottkurz]

Re: #784 (comment) the problem I pointed out seems to be fixed in eclipse/lsp4jakarta#506. I'm confused, maybe I'm not helping.

[dessina-devasia]

Hi @TrevCraw,
Here is the video showing the he diagnostic on the public int a field before and after the quick fix adding @Dependent from ManagedBean class

Screen.Recording.2024-10-28.at.3.58.36.PM.mov

[TrevCraw]

Hi @dessina-devasia , after the discussion with @scottkurz and others today, I think these are the next steps:

1. For the scenario that has been outlined in this issue, align the behaviour of LTI with the current behaviour of the LSP4Jakarta main branch (https://github.com/eclipse/lsp4jakarta). The current behaviour for this scenario was added by this PR: Update Managed Bean diagnostic when using non-static public fields eclipse/lsp4jakarta#506.
2. @turkeylurkey is going to look into the behaviour introduced to LSP4Jakarta by this section of the PR - https://github.com/eclipse/lsp4jakarta/pull/506/files#diff-499e942372a6d850d8e4aee1ef38dc77d7b1d4ac33c0ff51b54bb49af5f1af59R100-R111 - to determine if this behaviour is correct or not.

[turkeylurkey]

Testing on the latest code compiled in Eclipse I found that the quick fix resolved both diagnostics in the test case shown above (which is not the latest in the test suite). The bug in IntelliJ is that the quick fix is applied to the field whereas the fix should be applied to the class even when you use the quick fix from the field.

[turkeylurkey]

The latest test for the diagnostic is now available in LSP4Jakarta. Please convert this test and use it in the appropriate diagnostic collector in IntelliJ. The other piece of work for this issue is to update the quick fix so that if the quick fix on the field is selected then it updates the annotation on the class rather than on the field.

https://github.com/eclipse/lsp4jakarta/blob/10887f94c2d29624452f8ad6f76608259af9fb8d/jakarta.jdt/org.eclipse.lsp4jakarta.jdt.core/src/main/java/org/eclipse/lsp4jakarta/jdt/internal/cdi/ManagedBeanDiagnosticsParticipant.java#L89

More info: eclipse/lsp4jakarta#534

[TrevCraw]

Hi @dessina-devasia , please align the behaviour in LTI with the behaviour @turkeylurkey has added to LSP4Jakarta.

[dessina-devasia]

Sure @TrevCraw