Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constprop split_rest on tuple and namedtuple #47328

Closed
wants to merge 2 commits into from

Conversation

jakobnissen
Copy link
Member

Julia 1.9 adds Base.split_rest, which can be used to slurp in non-final positions. However, unlike slurping in the final position, split_rest does not constant propagate, causing type instability in a pattern like head..., tail = args::Tuple.

This PR adds @constprop :aggressive to split_rest for tuples and named tuples, and tests that it constant folds as expected.

Issue discovered by @sgaure and fix proposed by @simeonschaub.
Closes #47326

Julia 1.9 adds Base.split_rest, which can be used to slurp in non-final
positions. However, unlike slurping in the final position, split_rest does not
constant propagate, causing type instability in a pattern like
`head..., tail = args::Tuple`.

This PR adds `@constprop :aggressive` to split_rest for tuples and named tuples,
and tests that it constant folds as expected.
@jakobnissen
Copy link
Member Author

I'm wondering if a better solution is to add the @constprop :aggressive in the lowering step, since the arguments when lowering to split_rest are always known at compile time, and there might be gains from constant propagation across the board instead of for tuple and namedtuple only, but that fix is beyond me.

@simeonschaub
Copy link
Member

Ah, beat me to it... 😄 This will have issues bootstrapping since @constprop isn't defined yet. Also, have you tested whether the annotation for the NamedTuple case is actually needed or whether the one for Tuple is already enough to fix this case too?

@jakobnissen
Copy link
Member Author

jakobnissen commented Oct 26, 2022

It is sufficient to annotate tuple, and it will still infer for namedtuple. I'll close this PR - can you add tests for namedtuple in your PR then?

@jakobnissen jakobnissen deleted the splitrest branch October 26, 2022 13:10
simeonschaub added a commit that referenced this pull request Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type instability in front slurping of tuples.
2 participants