Android code quality improvements#17636
Conversation
scalar_type_to_java_dtype.at(scalarType) throws std::out_of_range for unknown types before the .count() guard on the next line can run, making the error handling dead code. Move the .count() check before .at() so unsupported scalar types throw a proper Java exception instead of crashing.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17636
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 51 Pending, 3 Unrelated FailuresAs of commit 0521e2a with merge base cf08087 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical check-after-use bug in the JNI scalar type lookup code. The issue was that scalar_type_to_java_dtype.at(scalarType) was called before checking if the key exists in the map with .count(), causing the C++ code to throw an unhandled std::out_of_range exception and crash instead of throwing a proper Java exception.
Changes:
- Move the
.count()check before the.at()lookup to prevent crashes on unsupported scalar types - Update error message to display the actual scalar type value instead of an uninitialized
jdtypevariable - Enable proper Java exception handling for unsupported tensor scalar types
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove dead root ext properties (minSdkVersion, targetSdkVersion, compileSdkVersion, buildToolsVersion) that were unused by the module.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
throwExecutorchException only sets a pending Java exception and does not abort C++ execution. Without an early return, subsequent code (e.g. map::at()) can throw std::out_of_range before the JVM sees the pending exception.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
.count()checkbefore
.at()so unsupported scalar types throw a Java exception insteadof C++
std::out_of_rangeinitHybrid→numThreadsparameter name inModule.javanativedeclaration
getMethodMetadata()error messageroot ext properties (minSdkVersion, targetSdkVersion, compileSdkVersion,
buildToolsVersion), update test dependencies (junit 4.13.2,
androidx.test.ext:junit 1.2.1, androidx.test:rules 1.6.1,
commons-io 2.18.0)
Review order:
jni_layer.cpp→Module.java→build.gradlefiles.Test plan
sync succeeds