Skip to content

Add typechecker handling for put_elem#15377

Closed
lukaszsamson wants to merge 4 commits into
elixir-lang:mainfrom
lukaszsamson:ls-put-elem-type
Closed

Add typechecker handling for put_elem#15377
lukaszsamson wants to merge 4 commits into
elixir-lang:mainfrom
lukaszsamson:ls-put-elem-type

Conversation

@lukaszsamson
Copy link
Copy Markdown
Contributor

@lukaszsamson lukaszsamson commented May 14, 2026

This PR adds missing handling for put_elem.

The initial implementation and tests in d6213f5 was heavily based on existing implementations of insert_element and delete_element. However, I later discovered and reproduced in tests some soundness issues that affected both setelement and insert_element and delete_element which were fixed in commits 1c58013 and 328f1fc
Summary of issues:

Also I noticed invalid warnings emitted for negative indices affecting all tuple ops:

    warning: expected a tuple with at least -1 elements in Tuple.insert_at/3:
        Tuple.insert_at({1, 2}, -1, :x)
    the given type does not have the given index:
        {integer(), integer()}

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

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
@gldubc
Copy link
Copy Markdown
Member

gldubc commented May 14, 2026

You already did what I was going to suggest :D I agree with the tuple_insert_at changes in descr.ex.

I'm wondering about replace_at. It's true that, before, it couldn't be implemented via delete + insert, because delete was broken. But now, it should just work.

:error ->
tuple_replace_at_checked(descr, index, type)

{dynamic_type, _static} ->
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if dropping the static part is right here. insert_element does drop it while delete_element preserves

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert_element should be changed to preserve it, this is a loss of precision

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like something for a follow up PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take care of it.

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
Copy link
Copy Markdown
Contributor Author

@lukaszsamson lukaszsamson May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests in 1819-1829 do not assert any new behaviour added in this PR but were failing pre #15376

Copy link
Copy Markdown
Member

@gldubc gldubc May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests in 1917-1925 do not assert any new behaviour added in this PR but were failing pre #15376

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested in the PR, can remove. :)

@josevalim
Copy link
Copy Markdown
Member

@lukaszsamson can you please break this into multiple PRs?

  1. No need to implement tuple_replace_at, we will do that ourselves
  2. Any soundness issue should be a separate PR/issue
  3. For the negative index, a PR is welcome. However, it should not be handled in the descr, it should be handled in apply before we call the descr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

put_elem not emitting typechecker warnings

3 participants