Allow code actions to be created if no Diagnostics is provided #1516

[vrubezhny]

No description provided.

[angelozerr]

Perhaps Im wrong but my first feeling with noError diagnostic is that the API is strange.

I wonder if we should update the API of ICodeActionPartiipant to introduce a new method like doCodeActionswithoutDiagnostic or à better name.

It would be nice too to have a test which implement a code action without diagnostic by registering a mock participant.

[vrubezhny]

Perhaps Im wrong but my first feeling with noError diagnostic is that the API is strange.

I wonder if we should update the API of ICodeActionPartiipant to introduce a new method like doCodeActionswithoutDiagnostic or à better name.

I also have such feeling...

When I add this fake "NoError" diagnostic it makes obligatory to CodeAction participants to know and check for that special diagnostics code in order to work correctly and not to produce any CodeAction duplication.

I thought in adding this diagnostics ONLY in case of empty diagnostics list, but this for sure makes CodeAction participants that do not require any dedicated error code to produce duplicated CodeAction-s in some cases without any possibility to control it.

Having an API method like doCodeActionswithoutDiagnostic or doCodeActionsIncondicional would be a better solution.

[mickaelistria]

I agree the NO_ERROR is not so good, but I don't think a new API would be necessary nor desirable as it would break compatibility. The ICodeActionRequest does include the diagnostic, what about always just sending another request with this diagnostic being null, after sending requests that reference particular diagnostics?

[vrubezhny]

@mickaelistria Imagine a situation when you have a piece of code that contains one or more diagnostic (like error + warnings) and you invoke a code action request...
Registered Code Action Participants are been acquired to return code actions. Those which are related to the a certain diagnostic code will return their code actions, while those which aren't related to any diagnostic code will not return any code actions because diagnostics list is not null nor empty in this situation.

If we make these "null-diagnostic-related" code action participants to not require"null or empty" diagnostics - then they will create their code action multiple times (as much as number of diagnostic elements we have set for the request)

To avoid such skipping or duplication we need such an fake "NoError" (or what ever we name it" diagnostic or adding a new API method for code actions that are independent of diagnostics provided.

[mickaelistria]

I may be missing something, but I see ICodeActionParticipant.doCodeAction(...) takes only 1 ICodeActionRequest, and 1 ICodeActionRequest can refer at most 1 diagnostic. So in case of multiple diagnostics, several ICodeActionRequests are created and processed by the participant. Participants could already process a ICodeActionRequest that returns null for getDiagnostic. So I don't see the need for a new extension API here, this can all be done internally.

[vrubezhny]

@mickaelistria I have changed the PR according to your suggestion. From now the Code Action participants are obliged to check whether the correct diagnostic is provided for diagnostic-dependent participants and check for null-diagnostic value for the ones that should return code actions independently of presence and codes of diagnostics

[angelozerr]

Code Action participants are obliged to check whether the correct diagnostic is provided for diagnostic-dependent

This kind of think that I would like to avoid. I like the idea with adding à New method because when you implement it you are sûre that diagnostic is null and the code is clean. When you implement doCodeAction you work with diagnostic, when you implement doCodeActionsIncondicional you work without diagnostic.

We can add this method in participant and implement as default

[mickaelistria]

This kind of think that I would like to avoid.

Why? It's really simple and efficient, and very close to LSP specification; so no-one would be surprised with such pattern.

[angelozerr]

Why? It's really simple and efficient, and very close to LSP specification; so no-one would be surprised with such pattern.

Because it requires for each participant to check diagnostic is not null. I'm surprised that there are not an NPE at

https://github.com/eclipse/lemminx/blob/7f668204d03ea82235efc664d13f462a38bf7fa3/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/participants/codeactions/CloseTagCodeAction.java#L41 (perhaps it is logged)

More it means that you will call twice doCodeActions. IMHO I think it is better to separate the code actions:

• code action for a given diagnostic in a doCodeActions method
• code action for none diagnostic in a doCodeActionsIncondicional method

The dispatch of those code actions are cleaned. If youhave just one doCodeActions, it will require to check if the diagnostic is null.

More if we have a new method doCodeActionsIncondicional , we can find with the IDE all code actions which implement this method. It is an easy and fats mean to retrieve all refactoring code actions for instance.

[mickaelistria]

Because it requires for each participant to check diagnostic is not null.

I suspect many already do. Do you have a list of extensions of lemminx? If it requires to just tweak a few ones to add a null-check in order to be more consistent with LSP and avoid new APIs, it can be worth it.

More it means that you will call twice doCodeActions.

We already call it as many times as there are diagnostics on that position. This can be multiple times.

[angelozerr]

Do you have a list of extensions of lemminx?

See https://github.com/eclipse/lemminx/blob/main/docs/LemMinX-Extensions.md#lemminx-extensions

in order to be more consistent with LSP and avoid new APIs, it can be worth it.

Adding just a new method doCodeActionsIncondicional in ICodeActionParticipant implemented by default will not break the current API.

I like the idea to have doCodeActionsIncondicional because it provides some convention of the lemminx extension:

• if you wish to manage code action for diagnostic, let's implement doCodeActions
• if you wish to manage code action like refactoring which doesn't need diagnostic, let's implement doCodeActionsIncondicional

It means that when you contribute to a lemminx extsenion of a given project that you don'tknow, you need just to seach implementation of doCodeActionsIncondicional to implement new code action refactoring. You use your IDE search to retrieve all existing code action of refactoring.

[vrubezhny]

Because it requires for each participant to check diagnostic is not null. I'm surprised that there are not an NPE at

https://github.com/eclipse/lemminx/blob/7f668204d03ea82235efc664d13f462a38bf7fa3/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/participants/codeactions/CloseTagCodeAction.java#L41

(perhaps it is logged)

Haven't seen any NPEs loged regarding this Code Action.
This code action participant isn't invoked directly from XMLCodeActions, it's invoked from ContentModelCodeActionParticipant.doCodeActions() which performs all the required verification of request.getDiagnostics() result, so if request.getDiagnostics() == null this participant will never be used.

[vrubezhny]

@mickaelistria @angelozerr Would you mind if I merge this PR - I need it to continue working on eclipse-lemminx/lemminx-maven#383 to provide Inline code actions as well as to continue on other parts of the issue.

[mickaelistria]

I defer the final say to @angelozerr

[angelozerr]

Me I would prefer having doCodeActionsIncondicional to provide some rules to implement code actions without diagnostic and find easily in a project those code actions kind.

After that if @mickaelistria and @vrubezhny you are not happy with this API, let's give up my suggestion.

[vrubezhny]

The PR is changed so now the org.eclipse.lemminx.services.extensions.codeaction.ICodeActionParticipant.doCodeActionUnconditional(ICodeActionRequest, List<CodeAction>, CancelChecker) (default) method is introduced and is to be invoked for the cases where no any Dignostic is required when creating Code Actions . JUnit test is modified accordingly.

The order of Code Action creation is saved to create at first - the code actions that refer to a certain diagnostics, and at second - the rest of code actions, which means that Error/Warnings related Code Actions would be placed above the rest of Code Actions created without any diagnostics (If the Code Action list is not going to be reordered after the creation)

@angelozerr Please review.

[vrubezhny]

Actually I would suggest:

• Make ICodeActionParticipant.doCodeAction default as well in order to make it not necessary to add its default implementation in case it's not needed
• Bump version +0.1 and add @since 0.26 annotation to ICodeActionParticipant.doCodeActionUnconditional` method,

[angelozerr]

Thanks so mu h @vrubezhny !