Skip to content

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Aug 18, 2025

Resolves #59269.

Makes #59292 somewhat worse by doubling-down on the same assumptions.

We should straighten out that issue properly, but I don't see any reason not to get better inference in the mean time for the very common @Kwargs case. We are already relying on those assumptions in Base (as that issue demonstrates), and @JeffBezanson says that this functionality wasn't intended to be supported:

I think the use case for it is picking not a subset, but alternate key set, specifically linear or cartesian indices for arrays. [...] So go ahead and add that haskey method

edit: now also resolves #59292.

@topolarity topolarity added trimming Issues with trimming functionality or PR's relevant to its performance/functionality backport 1.12 Change should be backported to release-1.12 labels Aug 18, 2025
@JeffBezanson
Copy link
Member

Yes I think this is ok. Note Base.Pairs was not exported (though is now public). I think we should add an admonishment to the docstring for Pairs that the keys must match the keys of the data.

We may have thought of extending Pairs to allow e.g. subsetting keys, but obviously we are not going to implement that now. I think it really is out of scope and better suited to a different view-like type.

@JeffBezanson
Copy link
Member

Idea to consider: we could make this "safer" by making pairs(x::NamedTuple) return a Pairs{...,...,Nothing,typeof(x)} so we statically know the keys match the original namedtuple plus we save a pointer.

@KristofferC KristofferC mentioned this pull request Aug 19, 2025
27 tasks
@topolarity topolarity force-pushed the ct/const-eval-haskey branch from 76430fb to 7a38428 Compare August 19, 2025 22:16
@topolarity
Copy link
Member Author

Idea to consider: we could make this "safer" by making pairs(x::NamedTuple) return a Pairs{...,...,Nothing,typeof(x)} so we statically know the keys match the original namedtuple plus we save a pointer.

I tried this approach, and I think I like it better. .itr is a "private" field, so it should be safe for us to change it (keys(::Pairs) is unaffected)

That puts us in a better position to resolve #59292 (merge is easily fixed now, although values(::Pair) remains an issue)

Rather than assume the value of `.itr` for `Pairs{..., <:NamedTuple}`
this introduces `nothing` as a sentinel to iterate `.data` directly,
which is sufficient for this to const-prop.
This restricts this optimization to apply only to "fully-iterated"
NamedTuple Pairs, which are the only kind generated by lowering and
`Base.pairs`.

Resolves JuliaLang#59292.
@topolarity topolarity force-pushed the ct/const-eval-haskey branch from eff4126 to 3f20eec Compare August 20, 2025 00:32
@topolarity topolarity merged commit fa4972d into JuliaLang:master Aug 20, 2025
8 checks passed
topolarity added a commit that referenced this pull request Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 trimming Issues with trimming functionality or PR's relevant to its performance/functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base.merge gives incorrect results for Base.Pairs + NameTuple haskey(::@Kwargs{item::Bool}, :item) does not constant-fold
3 participants