Removed synthetic methods

[RiteshChandnani]

Description

This removes all the synthetic methods.
Issue #1555

Motivation and Context

This change helps us reduce the method count and therefore also helps us avoid the trampoline created because of synthetic methods.

[TWiStErRob]

Looking at the diff I think it would be a good idea to leave /private/ in the code and put a comment in the default constructors so that a future cleanup wouldn't reverse the changes.

How does adding a default scoped ctor help? Without a default scoped ctor (1 method) a private one is generated with an accessor (2 methods)?

The 26 PMD violations are likely UnnecessaryConstructors.

[RiteshChandnani]

@TWiStErRob
Thanks for checking my PR.
I will put a comment in default constructors to avoid cleanup. But, I can't get my arms around - " I think it would be a good idea to leave /private/ in the code", can you please elaborate a bit more.

And, yes, if we don't have a default scoped ctor, two ctors will be synthesized, one will be the private constructor with no params and one will be a package private constructor with an arbitrary param (its type is dependent on compiler implementation, and hence one addtional empty class to support ctor overloading.
I also made a dummy implementation and used reflection and found the same.

Sorry, due to different timezones I was unable to get back to you immediately.

[TWiStErRob]

I think it would be a good idea to leave /*private*/ in the code

If you look at this code out of context, what you see is that one of the fields are non-private for seemingly no reason (all usages of it are within the same class).

  private final Glide glide;
  final Lifecycle lifecycle;
  private final RequestTracker requestTracker;

This would make it more clear that it was supposed to be private, but cannot due to some limitation.

  private final Glide glide;
  /*private*/ final Lifecycle lifecycle;
  private final RequestTracker requestTracker;

potentially an even better approach would be a @PreventSynthetic or @Private source-level annotation to prevent thinking of commenting out as a forget-to-revert-after-debug thing.

Actually, thinking of it, isn't there a library out there which does this automatically? I mean if there is a way to have lambdas in Java 5, some APT code should be able to increase visibility of fields.

[RiteshChandnani]

Got it.
So, we can have a @PreventSynthetic source-level annotation to make the code more clear.

Like this one:
https://github.com/hidroh/materialistic/blob/master/app/src/main/java/io/github/hidroh/materialistic/annotation/Synthetic.java

So, I can go forward and implement that source-level annotation and update the code accordingly?

Yeah, there should be a library which does this automatically, but I was unable to find something after doing a quick search.

[TWiStErRob]

Yep, that example looks good, hidroh/materialistic#642 is where it was introduced.

So the changes are:

-   private final Type name;
+   @Synthetic final Type name;

I like the shorter @Synthetic name. Annotate the constructors as well, then they can be left empty.

[TWiStErRob]

Btw, don't know what @sjudd's stand is on this, I hope he'll accept, but not sure.

[RiteshChandnani]

Great.
Will do it right away!

Yeah, even I hope the same about that.

[RiteshChandnani]

Btw, I think we should create a new directory named annotation in glide/library/src/main/java/com/bumptech/glide/

What's your take?

[RiteshChandnani]

Also, diskLruCache is a git submodule, can I go and update that one too?

[TWiStErRob]

Yes, you can contribute to disklrucache as well, it's http://github.com/sjudd/disklrucache that you need to fork. You need two separate PRs (this and another) and you can't depend on code in each other (i.e. the annotation). As far as I remember the cache doesn't have many classes, so it should be less likely to have synthetics.

I would put the annotation in library/src/main/java/com/bumptech/glide/util, because it's not part of the public API.

[RiteshChandnani]

@TWiStErRob
Got it.
Thanks!
I will first finish with this PR and later look into disklrucache.

Will move the annotation directory under library/src/main/java/com/bumptech/glide/util

[RiteshChandnani]

No worries. I will fix it.

[TWiStErRob]

You can run gradlew checkstyle pmd to see the problems.

You'll likely need @SuppressWarnings("PMD.SpecificViolationName") on the constructors, or try to modify the PMD config if it's possible to suppress the violations when the Synthetic annotation is present.

[RiteshChandnani]

All checks passed locally, but I used @SuppressWarnings("PMD.UncommentedEmptyConstructor") rather than changing PMD config because I don't know much about writing custom PMD rules.

What's your take?
Can I go forward and push that code?

[TWiStErRob]

Why suppress if you can write a comment inside? Suppressions usually need to be explained, but if there's a comment, it explains itself.

I don't know much either, but based on a quick research it's quite simple. I found this: http://pmd.sourceforge.net/pmd-5.1.1/howtowritearule.html and the source for the rule is https://github.com/pmd/pmd/blob/master/pmd-java/src/main/resources/rulesets/java/design.xml#L1401-L1431 . I don't have time right now, but it looks like it should be as easy as adding and not(hasAnnotation(....Synthetic)) (junit.xml has examples of how you can do the hasAnnotation part). Since xpath is just a property I think it could be overridden like allowCommentedBlocks in <glide>/library/pmd-ruleset.xml.

