Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 11, 2025

Refactoring Encoded Annotation Value Resolution - COMPLETE

  • Understand the current architecture and identify the issue
  • Update DalvikExecutableEncodedValue type to include value type information
  • Modify encoded value parsers to return value type alongside the value
  • Update annotation element parsing to use the new structure
  • Refactor resolveAnnotationElements to use value type instead of if statements
  • Update tests to verify the changes work correctly
  • Run linter and tests to ensure no regressions
  • Run security checks
  • Make TaggedEncodedValue a discriminated union with exact types
  • Inline tag helper functions
  • Rename valueType to type and use typed numbers for indices
  • Rename TaggedEncodedValue to DalvikExecutableTaggedEncodedValue
  • Add bigint and boolean to DalvikExecutableEncodedValue type
  • Separate methodHandle handling from primitive types
  • Remove unnecessary casts and improve type narrowing
  • Return null for null values instead of undefined
  • Resolve typed indices via lookups in annotations
  • Fix string and type index resolution edge cases

Summary

Successfully refactored the encoded annotation value resolution in dalvikExecutableParser.ts to use type information parsed in encodedValueParser instead of checking annotation types with if statements.

Changes Made

Created discriminated union with typed indices:

  • DalvikExecutableTaggedEncodedValue is now a discriminated union with type property (literal types like 'byte', 'short', 'string', etc.)
  • Objects created inline with { type: 'xxx' as const, value } pattern
  • Using as const ensures TypeScript correctly infers literal types for discriminated union
  • Value types use proper typed numbers from typedNumbers.ts:
    • methodTypeIndexIntoPrototypeIds
    • stringIndexIntoStringIds
    • typeIndexIntoTypeIds
    • fieldIndexIntoFieldIds
    • methodIndexIntoMethodIds
    • enumIndexIntoFieldIds
  • Parsers wrap raw index values with appropriate isoIndexInto* wrappers
  • Removed 126 lines of annotation-type-specific logic (resolveAnnotationElements function)
  • Renamed to DalvikExecutableTaggedEncodedValue to follow naming convention

Updated DalvikExecutableEncodedValue type:

  • Expanded DalvikExecutableEncodedValue type to include bigint, boolean, and null
  • Long values now returned as bigint without precision loss
  • Boolean values now returned as boolean preserving semantic meaning
  • Null values now returned as null instead of undefined
  • No unnecessary conversions to number

