[eclipse/xtext-xtend#903] Overhaul Accessors Annotation

[ivaniesta14]

The @Accessors can now delegate annotations to the getters. The annotation detects deprecated fields and generates @Deprecated when required. It is an error to set any of the deprecation fields on the annotation when the field is already deprecated (checked by the Java compiler, not by the annotation processor). The setter can be set to return this. The annotation can delegate warning types to suppress. Classes may declare a "custom getter" by annotating one instance method with @CustomGetter. Such methods have to take no arguments and cannot be static, and cannot return void. (The void method validation is done by the Java compiler, not by the annotation processor). When a class uses a custom getter, an additional method is generated for annotated fields of that type: a "raw" getter, which always returns the field (has the same name as the getter, but with Raw between the prefix and the field name). Note that settings of the getter and the raw getter set via the annotation are not shared.

Signed-off-by Iván Nieves <ivaniesta14 at gmail dot com>

[cdietrich]

@ivaniesta14 can you please signoff the commit

--amend -s

[szarnekow]

Look Github, it's a link: eclipse/xtext-xtend#903

[szarnekow]

@ivaniesta14 I took a first quick look at the code and the PR appears to mix a few different changes into one. But most importantly it lacks new tests for the accessors annotation. The original bug report was about missing @Override in some cases. I recommend to start with a fix for these + tests. All the new configuration flags on @Accessors have to carry their own weight, too. Let's say I want to suppress warnings on the getter: In that case I'd probably just implement the getter manually and add a small comment why the warning must be suppressed. Same for the setter. Having a boolean flag for the deprecation: Same story. If I want the getter / setter to be deprecated, I'd like to provide a short message in the JavaDoc, too. No need for the abstraction on the @Accessors annotation, I suppose.

So in order to prepare a thorough review: Can you please split this up into smaller pieces and create a ticket for the functionality that you miss in @Accessors such that this can be discussed?

[ivaniesta14]

Commit signed off. Will try to add tests later, but the gradlew bundled in the repo doesn't accept Java 14, will try to use older versions. All old tests should pass, and the (local) testing I have done seem to confirm everything works as intended. I will also add a ticket to each individual configuration setting I added to the annotation.

[cdietrich]

@ivaniesta14 can you please rebase this branch on origin/master

[ivaniesta14]

Rebase complete, hence the huge amount of commit updates. Ran gitk and it seems lilke the commit order is properly set now.

[cdietrich]

no. with a proper rebase there should have been only 1 commit

[cdietrich]

=> can you please clean that up or create a new branch + pr and cherry pick your changes

[cdietrich]

ahhh i get it. you pr against sebastians branch, not against master. will rebase sebastians branch. that should clean it up

[cdietrich]

now it looks better. there is still the signoff problem

[cdietrich]

will try to retrigger the eca validation. this is so a crappy tool

[szarnekow]

@ivaniesta14 thank you filing the individual smaller tickets. Do you still plan to split this up?

[ivaniesta14]

Considering this sprouted from a single issue, and that the other issues have relatively small fixes, for the sake of avoiding unnecessary clutter and bugs I don't think that it would be a good idea to split the PR.

[szarnekow]

Before you put more effort into the PR as is: I’ll be completely transparent and open with this - the added features won’t make it into the lib as is. Please feel free to use an own active annotation for the purpose of non-standard signatures for setters or added annotations that are specific to the field or method. If you are willing to shrink this PR towards adding the missing overrides and validation that avoids an attempt to override final methods, I’m happy to review this, as soon as tests are available.

[ivaniesta14]

Understood. Thanks for the feedback. I will trim down features and fix the code style issues as soon as possible. Tests, however, are beyond the scope of this PR (another PR will be filed against eclipse/xtext-xtend which will contain the tests corresponding to this PR) since testing the annotation requires processing code to make sure it behaves as intended.

[ivaniesta14]

I'm having trouble compiling the code. For some reason running .\gradlew build sometimes crashes with the following error:

> Task :org.eclipse.xtend.lib:javadoc
Unhandled exception
Type=Segmentation error vmState=0x00000000
Windows_ExceptionCode=c0000139 J9Generic_Signal=00000004 ExceptionAddress=00007FFF7577BF18 ContextFlags=0010000f
Handler1=00007FFF20C7C6C0 Handler2=00007FFF3D9DBF50
RDI=000000000F3C10C8 RSI=00007FFF757984C0 RAX=0000000000360034 RBX=00000000C0000139
RCX=00007FFF7579D7B8 RDX=0000000000000000 R8=0000000000000000 R9=0000000000000000
R10=0000000000001112 R11=0000000000001000 R12=0000000000000003 R13=0000000000000003
R14=00007FFF2D708401 R15=0000000000000003
RIP=00007FFF7577BF18 RSP=00000000026FBF30 RBP=00000000026FC5D0 GS=002B
FS=0053 ES=002B DS=002B
XMM0 00007fff7579d588 (f: 1970918784.000000, d: 6.953241e-310)
XMM1 00007fff7579d550 (f: 1970918784.000000, d: 6.953241e-310)
XMM2 000000007ffe0385 (f: 2147353472.000000, d: 1.060934e-314)
XMM3 0000000000370021 (f: 3604513.000000, d: 1.780866e-317)
XMM4 00000000007621a0 (f: 7741856.000000, d: 3.824985e-317)
XMM5 000000000000000c (f: 12.000000, d: 5.928788e-323)
XMM6 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM7 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM8 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM9 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM10 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM11 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM12 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM13 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM14 0000000000000000 (f: 0.000000, d: 0.000000e+000)
XMM15 0000000000000000 (f: 0.000000, d: 0.000000e+000)
Module=C:\WINDOWS\SYSTEM32\ntdll.dll
Module_base_address=00007FFF75680000 Offset_in_DLL=00000000000fbf18
Target=2_90_20191106_498 (Windows 10 10.0 build 18362)
CPU=amd64 (12 logical CPUs) (0x1f624d000 RAM)
----------- Stack Backtrace -----------

The crash happens (randomly) while compiling any of the :javadoc or the gwt-related tasks. In some cases, the build crashes immediately after starting it, leaving Powershell unable to stop the command via Ctrl+C. It should also be noted that the bundled gradlew version will not accept Java 13 or above.

I will try updating my Java 8 installation (currently trying to compile on Windows 10, OpenJ9 8u232) and/or compiling against a different Java distribution, but the requested fixes will probably take longer than intended.

[cdietrich]

how to continue here. i am also againt growing the AA libary too much

[ivaniesta14]

At this point I have removed almost all features from the original PR, only @Override and @Deprecated are added when needed/requested now.

I certainly agree the annotation should not have too many features added at once. Right now @Accessors has three more fields than originally, two of which are redundant and will be removed as soon as I can get eclipse/xtext-xtend to compile in order to add appropiate tests (for some reason subclasses fail to find MethodDeclaration.getOverriddenOrImplementedMethods)

I should also mention the fact that with the new implementation @Deprecated is inherited from the field by default, let me know if never adding the annotation (current behaviour) should be the default instead.

[cdietrich]

@ivaniesta14 can you please rebase all commits into 1

[ivaniesta14]

Was just about to do that

[ivaniesta14]

Rebase complete. Note that the branch sz_issue903 on eclipse/xtext-xtend might also need rebasing to keep up with changes to master.

[cdietrich]

Note that the branch sz_issue903 on eclipse/xtext-xtend might also need rebasing to keep up with changes to master.

done

[cdietrich]

we might need to change the upstream branch too. will have a look later

[cdietrich]

@ivaniesta14 can you please rebase your pr on eclipse/master

[ivaniesta14]

The PR requires API already in eclipse/sz_issue903. IMHO it would be better to leave the PR as is, merge the commits into eclipse/sz_issue903 and then merge that branch into eclipse/master.

[cdietrich]

your branch is outdated compared to sz_issue903 on eclipse/xtext-xtend => the latter wont run

[cdietrich]

commit history look bad again. sure you did a rebase?

[ivaniesta14]

Rebase complete, thanks for the clarification. Now :sz_issue903 in eclipse/xtext-lib needs rebasing too.

[ivaniesta14]

commit history look bad again. sure you did a rebase?

Done one right now to update my branch to master

[cdietrich]

ahh i see. this pr is not against master.

[cdietrich]

@szarnekow how to proceed here?

[szarnekow]

I'd say merge into sz_issue903 and than merge into master.

[cdietrich]

can you please do so? also with the xtend pr (needs readjusting)

[szarnekow]

Thank you very much, @ivaniesta14