-
-
Notifications
You must be signed in to change notification settings - Fork 8
feat(r8-tests): Add R8 ambiguous tests #71
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
base: master
Are you sure you want to change the base?
Conversation
| "; | ||
|
|
||
| let expected = "\ | ||
| com.android.tools.r8.CompilationException: foo[parens](Source:3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R8's expected output looks like this:
"com.android.tools.r8.CompilationException: foo[parens](Source:3)",
--
" at com.android.tools.r8.R8.bar(R8.java:7)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:7)",
" at com.android.tools.r8.R8.bar(R8.java:8)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:8)",
" at com.android.tools.r8.R8.main(Unknown Source)",
"Caused by: com.android.tools.r8.CompilationException: foo[parens](Source:3)",
" at com.android.tools.r8.R8.bar(R8.java:9)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:9)",
" ... 42 more
but our UI does not really support the <OR> case, so I'm unsure if we should tackle this now? Ideally though, we should, because the current remapped stacktrace can be quite misleading.
|
|
||
| let expected = "\ | ||
| com.android.tools.r8.CompilationException: | ||
| at com.android.tools.r8.R8.foo(R8.java:42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here -- retrace actually expands this to mention all of the potential inline frames:
"com.android.tools.r8.CompilationException:",
--
" at com.android.tools.r8.R8.foo(R8.java:42)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:43)",
" <OR> at com.android.tools.r8.R8.foo(R8.java:44)",
" at com.android.tools.r8.R8.bar(R8.java:32)",
" at com.android.tools.r8.R8.baz(R8.java:10)
| "; | ||
|
|
||
| let expected = "\ | ||
| java.lang.IndexOutOfBoundsException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the order in retrace is different:
"java.lang.IndexOutOfBoundsException",
--
"\tat some.inlinee1(some.java:10)",
"\t<OR> at some.inlinee2(some.java:20)",
"\tat com.android.tools.r8.Internal.foo(Internal.java:10)",
"\t<OR> at com.android.tools.r8.Internal.foo(Internal.java:42)
seems like retrace has two alternative expansion paths for a single obfuscated frame:
Alt A: some.inlinee1 → Internal.foo(:10)
Alt B: some.inlinee2 → Internal.foo(:42)
When it prints with <OR>, it’s not printing “Alt A then Alt B”, but a column-wise merge by stack depth:
depth 0: inlinee1
depth 0 alt: inlinee2
depth 1: Internal.foo(:10)
depth 1 alt: Internal.foo(:42)
I guess this also depends on the decision if we're going to implement alternative stacktraces or not
|
@loewenheim I've updated tests to match the exact expected output from retrace, and have a couple of questions that we need to make a decision on, hence re-requested a review |
|
I would honestly not support these alternative stacktraces for now—I don't understand how they work and can't see how we would consume them. |
|
|
||
| This doc summarizes the current failures from running `cargo test --test r8-ambiguous`. | ||
|
|
||
| ## `test_ambiguous_method_verbose_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
| - The input frames have **no line numbers** (e.g. `(Foo.java)`), which means `frame.line == 0`. | ||
| - The current remapper does not produce any remapped candidates for these frames (so `format_frames` falls back to printing the original line). This indicates a gap in the “no-line” / best-effort ambiguous member mapping behavior for `line == 0` frames. | ||
|
|
||
| ## `test_ambiguous_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
| - These frames have `Unknown Source` with **no line number**, so `frame.line == 0`. | ||
| - For `line == 0`, the current implementation ends up with **no remapped candidates** and prints the original frame line unchanged (instead of emitting ambiguous alternatives `foo`/`bar`). | ||
|
|
||
| ## `test_ambiguous_missing_line_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
| - The mapping entries have **no original line information** (base/no-line mappings). | ||
| - The current mapping logic uses the mapping’s “original start line” (which defaults to `0`) rather than propagating the **caller-provided minified line** when available. | ||
|
|
||
| ## `test_ambiguous_with_multiple_line_mappings_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
| - `(...(Unknown))` parses as a frame with `file = "Unknown"` and `line = 0`. | ||
| - All available member mappings are **line-ranged** (e.g. `10:10`, `11:11`, `12:12`), so with `line == 0` they do not match and the remapper produces **no candidates**, falling back to the original frame line. | ||
|
|
||
| ## `test_ambiguous_with_signature_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
| - Same `line == 0` issue: line-ranged overload mappings cannot be selected without a minified line. | ||
| - The remapper currently has no fallback that returns “best effort” candidates for `line == 0` frames (e.g., returning all overloads, or preferring a base mapping if present). | ||
|
|
||
| ## `test_inline_no_line_assume_no_inline_ambiguous_stacktrace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs fixing
Part of #40
There are failing tests in this PR that need fixes in the mapper/cache itself, so I will open a follow-up PR that targets this branch.
R8 Retrace Test Coverage