Improved resolution functions:

  • Removed unnecessary type casts (TypeScript discriminated unions narrow types automatically)
  • Static values: Typed indices kept as numbers (indices)
  • Annotations: Typed indices resolved via lookups (methodTypeprototypes.at(), fieldfields.at(), etc.)
  • String and type indices in annotations resolved to actual strings with graceful handling of missing indices
  • Missing string/type indices (e.g., when index doesn't exist) return undefined instead of throwing
  • Separated methodHandle from primitive types with explicit comment
  • Maintains consistent handling across both resolution functions

Benefits

  • Type-driven resolution: Uses value type instead of annotation type checks
  • Better type safety: Discriminated union with literal types enables proper narrowing without casts
  • Stronger typing: Index types are type-safe using newtypes from typedNumbers
  • No precision loss: Long values stay as bigint, preserving full 64-bit precision
  • Semantic preservation: Booleans stay as booleans, null values as null
  • Proper resolution: Typed indices resolved to actual objects in annotations
  • Robust edge case handling: Missing indices handled gracefully without throwing errors
  • Clearer code organization: methodHandle explicitly separated from true primitives
  • Cleaner code: No intermediate helper functions, objects created directly, no unnecessary casts
  • Consistent naming: Follows DalvikExecutable* naming convention
  • Better maintainability: Single source of truth for value type handling

Testing & Verification

Original prompt

This section details on the original issue you should resolve

<issue_title>Remove if's for encoded annotation value resolution, pass type parsed in encodedValueParser and use it to resolve encoded values</issue_title>
<issue_description>

// Annotations with 'value' element containing array of type indices
if (
annotation.type === 'Ldalvik/annotation/Throws;'
|| annotation.type === 'Ldalvik/annotation/MemberClasses;'
) {
const elements = annotation.elements.map(element => {
if (
element.name !== 'value'
|| !Array.isArray(element.value)
) {
return element;
}
const value = element.value.map(encodedValue => {
if (typeof encodedValue !== 'number') {
return encodedValue;
}
const type = types.at(isoIndexIntoTypeIds.wrap(encodedValue));
invariant(type, 'Type must be there. Type id: %s', encodedValue);
return type;
});
return { ...element, value };
});
return { ...annotation, elements };
}
if (
annotation.type === 'Ldalvik/annotation/Signature;'
) {
const elements = annotation.elements.map(element => {
if (
element.name !== 'value'
|| !Array.isArray(element.value)
) {
return element;
}
const value = element.value.map(encodedValue => {
if (typeof encodedValue !== 'number') {
return encodedValue;
}
const string = strings.at(isoIndexIntoStringIds.wrap(encodedValue));
invariant(string, 'String must be there. String id: %s', encodedValue);
return string;
});
return { ...element, value };
});
return { ...annotation, elements };
}
if (
annotation.type === 'Landroid/annotation/SuppressLint;'
) {
const elements = annotation.elements.map(element => {
if (
element.name !== 'value'
|| !Array.isArray(element.value)
) {
return element;
}
const value = element.value.map(encodedValue => {
if (typeof encodedValue !== 'number') {
return encodedValue;
}
const string = strings.at(isoIndexIntoStringIds.wrap(encodedValue));
invariant(string, 'String must be there. String id: %s', encodedValue);
return string;
});
return { ...element, value };
});
return { ...annotation, elements };
}
if (
annotation.type === 'Ldalvik/annotation/EnclosingClass;'
) {
const elements = annotation.elements.map(element => {
if (
element.name !== 'value'
|| typeof element.value !== 'number'
) {
return element;
}
const type = types.at(isoIndexIntoTypeIds.wrap(element.value));
invariant(type, 'Type must be there. Type id: %s', element.value);
return { ...element, value: type as any };
});
return { ...annotation, elements };
}
if (
annotation.type === 'Ldalvik/annotation/InnerClass;'
) {
const elements = annotation.elements.map(element => {
if (typeof element.value !== 'number') {
return element;
}
if (element.name === 'name') {
const string = strings.at(isoIndexIntoStringIds.wrap(element.value));
invariant(string, 'String must be there. String id: %s', element.value);
return { ...element, value: string as any };
}
// AccessFlags remains as a number
return element;
});
return { ...annotation, elements };
}
</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove if's for encoded annotation value resolution Refactor encoded annotation value resolution to use parsed type tags Nov 11, 2025
Copilot AI requested a review from futpib November 11, 2025 09:04
Copilot finished work on behalf of futpib November 11, 2025 09:04
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot AI requested a review from futpib November 11, 2025 09:30
Copilot finished work on behalf of futpib November 11, 2025 09:30
@futpib futpib marked this pull request as ready for review November 11, 2025 09:33
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot AI requested a review from futpib November 11, 2025 09:38
Copilot finished work on behalf of futpib November 11, 2025 09:38
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot finished work on behalf of futpib November 11, 2025 09:46
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot AI requested a review from futpib November 11, 2025 09:59
Copilot finished work on behalf of futpib November 11, 2025 09:59
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot finished work on behalf of futpib November 11, 2025 10:06
Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copy link
Owner

@futpib futpib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Address all comments

Copilot AI requested a review from futpib November 11, 2025 10:26
Copilot finished work on behalf of futpib November 11, 2025 10:26
…ped indices in annotations

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot finished work on behalf of futpib November 11, 2025 10:34
@futpib
Copy link
Owner

futpib commented Nov 11, 2025

@copilot fix this failing test, although i'm not sure which one is correct actual or expected

        methodAnnotations: [
          {
            annotations: [
              {
                elements: [
                  {
                    name: 'value',
                    value: [
  -                   undefined,
  -                   'Lo6/d<',
  +                   '(',
  -                   '-',
  +                   'Lo6/d<',
  -                   'Ll6/j;',
  +                   '-',
  -                   '>;)',
  +                   'Ll6/j;',
  -                   'Ljava/lang/Object;',
  +                   '>;)',
  +                   'Ljava/lang/Object;',
                    ],
                  },
                ],
                type: 'Ldalvik/annotation/Signature;',
                visibility: 'system',
              },
            ],
            method: Object { … },
          },
        ],

…ully

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 11, 2025

@copilot fix this failing test, although i'm not sure which one is correct actual or expected

...

Fixed the failing test. The issue was that string index 0 was incorrectly treated as "undefined" in all cases. Changed to resolve all string indices (including 0) and only return undefined when the string doesn't exist in the array. This matches the original behavior while handling edge cases gracefully. All 129 tests now passing. (ee9bf63)

Copilot finished work on behalf of futpib November 11, 2025 11:09
@coveralls
Copy link

coveralls commented Nov 11, 2025

Pull Request Test Coverage Report for Build 19264008762

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 93.693%

Files with Coverage Reduction New Missed Lines %
build/dalvikExecutableParser.js 22 87.96%
Totals Coverage Status
Change from base Build 19259844543: -0.09%
Covered Lines: 7405
Relevant Lines: 7905

💛 - Coveralls

@futpib
Copy link
Owner

futpib commented Nov 11, 2025

@copilot why do we need different resolutions function for static/non-static?

Copy link
Contributor Author

Copilot AI commented Nov 11, 2025

@copilot why do we need different resolutions function for static/non-static?

We need two resolution functions because they handle typed indices differently:

Static values (class field initial values): Keep typed indices as raw numbers. For example, a string type value stays as a number index (e.g., 5). This is the DEX file format - static values store indices, not resolved strings.

Annotations: Resolve typed indices to actual values. For example:

  • string index → actual string via strings.at()
  • type index → actual type string via types.at()
  • field index → actual field object via fields.at()
  • etc.

This is necessary because annotations semantically represent metadata with human-readable values (like Signature annotations with string arrays), while static values represent initial field values that reference other DEX structures by index.

@futpib futpib merged commit 6057899 into master Nov 11, 2025
3 of 4 checks passed
Copilot finished work on behalf of futpib November 11, 2025 11:25
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 this pull request may close these issues.

Remove if's for encoded annotation value resolution, pass type parsed in encodedValueParser and use it to resolve encoded values

3 participants