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

Disagreements in token type are not handled correctly #30

Open
JakeWharton opened this issue Jan 14, 2023 · 2 comments
Open

Disagreements in token type are not handled correctly #30

JakeWharton opened this issue Jan 14, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@JakeWharton
Copy link
Collaborator

res/values/strings.xml:

<resources>
  <string name="library_text_argument">{name, date}</string>
</resources>

res/values-es/strings.xml:

<resources>
  <string name="library_text_argument">{name, number}</string>
</resources>

Currently this produces:

public object FormattedResources {
  public fun library_text_argument(name: Instant): FormattedResource {
    val arguments = ArrayMap<String, Any>(1)
    arguments.put("name", Date.from(name))
    return FormattedResource(
      id = R.string.library_text_argument,
      arguments = arguments
    )
  }

  public fun library_text_argument(name: Number): FormattedResource {
    val arguments = ArrayMap<String, Any>(1)
    arguments.put("name", name)
    return FormattedResource(
      id = R.string.library_text_argument,
      arguments = arguments
    )
  }
}

I suspect we simply want to enforce the types to match across all configurations and fail the build otherwise.

Are there any cases where you might want to have multiple types for a single token? Solutions to this depend on #29 a bit, since a workaround for that would be to name them differently (provided we allow this with an XML attribute somehow). Then at the callsite you would have to do library_text_argument(nameDate = someInstance, nameNumber = 12).

@JakeWharton
Copy link
Collaborator Author

Regarding this and #29, in ViewBinding the resource processing was split into three phases:

  1. Parse resource XMLs in each configuration, warn or error on broken things within a single XML (malformed)
  2. Merge resource XMLs by ID across configurations, warn or error on broken things across XMLs (semantic breakable such as disagreements)
  3. Codegen from merged resource XMLs

We probably need a similar pipeline here. It may require two models, at least partially. Otherwise you might lose some info.

JakeWharton added a commit that referenced this issue Jan 17, 2023
For strings which match, this reduces the generated bytecode impact.
Each argument 'put' operation in the map required:

    36: aload         6
    38: ldc           #99                 // String 0
    40: aload_1
    41: invokevirtual #30                 // Method android/util/ArrayMap.put:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
    44: pop

plus the associated string pool constant's size.

In the array, storage requires fewer bytecodes and bytecodes with fewer arguments reducing their size:

    32: aload         7
    34: iconst_0
    35: aload_1
    36: aastore

Creating the map used to cost 10 bytes and require invoking the ArrayMap constructor:

    26: new           #23                 // class android/util/ArrayMap
    29: dup
    30: iconst_5
    31: invokespecial #26                 // Method android/util/ArrayMap."<init>":(I)V
    34: astore        6

But the array requires only 5 bytes and zero method calls:

    27: anewarray     #4                  // class java/lang/Object
    30: astore        7

Finally, previously, the `Map` property of the `FormattedResource` class required an explicit cast inside the generated code:

     89: new           #32                // class app/cash/gingham/FormattedResource
     92: dup
     93: ldc           #108               // int 2131689631
     95: aload         6
     97: checkcast     #35                // class java/util/Map
    100: invokespecial #38                // Method app/cash/gingham/FormattedResource."<init>":(ILjava/util/Map;)V
    103: areturn

But with the property changing to `Any` (i.e., `Object`) the cast is no longer required for either the array-based or map-based generated code:

    69: new           #32                 // class app/cash/gingham/FormattedResource
    72: dup
    73: ldc           #96                 // int 2131689631
    75: aload         6
    77: invokespecial #36                 // Method app/cash/gingham/FormattedResource."<init>":(ILjava/lang/Object;)V
    80: areturn
@JakeWharton
Copy link
Collaborator Author

This is actually a problem within a single configuration, too:
values/strings.xml:

<string name="hi">{count,date} {count,select}</string>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant