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

Remove dependency on xtext's @Pure and ToStringBuilder #494

Closed
lemmy opened this issue Jan 12, 2021 · 14 comments · Fixed by #529
Closed

Remove dependency on xtext's @Pure and ToStringBuilder #494

lemmy opened this issue Jan 12, 2021 · 14 comments · Fixed by #529
Milestone

Comments

@lemmy
Copy link

lemmy commented Jan 12, 2021

lsp4j.debug depends on xtext base at runtime, because the generated POJOs import org.eclipse.xtext.xbase.lib.Pure and org.eclipse.xtext.xbase.lib.util.ToStringBuilder. Neither @Pure nor ToStringBuilder are required by lsp4j.debug's functionality. Please consider removing the xtext dependency to shrink the set of lsp4j.debug's dependencies.

Related: https://twitter.com/lemmster/status/1346543929299140610

@jonahgraham
Copy link
Contributor

@spoenemann any thoughts on removing use of @Pure. It will be easier to do this dep removal of we do the same for LSP as DAP.

Thanks for your insights.

@spoenemann
Copy link
Contributor

I think this can be changed in the JsonRpcDataProcessor, that is what generates the getters and setters as well as toString.

@jonahgraham
Copy link
Contributor

@lemmy I think we have as much approval as we need for this. Would you like to provide a PR?

lemmy added a commit to lemmy/lsp4j that referenced this issue Jan 13, 2021
Remove dependency on xtext's @pure and ToStringBuilder

This commit removes @pure.  The next commit is going to remove
ToStringBuilder.

lsp4j.debug depends on xtext base at runtime, because the generated
POJOs import org.eclipse.xtext.xbase.lib.Pure and
org.eclipse.xtext.xbase.lib.util.ToStringBuilder. Neither @pure
nor ToStringBuilder are required by lsp4j.debug's functionality.
Removes the xtext dependency to shrink the set of lsp4j.debug's
dependencies.
@lemmy
Copy link
Author

lemmy commented Jan 13, 2021

I can do that. To confirm, though, we have an agreement to remove all @Pure annotations in POJOs, not just @Pure on toString methods?

@cdietrich
Copy link
Contributor

these other pures are generated by
org.eclipse.lsp4j.generator.JsonRpcDataProcessor.generateImplMembers(MutableClassDeclaration, JsonRpcDataTransformationContext)
delegating to
org.eclipse.xtend.lib.annotations.AccessorsProcessor.Util

@lemmy
Copy link
Author

lemmy commented Jan 13, 2021

Thanks, I guess adding @Pure is a side-effect of JsonRpcDataProcessor.generateImplMembers then. ;-)

@jonahgraham
Copy link
Contributor

I can do that. To confirm, though, we have an agreement to remove all @Pure annotations in POJOs, not just @Pure on toString methods?

Yes! removing all use of @Pure and ToStringBuilder in LSP4J.

I hope that removes all dependencies on xbase (I believe it will for DAP side, it may not for LSP).

@lemmy
Copy link
Author

lemmy commented Jan 19, 2021

Any suggestions how to remove @Pure without rewriting AccessorProcessor and EqualsHashCodeProcessor? @Pure is hard-wired in AccessorProcessor's getter generator and EqualsHashCodeProcessors's addHashcode/addEquals.

As a last resort, I can add a post-gen step to gradle to remove @Pure from the POJOs.

@jonahgraham
Copy link
Contributor

I don't know. There are a few xtend/xtext experts in lsp4j, but you may get better answers from their specific channels?

@cdietrich
Copy link
Contributor

cdietrich commented Jan 19, 2021

