[Hardik K (hardikkaurani)]

Summary

This PR hardens ScatterAllDynamic.check against invalid dimension lookups that could previously cause unhandled IndexError crashes during the rewrite pass.

It reuses the existing _ir_utils.get_dim() helper to safely retrieve dimensions while handling out-of-bounds axes, missing shape information, and rank-zero inputs.

Problem

In _redundant_scatter_nd.ScatterAllDynamic.check, unsafe dimension access could cause the rewrite pass to crash:

1. Out-of-bounds axis: Accessing the data tensor's dimension using an invalid axis_value could raise an unhandled IndexError.
2. Rank-zero transposed_data: Accessing dimension 0 when transposed_data is a scalar could also raise an IndexError.
3. Missing shape information: Unranked tensors without shape metadata could not provide the required dimensions for matching.

Solution

Updated ScatterAllDynamic.check to use the existing _ir_utils.get_dim() helper for safe dimension lookups:

• Retrieve the data dimension using _ir_utils.get_dim(data, axis_value).
• Retrieve the leading dimension of transposed_data using _ir_utils.get_dim(transposed_data, 0).
• Reject the match with result.fail(...) when either lookup cannot provide a valid dimension.
• Delegate negative-index handling and bounds validation to the existing helper instead of duplicating dimension-validation logic.

This prevents invalid dimension accesses from propagating as unhandled exceptions while preserving the existing rewrite behavior for valid inputs.

Tests

Added and updated regression tests in _redundant_scatter_nd_test.py:

• test_dynamic_indices_out_of_bounds_axis_rejected: Verifies that an out-of-bounds positive axis is safely rejected.
• test_dynamic_indices_negative_out_of_bounds_axis_rejected: Verifies that an out-of-bounds negative axis is safely rejected.
• test_dynamic_indices_valid_negative_axis_supported: Verifies valid negative-axis handling and numerical equivalence of the rewritten result.
• test_dynamic_indices_scalar_transposed_data_rejected: Verifies that scalar transposed_data is safely rejected.
• test_dynamic_indices_missing_data_shape_rejected: Verifies that unranked input data without shape metadata is safely rejected.
• test_dynamic_indices_missing_transposed_data_shape_rejected: Verifies that unranked transposed_data without shape metadata is safely rejected.

[Hardik K (hardikkaurani)]

Hi Ti-Tai Wang (@titaiwangms), Could you please review this PR when you have a moment? It fixes an IndexError crash by adding axis bounds validation and negative axis normalization in ScatterAllDynamic. All tests and lint checks are passing. Thank you for your time!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[Copilot]

🟢 Approval recommended

No active review issues remain for the ScatterAllDynamic axis-validation and scalar-input guard changes.

No open issues 🎉

_{Review effort: Balanced}

[Ti-Tai Wang (titaiwangms)]

The bounds and rank-zero guards address real IndexError paths. I found no blocking issue in the changed behavior during static review.

Two small simplification/documentation suggestions:

• In onnx-ir 1.1.0, Shape.rank() always returns an integer, so rank is None is unreachable once shape is known.
• IR Shape indexing already supports valid negative indices. Explicit normalization is harmless but not required; the actual bug fixed here is out-of-bounds indexing, not incorrect handling of valid negative axes.

There is also an existing repository helper, _ir_utils.get_dim(value, dim), that handles missing shapes, negative indices, and bounds checks. Using it for data at axis_value and transposed_data at dimension zero could avoid duplicating this logic, while preserving explicit match-failure diagnostics when either lookup returns None.

The new negative-axis test verifies that rewriting still happens; numerical equivalence coverage would strengthen it, but I do not consider that a blocker for these guards.

Validation: static review only, including the minimum supported IR implementation; tests were not run locally because onnx_ir is unavailable in this environment.

[Hardik K (hardikkaurani)]

Hi Ti-Tai Wang (@titaiwangms),

Thank you for the review and suggestions! I have updated the PR accordingly across five focused commits:

1. Use _ir_utils.get_dim for axis lookup: Refactored ScatterAllDynamic.check to retrieve the data dimension using _ir_utils.get_dim(data, axis_value). This avoids redundant rank is None guards and manual negative-axis normalization while preserving explicit match-failure diagnostics when the axis is out of bounds.
2. Use _ir_utils.get_dim for transposed_data: Refactored the leading dimension lookup and rank-0 guard on transposed_data to use _ir_utils.get_dim(transposed_data, 0).
3. Numerical equivalence test: Updated test_dynamic_indices_valid_negative_axis_supported to opset 17 and added numerical parity verification using testing.assert_numerically_equal.
4. Missing data shape regression test: Added test_dynamic_indices_missing_data_shape_rejected to verify unranked input data tensors without shape metadata are safely rejected.
5. Missing transposed_data shape regression test: Added test_dynamic_indices_missing_transposed_data_shape_rejected to verify unranked transposed data tensors are safely rejected.

All 16 tests in _redundant_scatter_nd_test.py and ruff formatting and lint checks pass cleanly.

[Ti-Tai Wang (titaiwangms)]

The revised implementation appropriately reuses _ir_utils.get_dim() for safe dimension lookup, and the added numerical-equivalence and missing-shape tests strengthen coverage. No blocking findings remain in my static review.

Please update the PR description to match the final implementation: bounds checking and negative-index handling are delegated to get_dim(), rather than implemented with an explicit rank-is-None check and axis-normalization branch. Valid negative indices were already supported by IR Shape indexing; the substantive fixes are the out-of-bounds and rank-zero crash guards.

Approval is based on static review; local tests were not run. Please ensure the required CI checks pass before merging.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.32%. Comparing base (682b59d) to head (4a8434a).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3146      +/-   ##
==========================================
+ Coverage   73.29%   73.32%   +0.03%     
==========================================
  Files         267      269       +2     
  Lines       33214    33481     +267     
  Branches     3183     3194      +11     
==========================================
+ Hits        24344    24551     +207     
- Misses       7801     7868      +67     
+ Partials     1069     1062       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.