Scope symbol IDs per element package name #788

felipecsl · 2016-10-26T20:53:09Z

the integer ID itself is not enough to correctly identify a symbol. We need to scope that somehow in order to prevent clashes. This can be observed in a multiple modules project where butterknife is used in modules that depend on other modules that also use Butterknife.
Since IDs can repeat across modules, we need to scope them according to the package name of the current element being evaluated.
This PR fixes #779

felipecsl · 2016-10-26T20:57:34Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

@@ -1231,17 +1246,20 @@ private void scanForRClasses(RoundEnvironment env) {
      for (Element element : env.getElementsAnnotatedWith(annotation)) {
        JCTree tree = (JCTree) trees.getTree(element, getMirror(element, annotation));
        if (tree != null) { // tree can be null if the references are compiled types and not source
+          String respectivePackageName =
+              elementUtils.getPackageOf(element).getQualifiedName().toString();
+          scanner.setCurrentPackageName(respectivePackageName);


This is pretty janky, but couldn't figure out a simpler way to do this. Suggestions welcome

felipecsl · 2016-10-26T20:57:50Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

@@ -1284,28 +1302,38 @@ private void parseCompiledR(TypeElement rClass) {
  }

  private static class RClassScanner extends TreeScanner {
-    private final Set<String> rClasses = new LinkedHashSet<>();
+    // Maps the currently evaulated packageName to R Classes
+    private final Map<String, String> rClasses = new LinkedHashMap<>();


This is also a bit confusing

felipecsl · 2016-10-27T05:47:37Z

@kageiit wonder if you could take a look too, since you wrote this originally

kageiit · 2016-10-27T07:52:01Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

-  private Id getId(int id) {
-    if (symbols.get(id) == null) {
-      symbols.put(id, new Id(id));
+  private Id getId(QualifiedId qualifiedId) {


QualifiedId already has the id as part of it right? why do you still need the symbol map

Correct. I kept the Map just for the fast lookup

kageiit · 2016-10-27T07:52:40Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

    }
  }

-  private void parseRClass(String rClass) {
+  private void parseRClass(String respectivePackageName, String rClass) {


rename to rPackageName?

felipecsl · 2016-10-27T19:58:08Z

@JakeWharton friendly ping!

JakeWharton · 2016-10-28T01:52:30Z

Monday at earliest. Traveling.

On Thu, Oct 27, 2016 at 8:58 PM Felipe Lima notifications@github.com
wrote:

@JakeWharton https://github.com/JakeWharton friendly ping!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#788 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEeyfWeVNCyLjelMC0Tqs58Vb74muks5q4QJRgaJpZM4Khp_h
.

felipecsl · 2016-10-28T02:11:53Z

No worries, I just realized that this is not quite right yet. More updates coming

kunong · 2016-11-14T08:42:31Z

@felipecsl anymore updates coming? I'm a guy that waiting this fix. :)

felipecsl · 2016-11-14T18:43:36Z

@kunong yep, planning to finish today or tomorrow

javajoker · 2017-01-10T13:02:38Z

@felipecsl , any update on this issue? Thanks, Ning

felipecsl · 2017-01-11T22:46:17Z

no updates yet. We stopped seeing this issue so it's not burning anymore.

mrhether · 2017-01-17T18:12:40Z

@felipecsl what still needs to be done here?

felipecsl · 2017-01-17T18:52:16Z

@mrhether If I'm not mistaken, this change breaks usage of ButterKnife when you're not using it as a gradle plugin. If you always use it as a plugin, then it should work fine.

mrhether · 2017-01-17T20:03:41Z

@felipecsl Is this hosted anywhere?

felipecsl · 2017-01-17T20:05:22Z

Nope

…

On Tue, Jan 17, 2017 at 12:03 PM Mark ***@***.***> wrote: @felipecsl <https://github.com/felipecsl> Is this hosted anywhere? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#788 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALouJ3YzL6O42mVU0wU9Dx9WfocUvjSks5rTR6vgaJpZM4Khp_h> .

This PR does not work with projects that use butterknife in application modules!! Squashed commit of the following: commit 02a845f Merge: 34b2228 d5fb986 Author: Felipe Lima <felipe.lima@gmail.com> Date: Mon Nov 21 11:58:11 2016 -0800 Merges with master commit 34b2228 Author: Felipe Lima <felipe.lima@gmail.com> Date: Thu Oct 27 12:57:23 2016 -0700 Simple rename commit a34d982 Author: Felipe Lima <felipe.lima@gmail.com> Date: Thu Oct 27 12:56:48 2016 -0700 More checkstyle commit 8098a01 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 26 22:46:55 2016 -0700 Make checkstyle happy commit 4b8f7f5 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 26 14:01:01 2016 -0700 Also revert this commit dd64ff5 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 26 13:59:02 2016 -0700 Undo some changes commit e2847f1 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 26 13:54:44 2016 -0700 Remove debug statements commit 3d83d25 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 26 13:49:06 2016 -0700 Fixes JakeWharton#779 by scoping symbols by package commit 1692866 Author: Felipe Lima <felipe.lima@gmail.com> Date: Wed Oct 19 17:43:45 2016 -0700 Adds failing test for JakeWharton#779

My understanding of whats going on is it is possible for R files in two different modules to generate the same id for different resoureces. Butterknife used to use a Map<Integer,Id> where Id is a class that contains knowledge of how to generate code. Thus, when ids are duplicated only one would win the map. The loser would have it's generated code import the wrong id. Now we qualify the id with the package name of the id element. However, due to the structure of the code-gen, we parse R files without knowing exactly what packages reference which generated ids. But we do know all the packages that reference something in the R file. So we can defensively generated id's for ALL permutations. This sorta stinks from a performance stand-point, but is the only quick way I can figure out to use the QualifiedId, and gaurentee we generate all the qualified ids we will need. Essentially, for every package that has a butterknife annotation, we parse all R files in that package with all package combinatios. This creates many unneeded qualified ids, but it gaurentees that everything will be present.

felipecsl · 2017-02-23T19:11:46Z

Alright thanks to @BenSchwab 's airbnb@b0aa4ad this is finally functional and ready for review.
We've been using it internally for a ~week and it seems to have fixed the bug.
@JakeWharton PTAL at your earliest convenience 😄

JakeWharton · 2017-02-24T04:35:05Z

build.gradle

@@ -37,7 +37,7 @@ subprojects { project ->
      }
    }
    dependencies {
-      classpath 'com.android.tools.build:gradle:2.2.2'
+      classpath 'com.android.tools.build:gradle:2.3.0-rc1'


Is this required? Non-final releases are timeboxed and I don't want to commit code that uses pre-release versions because that makes the commit unbuildable after a month or two.

not required. it was just Android Studio being annoying. Reverting now..

JakeWharton · 2017-02-24T04:38:51Z

Looks like there's a checkstyle failure.

felipecsl · 2017-02-24T19:23:01Z

Green and ready to go 🎉

felipecsl added 2 commits October 19, 2016 17:43

Adds failing test for JakeWharton#779

1692866

Fixes JakeWharton#779 by scoping symbols by package

3d83d25

felipecsl changed the title ~~Fix #779~~ Scope symbol IDs per element package name Oct 26, 2016

felipecsl added 2 commits October 26, 2016 13:54

Remove debug statements

e2847f1

Undo some changes

dd64ff5

felipecsl commented Oct 26, 2016

View reviewed changes

felipecsl added 2 commits October 26, 2016 14:01

Also revert this

4b8f7f5

Make checkstyle happy

8098a01

kageiit reviewed Oct 27, 2016

View reviewed changes

felipecsl added 2 commits October 27, 2016 12:56

More checkstyle

a34d982

Simple rename

34b2228

Merges with master

02a845f

mrhether mentioned this pull request Jan 17, 2017

ButterKnife generates ViewBinding class with wrong resource ID #779

Closed

felipecsl and others added 4 commits February 23, 2017 10:55

Merge branch 'master' into felipe/fix-779

25ee5d5

Final touches

4b6b08c

Fix failing unit test

aa34174

JakeWharton approved these changes Feb 24, 2017

View reviewed changes

Fix checkstyle

4e145cd

JakeWharton added 4 commits February 24, 2017 23:58

Fix bad import ordering.

adf5c1d

Fix more bad import ordering.

67acd50

Correct wrapping.

af81eff

Better toString and equals

663d91c

JakeWharton merged commit 4ad9f98 into JakeWharton:master Feb 27, 2017

felipecsl deleted the felipe/fix-779 branch May 24, 2017 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scope symbol IDs per element package name #788

Scope symbol IDs per element package name #788

felipecsl commented Oct 26, 2016 •

edited

felipecsl Oct 26, 2016

felipecsl Oct 26, 2016

felipecsl commented Oct 27, 2016 •

edited

kageiit Oct 27, 2016

felipecsl Oct 27, 2016

kageiit Oct 27, 2016

felipecsl Oct 27, 2016

felipecsl commented Oct 27, 2016

JakeWharton commented Oct 28, 2016

felipecsl commented Oct 28, 2016

kunong commented Nov 14, 2016

felipecsl commented Nov 14, 2016

javajoker commented Jan 10, 2017

felipecsl commented Jan 11, 2017

mrhether commented Jan 17, 2017

felipecsl commented Jan 17, 2017

mrhether commented Jan 17, 2017

felipecsl commented Jan 17, 2017 via email

felipecsl commented Feb 23, 2017 •

edited

JakeWharton Feb 24, 2017

felipecsl Feb 24, 2017

JakeWharton commented Feb 24, 2017

felipecsl commented Feb 24, 2017

Scope symbol IDs per element package name #788

Scope symbol IDs per element package name #788

Conversation

felipecsl commented Oct 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecsl commented Oct 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecsl commented Oct 27, 2016

JakeWharton commented Oct 28, 2016

felipecsl commented Oct 28, 2016

kunong commented Nov 14, 2016

felipecsl commented Nov 14, 2016

javajoker commented Jan 10, 2017

felipecsl commented Jan 11, 2017

mrhether commented Jan 17, 2017

felipecsl commented Jan 17, 2017

mrhether commented Jan 17, 2017

felipecsl commented Jan 17, 2017 via email

felipecsl commented Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeWharton commented Feb 24, 2017

felipecsl commented Feb 24, 2017

felipecsl commented Oct 26, 2016 •

edited

felipecsl commented Oct 27, 2016 •

edited

felipecsl commented Feb 23, 2017 •

edited