aio-libs / aio-libs/aiobotocore

Investigate AST-level hashing in test_patches.py to distinguish substantive from cosmetic changes

Abierto
#1,566 0 comentarios 0 reacciones 0 asignados Ver en GitHub
enhancement
Lenguaje dominante
Python
Estrellas
1.4k
Forks
209
Merge medio
4 d 15 h
PR fusionados (30 d)
16

Descripción

## Summary

`tests/test_patches.py` currently hashes botocore function **bytes** (SHA1 of source text). This catches every change — substantive behavior shifts AND cosmetic edits (docstrings, whitespace, type hints, import reorder) alike. The result: contributors can't tell from a hash-fail alone whether a real port is needed or just a hash bump.

Proposal: add an **AST-level hash alongside the existing byte hash**. Two signals, different intents:

| Hash | What it catches | Enforcement meaning |
|-|-|-|
| **Byte hash (existing)** | All changes, including cosmetic | Aiobotocore should mirror 1:1 for diff minimality (the repo's core principle). A mismatch means the override may be out of sync even for docstrings/whitespace. |
| **AST hash (new)** | Substantive changes only — signature, control flow, call structure | A mismatch means a real functional port is needed, not just a mechanical mirror. |

With both, a pure-docstring botocore change busts the byte hash but not the AST hash, so the contributor knows to apply the cosmetic mirror + bump the byte hash but doesn't need to do behavioral porting work. A real body change busts both, signaling both mirror and port.

## Why this matters now

- The `/aiobotocore-bot:check-async-need` classifier distinguishes substantive from cosmetic per Step 2/3 (refined in #1565). The byte hash doesn't have that distinction baked in.
- A recent case (PR #1480) had a mix: botocore's `_apply_request_trailer_checksum` had a real body change AND adjacent classes got docstrings/type hints. The byte hash fires on both; the classifier now correctly only port-flags the body change. An AST hash would let the test suite encode the same distinction.
- Contributors doing a relax upgrade (no-port) with only cosmetic botocore changes upstream currently have to update hashes for no functional reason — looks like a port but isn't.

## Proposed approach (sketch)

For each entry in `test_patches.py` track two hashes:

```python
# before
'hash': 'a1b2c3...' # SHA1 of source bytes

# after
'byte_hash': 'a1b2c3...' # unchanged; mirrors minimality principle
'ast_hash': 'd4e5f6...' # normalized AST hash; behavior-only signal
```

AST normalization:

1. Load the botocore source module.
2. Extract the function/class via `ast.parse` → find by name.
3. Normalize the AST:
- Strip module-level and function-level docstrings (first `Expr(Constant(str))`).
- Strip pure type annotations (`ast.arg.annotation`, `FunctionDef.returns`).
- Unparse back to source with a canonical formatter (`ast.unparse`).
4. Hash the normalized unparsed source.

On test failure, the contributor sees which hash(es) failed and gets a clear signal: byte-only = cosmetic mirror + hash bump, both = real port needed.

## Risks / considerations

- **False negatives**: an AST-equivalent refactor (`if cond: return a\nreturn b` → `return a if cond else b`) busts the byte hash but equals the AST hash. Usually that's fine — behavior is the same — but edge cases exist (walrus-in-expr-context, tuple-unpacking reorder). Worth auditing the worst 5-10 AST-equivalence classes against aiobotocore's actual overrides before committing.
- **Ordering stability**: `ast.unparse` output can vary across Python versions. Pin the hashing-Python version in `test_patches.py` or bake it into the normalization.
- **Debuggability**: when the AST hash fails, the raw byte diff is harder to map to. Need a small diff tool that shows normalized forms side-by-side on failure.

## Non-goals

- Replacing the byte hash. Aiobotocore's "minimize divergence from botocore" principle (see docs/override-patterns.md) means byte-level fidelity matters — the AST hash is additive signal, not replacement.
- Replacing the check-async-need classifier. The hash tests and the classifier serve different purposes (mechanical coupling vs async-need judgment). All three signals should coexist.
- Changing aiobotocore's override model. The hashing is CI-side enforcement, not a runtime concern.

## Related

- PR #1563 introduced `/aiobotocore-bot:check-async-need` which encodes similar logic in prompt form.
- PR #1565 refined the classifier's "substantive change" criterion after a real miss on PR #1480.
- `CONTRIBUTING.rst` §"Hashes of Botocore Code" is the current documentation for test_patches.py behavior.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.