I say if you have time or curious go ahead with changing the rule, if not, just go with comments.

[RiteshChandnani]

No worries, I will give it a try!
Thanks Robert!

[TWiStErRob]

Nice. Just noticed something: the samples are not part of the API, they're just usage examples. I think it's better to keep synthetics there, so they're more portable (copy-paste).

[RiteshChandnani]

Thanks a lot Robert!
So, I will remove synthetics from samples.

[TWiStErRob]

Thanks :) Now, we just have to wait for @sjudd to ack.

Note: there are some weird new-lines left in diff
https://github.com/bumptech/glide/pull/1556/files#diff-4fdac1e84ca614a23bd46483d941e644

[RiteshChandnani]

Yeah, no worries.

I will remove those weird lines.

[TWiStErRob]

I think you misunderstood me there, I mean that there are some 1-line changes where the change is just inserting a new line without any other change:

The import groups should be separated by a new line, and Checkstyle/PMD may fail on this last commit.

[RiteshChandnani]

ohh!
I am sorry.
I will jump to the previous commit.
Btw, that was the only file where that change was to be made?

[TWiStErRob]

No, but I mostly saw them in samples after your last push removing code from those. I suggest you squash your commits locally first, then amend the last commit to see if there are irrelevant modifications in the squashed diff. We would squash-merge anyway, so it doesn't matter that the commits are lost.

[RiteshChandnani]

Btw, I mean that there are some 1-line changes where the change is just inserting a new line without any other change

I have to just double sure that you mean to say that I have to get rid of that new line, right?

[TWiStErRob]

If there's no related change (like adding/removing an import or method), then there should no random whitespace changes in the diff either. So, yes, make sure any standalone added/removed newline is not in the diff, especially in files with no other changes.

[RiteshChandnani]

Btw, looking at the original source files I found that grouping is not applied in imports. Hence, to maintain consistency across the project I think I should also follow the same path.

[TWiStErRob]

Huh, that's interesting. Here's the commit that added groups: f7a6d65, but you're right the current code doesn't have groups, and that's because of 3f5543a. This is a recent change, I haven't been following the master branch closely enough. So, no separation. I wonder how Sam formatted the code... it looks like the import order in settings wasn't updated to reflect the style.

[RiteshChandnani]

Yeah! You are right too!

[TWiStErRob]

The current diff looks good, sorry about the confusion.

[RiteshChandnani]

No worries at all Robert!

I was honored to work on this!
Was not aware about PMD before this, now will go back and update my projects!

Thanks a lot! :)

[sjudd]

Thanks for putting this together!

Can you do a quick before/after on the class/method count to verify the changes?

[RiteshChandnani]

Yeah Sam,
The accessor methods -
Before count: 54
After count: 3

[TWiStErRob]

Huh, what's that 3 remaining? Btw, I think Sam wanted to see total numbers as well.
For comparison, according to this the old-old version had 2879 methods.

[RiteshChandnani]

After analysis I found:

But why Flag is not being raised by the IDE for these :/

[TWiStErRob]

IDEA/AS is not perfect, they are always working to make them better, just check out the EAP release notes. I reported quite a few myself the latest one being what you just found: IDEA-163379.

[RiteshChandnani]

Ohh!
Thanks!
I was able to bring that down to 0 by annotating those variables as @Synthetic and changing scope to package-private

[TWiStErRob]

Yep, the class file is what matters in the end, IDE warnings are just helpers.
Don't forget to commit and push the changes ;)

[RiteshChandnani]

Yeah! Will do it right away! 👍

[RiteshChandnani]

After analyzing both versions of dex files I found total method count as -

Before: 3282
After: 3202

Also,

The accessor methods count -
Before: 54
After: 0

[RiteshChandnani]

I found class count as -

Before: 470
After: 456

[sjudd]

LGTM, Thanks!

I wonder if it's possible to write a build check for this?

[RiteshChandnani]

@sjudd

I am thankful to you too for writing such a great library! :)

I wonder if it's possible to write a build check for this?
Even I am musing about the same..

[TWiStErRob]

I think PMD works on class files, so we can check if they have access\$\d\d[02] methods.

[TWiStErRob]

@RiteshChandnani bug you found with sibling-synthetics is fixed already.

[RiteshChandnani]

Yeah I checked too!
https://youtrack.jetbrains.com/issue/IDEA-163379

Nice that it has been fixed fixed! :)
Thanks Robert! 👍