Add typechecker handling for put_elem#15377
Conversation
surface badtuple, badindex in dynamic require dynamic upper bound eliminate negations before replacing stronger difference tests fixed off by one in dynamic pruning in delete_element
328f1fc to
af6b56a
Compare
|
You already did what I was going to suggest :D I agree with the I'm wondering about |
| :error -> | ||
| tuple_replace_at_checked(descr, index, type) | ||
|
|
||
| {dynamic_type, _static} -> |
There was a problem hiding this comment.
I wonder if dropping the static part is right here. insert_element does drop it while delete_element preserves
There was a problem hiding this comment.
insert_element should be changed to preserve it, this is a loss of precision
There was a problem hiding this comment.
It sounds like something for a follow up PR
| assert tuple_delete_at(dynamic(tuple([atom([:a]), atom([:b])])), 2) == :badindex | ||
| assert tuple_delete_at(dynamic(tuple([atom([:ok])])), 1) == :badindex | ||
|
|
||
| # Deletion is not injective at the deleted position; the negative |
There was a problem hiding this comment.
Tests in 1819-1829 do not assert any new behaviour added in this PR but were failing pre #15376
There was a problem hiding this comment.
You can get rid of them, this was tested in the PR.
|
|
||
| assert tuple_insert_at(dynamic(tuple([atom([:ok])])), 2, binary()) == :badindex | ||
|
|
||
| # Even at index 0 (where the size constraint is vacuous) the dynamic |
There was a problem hiding this comment.
Tests in 1917-1925 do not assert any new behaviour added in this PR but were failing pre #15376
There was a problem hiding this comment.
Also tested in the PR, can remove. :)
|
@lukaszsamson can you please break this into multiple PRs?
|
This PR adds missing handling for
put_elem.The initial implementation and tests in d6213f5 was heavily based on existing implementations of
insert_elementanddelete_element. However, I later discovered and reproduced in tests some soundness issues that affected bothsetelementandinsert_elementanddelete_elementwhich were fixed in commits 1c58013 and 328f1fcSummary of issues:
Tuple.insert_at/3%{dynamic: :badindex}/%{dynamic: :badtuple}return instead of propagating the atom valueout of bound insert into dynamic closed tuple returned- Already addressed in Tuple operations performance boost + fix logic bug #15376dynamic(none())instead of:badindexTuple.delete_at/2off by one in dynamic closed tuple, returned same size instead of size - 1- Already addressed in Tuple operations performance boost + fix logic bug #15376:badindexnegative constraint on deleted index was preservedinstead of dropping to- Already addressed in Tuple operations performance boost + fix logic bug #15376tuple([atom()])Also I noticed invalid warnings emitted for negative indices affecting all tuple ops:
Fixed in 1c58013
@gldubc Note this PR partially overlaps with #15376 so you may want to include my changes into yours
I can split it into smaller PRs if this one is too big
AI use disclosure
Assisted-by: Claude Opus:4.7
Assisted-by: GPT:5.5
Fixes #15375
Edit: Rebased and updated since #15376 is already merged