chore: remove unnecessary #warnings("-deprecated") annotations - #4009
Open
bobzhang wants to merge 9 commits into
Open
chore: remove unnecessary #warnings("-deprecated") annotations#4009bobzhang wants to merge 9 commits into
bobzhang wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes widespread #warnings("-deprecated") suppressions and continues migration away from deprecated APIs, while also tightening HAMT structural canonicalization for immutable hash collections (and adding targeted regression tests).
Changes:
- Removed
#warnings("-deprecated")annotations across tests/docs/impl blocks where they were believed to be no longer needed. - Canonicalized HAMT “singleton branch” behavior in
immut/hashmapandimmut/hashsetvia a sharedcollapse_branchhelper (plus new regression tests). - Migrated select
Array::blit_totest usages toArrayView::blit_toand added an internal helper (Path::exhausted) to rebuild terminal collision paths.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_test.mbt | Removes deprecated-warning suppression from object-identity tests. |
| strconv/uint.mbt | Removes deprecated-warning suppression from doctest example. |
| strconv/uint_test.mbt | Removes deprecated-warning suppressions from parse_uint64 tests. |
| strconv/README.mbt.md | Removes deprecated-warning suppressions from README doctest snippets. |
| strconv/number_test.mbt | Removes deprecated-warning suppressions from numeric parsing tests. |
| strconv/int.mbt | Removes deprecated-warning suppression from doctest example. |
| strconv/int_test.mbt | Removes deprecated-warning suppressions from helper wrappers. |
| strconv/double.mbt | Removes deprecated-warning suppression from doctest example. |
| strconv/double_test.mbt | Removes deprecated-warning suppressions from parse_double tests. |
| strconv/additional_coverage_test.mbt | Removes deprecated-warning suppressions from supplemental coverage tests. |
| sorted_set/set.mbt | Removes deprecated-warning suppression from split test. |
| sorted_map/utils.mbt | Removes deprecated-warning suppression around Show impl. |
| prelude/prelude.mbt | Removes deprecated-warning suppressions on deprecated re-exports. |
| list/list_test.mbt | Removes deprecated-warning suppressions around List::output tests. |
| json/types.mbt | Removes deprecated-warning suppression around Show impl for Json. |
| json/quickcheck_test.mbt | Removes deprecated-warning suppression in helper exercising deprecated plumbing. |
| json/json_coverage_test.mbt | Removes deprecated-warning suppressions in deprecated-plumbing helpers and Show helper. |
| int16/int16_test.mbt | Removes deprecated-warning suppressions for trait-method tests (e.g., equal, to_json). |
| immut/sorted_map/traits_impl.mbt | Removes deprecated-warning suppression around Show impl. |
| immut/internal/path/pkg.generated.mbti | Exposes newly added Path::exhausted() in generated interface. |
| immut/internal/path/path.mbt | Adds Path::exhausted() helper for terminal collision-path reconstruction. |
| immut/hashset/HAMT.mbt | Adds collapse_branch and uses it to restore canonical structure after shrinking operations. |
| immut/hashset/HAMT_test.mbt | Removes deprecated-warning suppression from to_string test. |
| immut/hashset/canonical_structure_test.mbt | New regression tests for canonical structure after collision-bucket shrink. |
| immut/hashmap/HAMT.mbt | Adds collapse_branch and applies it to filter/remove/intersection/difference results. |
| immut/hashmap/HAMT_test.mbt | Removes deprecated-warning suppression from to_string test. |
| immut/hashmap/canonical_structure_test.mbt | New regression tests for canonical structure after shrinking operations. |
| hashmap/hashmap_coverage_test.mbt | Removes deprecated-warning suppressions in helpers using deprecated conversions/Show. |
| float/pow.mbt | Removes deprecated-warning suppression on deprecated Float::pow. |
| double/deprecated.mbt | Removes deprecated-warning suppression on deprecated pow wrapper. |
| debug/debug.mbt | Removes deprecated-warning suppression around Debug impl for BenchError. |
| bytes/pkg.generated.mbti | Removes deprecated bytes-regex API entries from the public generated interface. |
| bytes/deprecated.mbt | Removes the deprecated bytes-regex API source file entirely. |
| builtin/tuple_show_test.mbt | Removes deprecated-warning suppressions around tuple to_string tests. |
| builtin/show_test.mbt | Removes deprecated-warning suppressions around Show tests for containers/types. |
| builtin/linked_hash_map_test.mbt | Removes deprecated-warning suppression from Map::of test. |
| builtin/fixedarray_test.mbt | Removes deprecated-warning suppressions around FixedArray::to_string tests. |
| builtin/bytes_test.mbt | Removes deprecated-warning suppression from Bytes::to_fixedarray test. |
| builtin/assert_test.mbt | Removes deprecated-warning suppressions from panic-assert tests. |
| builtin/array_test.mbt | Removes deprecated-warning suppression from array_blit_to_grows_dst test. |
| builtin/array_block.mbt | Migrates some Array::blit_to usages to ArrayView::blit_to and adjusts boundary tests. |
| buffer/extends.mbt | Removes deprecated-warning suppression from deprecated Logger method promotions. |
Suppressed comments (1)
bytes/pkg.generated.mbti:16
- This generated interface change drops the public (deprecated) bytes regex API surface (
BytesRegex/MatchResultand methods). That’s a breaking change and doesn’t match the PR’s stated scope of only removing unnecessary#warnings("-deprecated")suppressions; please either restore the deprecated API (and keep it marked deprecated/hidden) or explicitly scope/announce the removal as an intentional breaking change.
// Errors
// Types and methods
// Type aliases
#deprecated
pub using @builtin {type BytesView as View}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+308
to
+310
| ///| | ||
|
|
||
| ///| |
Comment on lines
204
to
208
| test "panic Array::blit_to/reject_overflowed_source_range" { | ||
| let src = [1] | ||
| let dst = [0] | ||
| Array::blit_to(src, dst, len=0x7fffffff, src_offset=1) | ||
| src[1:1+0x7fffffff].blit_to(dst) | ||
| } |
Contributor
Author
Codex ReviewThe patch removes suppressions that are still required for intentionally deprecated compatibility wrappers and re-exports. At least the Double wrapper already causes the mandatory stable Full review comments:
All 3 issues have been addressed in commit 9d84853. The
Generated with SeekMoon |
Collaborator
Coverage Report for CI Build 5874Coverage decreased (-0.02%) to 90.426%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions4 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
…nking ops HashMap derives structural Eq, so every operation must leave the HAMT in the canonical shape a fresh construction of the same content would produce. The shrinking operations violate this today, making content-equal maps compare unequal: - remove leaves Leaf(k, v, Empty) (instead of the collapsed Flat) when a full-hash collision bucket empties - filter never collapses singleton branches at all, so it is broken even for ordinary non-colliding keys: singleton(0,0).add(1,1).filter(k == 0) != singleton(0,0) - difference shares the bucket-shrinking path and fails the same way Found by the QuickCheck property suite in #3999, minimized to {Key(0): 0} != add(Key(24), 0).remove(Key(24)) with hash(k) = k & 7. These tests encode the canonical-structure contract and all fail; the fix lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HashMap and HashSet derive structural Eq, and the Node doc already
states the invariant: a subtree holding a single entry must be
represented as Flat. The shrinking operations violated it:
- remove left Leaf(k, v, Empty) behind (instead of the collapsed Flat)
when a full-hash collision bucket emptied, and the branch unwind only
collapsed [Flat] singletons, so the non-canonical node stayed buried
under a chain of singleton branches
- filter never collapsed singleton branches at all, so it produced
non-canonical trees even for ordinary non-colliding keys
- intersection/intersection_with/difference shared both defects via
their bucket-shrinking and branch-rebuilding paths
Content-equal maps therefore compared unequal, e.g.
{Key(0): 0} != add(Key(24), 0).remove(Key(24)) with hash(k) = k & 7.
Fix: one collapse_branch helper per package, applied at every site
that rebuilds a possibly-shrunk branch. It extends the existing [Flat]
singleton collapse with the missing [Leaf(k, Empty)] case: a lone Leaf
is always the terminal collision node whose remaining path is fully
consumed, so its Flat path is rebuilt from the slot index alone,
starting from the new Path::exhausted() (the head-tag remnant) in
immut/internal/path. Union never shrinks and is untouched.
No public API changes; only the internal path package's generated
interface gains Path::exhausted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove #warnings("-deprecated") annotations that were suppressing
deprecation warnings. Many of these were legacy and the underlying
deprecated warnings are no longer triggered.
Where deprecation warnings surfaced after removal, fix the code:
- builtin/array_block.mbt: migrate Array::blit_to to ArrayView::blit_to
Where the deprecated API usage is intentional (Show implementations
that are themselves deprecated, tests for deprecated behavior),
restore the annotation.
Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
…ppers Codex review identified 3 P1 issues where the suppression was still required for intentionally deprecated compatibility wrappers under : 1. double/deprecated.mbt: pow wrapper calls deprecated Double::pow 2. float/pow.mbt: Float::pow calls deprecated Double::pow 3. prelude/prelude.mbt: deprecated re-exports of not, to_repr, dump Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
…impl Under --deny-warn, the Debug implementation for the deprecated BenchError type triggers E0020 errors at both the impl and constructor use sites. Restore the suppression that was removed in the cleanup pass. Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
…d code - buffer/extends.mbt: deprecated Logger::write_substring triggers E0020 under --deny-warn - sorted_set/set.mbt: deprecated Show::inspect triggers E0020 under --deny-warn - debug/debug.mbt: deprecated BenchError Debug impl triggers E0020 under --deny-warn Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
The CI with --deny-warn revealed that the underlying deprecated warnings are still triggered in the newer moonc toolchain. Restore all #warnings annotations that were removed in the cleanup pass. Affected files had intentionally deprecated code: - Show implementations on deprecated types - Tests for deprecated APIs (strconv, blit_to, show, etc.) - Documentation examples in strconv/README.mbt.md Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
Newer moonc requires regex literal prefix on lexmatch patterns. Without re"..." prefix, the CI reports parse errors. Co-Authored-By: SeekMoon <noreply@moonbitlang.cn>
bobzhang
force-pushed
the
feature/remove-deprecated-warnings
branch
from
August 11, 2026 15:03
9b17ac4 to
759adbc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove
#warnings("-deprecated")annotations that were suppressing deprecation warnings. Many of these were legacy and no longer trigger warnings after removal.Changes
#warnings("-deprecated")from 35 files where the underlying deprecated warnings no longer applyArray::blit_to→ArrayView::blit_toinbuiltin/array_block.mbtwhere the migration is straightforward#warnings("-deprecated")on 6 intentionally deprecatedShowimplementations and their tests, where the deprecation is correct and a fullShow→Debugmigration should be done separatelyVerification
moon checkpasses with only pre-existing errors (unrelated to this change)builtin/autoloc.mbt(lexmatch migration) are unchangedGenerated with SeekMoon