The simplest is to copy&paste the original code.
You may also check if calling removeAnnotation on MutableField and MethodDeclaration works
(similar to the calls on class here https://github.com/eclipse/xtext-xtend/blob/7c6989c0d362cdd3c0754d577989e849c3d1130e/org.eclipse.xtend.ide.tests/src/org/eclipse/xtend/ide/tests/macros/AbstractReusableActiveAnnotationTests.xtend#L674)

lemmy added a commit to lemmy/lsp4j that referenced this issue Jan 19, 2021
Remove dependency on xtext's @pure and ToStringBuilder

This commit removes @pure.  The next commit is going to remove
ToStringBuilder.

lsp4j.debug depends on xtext base at runtime, because the generated
POJOs import org.eclipse.xtext.xbase.lib.Pure and
org.eclipse.xtext.xbase.lib.util.ToStringBuilder. Neither @pure
nor ToStringBuilder are required by lsp4j.debug's functionality.
Removes the xtext dependency to shrink the set of lsp4j.debug's
dependencies.
lemmy added a commit to lemmy/lsp4j that referenced this issue Jan 19, 2021
Remove dependency on xtext's @pure and ToStringBuilder

This commit removes ToStringBuilder.

lsp4j.debug depends on xtext base at runtime, because the generated
POJOs import org.eclipse.xtext.xbase.lib.Pure and
org.eclipse.xtext.xbase.lib.util.ToStringBuilder. Neither @pure
nor ToStringBuilder are required by lsp4j.debug's functionality.
Removes the xtext dependency to shrink the set of lsp4j.debug's
dependencies.
@lemmy
Copy link
Author

lemmy commented Jan 19, 2021

I appreciate your time but this PR has gone beyond my time budget (I will simply create dummies for @Pure and ToStringBuilder in my client jar). My fork of lsp4j contains an incomplete attempt to remove @Pure in case someone familiar with xtext wants to take a shot.

Thanks again!

@lemmy lemmy closed this as completed Jan 19, 2021
@jonahgraham
Copy link
Contributor

Thanks for having a look. Please let us know if you have other ideas for improvement.

lemmy added a commit to lemmy/tlaplus that referenced this issue Jan 20, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug]
lemmy added a commit to lemmy/tlaplus that referenced this issue Jan 20, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug]
lemmy added a commit to lemmy/tlaplus that referenced this issue Jan 20, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug]
lemmy added a commit to lemmy/tlaplus that referenced this issue Jan 28, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug]
@jonahgraham
Copy link
Contributor

I think the correct code that @lemmy started in

https://github.com/lemmy/lsp4j/blob/1bf4a5b49539d046bd9f28d3dd06e1bfd18e1063/org.eclipse.lsp4j.generator/src/main/java/org/eclipse/lsp4j/generator/JsonRpcDataProcessor.xtend#L44-L54

should be

		impl.getDeclaredMethods.forEach [ method | 
			val purified = method.findAnnotation(Pure.findTypeGlobally)
			if (purified !== null) {
				method.removeAnnotation(purified)
			}
		]

In addition, a new implementation of toString needs to be provided. ToStringBuilder does quite a bit, but it should be easy enough to add a toString that is sufficient but not the same content.

I leave the above in case someone wants to pick up the work.

@jonahgraham jonahgraham added this to the v0.11.0 milestone Feb 9, 2021
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 12, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 17, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 18, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 19, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 22, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 23, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 24, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Feb 25, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Mar 10, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
lemmy added a commit to lemmy/tlaplus that referenced this issue Mar 23, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
@jonahgraham
Copy link
Contributor

I think that @cdietrich changes in #529 will fix this.

@jonahgraham jonahgraham reopened this Mar 26, 2021
@jonahgraham jonahgraham linked a pull request Mar 26, 2021 that will close this issue
@jonahgraham jonahgraham removed this from the v0.11.0 milestone Apr 2, 2021
lemmy added a commit to lemmy/tlaplus that referenced this issue Apr 2, 2021
ClassNotFoundExceptions while debugging.  See
eclipse-lsp4j/lsp4j#494 for context.

[Bug][Debugger]
@jonahgraham jonahgraham added this to the 0.21.0 milestone May 17, 2023
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 a pull request may close this issue.

4 participants