Add showrepr battery for short Base.show#21
Conversation
showrepr battery: heuristic short, recreatable Base.showshowrepr battery for short Base.show
d4b4c0b to
bbc9414
Compare
|
Thanks for merging the other PRs! I rebased this PR on top of Questions which might guide the review:
|
There was a problem hiding this comment.
A couple of thoughts.
- The implementation definitely makes me nervous. I don't love iterating on methods. I wonder could we add JET + Aqua to the tests in a separate PR before this one?
- I wonder if we should have a mechanism for the user to add their own definitions to our macros without having to touch StructHelpers.jl. For instance, this feature could be added in user space by doing something like:
struct ShowRepr <: AbstractBatteriesKeyword end
const showrepr = ShowRepr()
function StructHelpers.to_expr(::ShowRepr, T)::Expr
...
end- As you suggest lets put this into a separate file.
bbc9414 to
1cab327
Compare
| with: | ||
| version: ${{ matrix.version }} | ||
| arch: ${{ matrix.arch }} | ||
| - name: Drop JET deps on Julia versions JET does not support |
There was a problem hiding this comment.
Yikes, this will also bite when running tests locally, right?
Maybe we should just have a workflow step separate from running tests that just runs JET?
There was a problem hiding this comment.
Well, it is unfortunately a trade-off currently, but running the tests locally is exactly the reason why I went this route.
With the approach proposed in the current PR, you can simply run your tests locally on Julia 1.10 and 1.12 and you'll get the JET results. Note that Julia 1.10 uses an older JET version, but this is the most recent JET version which supports Julia 1.10.
You can also test on Julia 1.13 if you add the branch described in this comment to your default environment and start the test from there.
I don't know whether Julia 1.6 and JET can cooperate, as my default environment is not fully functional due to using custom registries and I didn't want to spend additional time to work around this. But I think it's unlikely that you'd learn something about useful code changes there. And CI is not worse than before the JET PR.
Julia nightly, is currently just not supported in JET.
With the separate workflow you'd get no local JET testing at all. You could surely always explicitly test it with JET, but we all know how easy it is to forget about this. With the approach from this PR all the most important Julia versions (1.10, 1.12, 1.13) can be tested locally.
It's unfortunate that the situation around JET is currently so tricky. You can follow the above link and the linked discussion therein to see that currently there does not seem to be a better solution. I don't know of a better tradeoff.
JET's registry compat currently requires 1.7 <= Julia <= 1.12, so Pkg cannot resolve JET on Julia 1.6 and nightly. To get a green CI, strip JET from Project.toml and runtests.jl on the Julia versions not supported by JET. This means only the 'latest stable' job actually has JET coverage on CI.
JET 0.11's flow analysis can't prove that the two reads of `fieldnames` (under `if nt.getproperties` and `if nt.kwconstructor`) are reachable only when `need_fieldnames` (the assignment guard) is true. Assign `fieldnames` unconditionally with a cheap fallback so the binding is always defined. Co-authored-by: Copilot <copilot@github.com>
@Batteries derives a curated set of methods by default; @battery does the opposite — every default flips to false, so only the explicitly listed batteries are derived. The bare-flag shorthand from the previous PR makes call sites read as a checklist: @battery T kwconstructor # only the kwarg constructor @battery T eq isequal hash # only structural ==/isequal/hash @battery T hash typesalt=0xab # only stable hash This addresses the side-effect concern with subset use cases of @Batteries (\`@batteries T showrepr=true\` silently also defines ==, hash, isequal, getproperties, constructorof, selfconstructor): users who want exactly one or two batteries can now ask for just those, with no surprise additions now or additional surprises in the future when the defaults change. @enumbattery is the analogous opt-in form of @enumbatteries. Implementation factors the macro bodies into shared codegen helpers that take the base named tuple. The 'all-false' bases are derived programmatically from the existing DEFAULTS so they stay in sync if a new option is added later. Co-authored-by: Copilot <copilot@github.com>
Closes jw3126#12. This is the real PR I wanted to do. All the others are basically there to enable this one. Introduces a new `@batteries` / `@battery` flag `showrepr=true` that overloads `Base.show` to print a heuristically short constructor call which round-trips through `eval`, as an alternative to the existing `kwshow`. The two are mutually exclusive (both target `Base.show`). I originally wanted to just extend `kwshow`. However, as the redundancy reduction should also be available if there is no keyword constructor and/or calls are short when using a parametric constructor, this would have lead to broken backwards-compatibility requiring a major version bump and user changes. But as both approaches have different advantages, it's probably a good thing to have both in the future. Highlights: * Probes every constructor of the type — positional, keyword, and hybrid (including the pair synthesized by `Base.@kwdef`) — and picks the shortest call whose result is `repr_eq` to the original. Trailing fields whose values match a default are omitted. * Per-field literal shortening where the constructor still accepts the substitute: `0x000000000000002a` → `42`, whole-valued floats drop the `.0` suffix, `2//1` → `2`, `2 + 0im` → `2`. * Uniform / run-length-compressed vectors render as `fill(v, n)` (or `[v for _=1:n]` for non-`isbits` elements, to avoid aliasing on re-eval); heterogeneous vectors get per-run RLE only when it actually shortens the literal. * Round-trip safety is the gating criterion throughout: every substitution is verified by reconstructing through the constructor and comparing via `repr_eq` (which accepts both `==` and `isequal` to handle `NaN`, `-0.0`, and custom equality). * Falls back to a non-executable `T(field = value, …)` rendering when no constructor recreates the object. `kwshow` keeps its existing fixed `T(f1 = v1, …)` shape and stability guarantees; `showrepr`'s output is heuristic and not pinned across minor releases. The README contrasts the two. This builds on jw3126#18 and jw3126#19 which should be merged first. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
JET (via report_package on downstream packages) flagged \`local variable fieldnames may be undefined\` at the call sites that pass \`fieldnames\` into \`def_getproperties\` and \`def_kwconstructor\`. The previous code only bound \`fieldnames\` inside \`if need_fieldnames\`, and JET could not correlate the guard with the later uses. Bind it unconditionally (with \`()\` as a harmless fallback when not needed) so the variable is always in scope. Co-authored-by: Copilot <copilot@github.com>
1cab327 to
20e8667
Compare
Yes, I also hesitated a bit initially. But in the end, I think it really makes sense: We want to find the best constructor, i.e. the constructor which generates the object with the least amount of code. So iterating on all constructors and checking that they work seems to be the generic solution. Of course we could limit it to
Currently, the batteries are defined statically. I can imagine that it might make sense to use a However, I am not yet sure whether the benefit would be large. The idea was to have the Or looking at it from a different angle: Writing your own macro to implement the pendant to a single battery is probably not so much more effort than using an interface of StructHelpers to add an additional battery. So the interface is probably only worth having when it significantly reduces the user effort. I am not saying that this is not the case, but I am also not yet convinced that it is. And we would need to have an answer to the question whether we want to distribute the batteries to different packages to still have them reusable.
done |
|
I think you are right, thanks for this PR. It is really a big improvement on what gets shown for big structs. |
|
Thanks for your patience, your openness to discuss topics, the improvements due to you review findings and for creating and maintaining |
|
I personally think this is quite a dangerous option... Can it at least get a prominent warnings in the docs? julia> struct RandCtor
x::Int
end
julia> RandCtor() = RandCtor(rand(1:2))
julia> @batteries RandCtor showrepr=true
julia> obj = RandCtor(1)
# sometimes shows correctly:
julia> obj
RandCtor(1)
# sometimes not:
julia> obj
RandCtor()julia> struct Cfg
n::Int
end
julia> function Cfg()
@info "loading defaults from some file"
Cfg(42)
end
julia> @batteries Cfg showrepr=true
# just printing the object would touch filesystem
julia> o = Cfg(7)
[ Info: loading defaults from some file
Cfg(7)
julia> o
[ Info: loading defaults from some file
Cfg(7)julia> mutable struct AbstractBox
x::Number # abstract: holds whatever the user passes
end
julia> @battery AbstractBox showrepr=true
# parsing the show result is not equal to original:
julia> AbstractBox(1.0)
AbstractBox(1)
julia> AbstractBox(1) == AbstractBox(1.0)
false |
|
Please note, that the third problem already exists in struct MyStruct a end
o = 42 |> Int32 |> MyStructjulia> o.a |> typeof
Int32julia> (o |> repr |> Meta.parse |> eval).a |> typeof
Int64I addressed the issues in #28. Thanks again for the findings! |
I didn't just mean |
Sorry, @aplavin, I seem to be missing something. In my example the results are not julia> struct MyStruct a end
julia> o = 42 |> Int32 |> MyStruct
MyStruct(42)
julia> o |> repr |> Meta.parse |> eval == o
falseBut anyway, I think your overall finding is correct that this is surprising. And I think #28 is doing the right thing: We can only rely on |
Closes #12.
This is the real PR I wanted to do. All the others are basically there
to enable this one.
Introduces a new
@batteries/@batteryflagshowrepr=truethatoverloads
Base.showto print a heuristically short constructor callwhich round-trips through
eval, as an alternative to the existingkwshow. The two are mutually exclusive (both targetBase.show).I originally wanted to just extend
kwshow. However, as theredundancy reduction should also be available if there is no keyword
constructor and/or calls are short when using a parametric
constructor, this would have lead to broken backwards-compatibility
requiring a major version bump and user changes. But as both
approaches have different advantages, it's probably a good thing
to have both in the future.
Highlights:
hybrid (including the pair synthesized by
Base.@kwdef) — and picksthe shortest call whose result is
repr_eqto the original. Trailingfields whose values match a default are omitted.
substitute:
0x000000000000002a→42, whole-valued floats drop the.0suffix,2//1→2,2 + 0im→2.fill(v, n)(or[v for _=1:n]for non-isbitselements, to avoid aliasing onre-eval); heterogeneous vectors get per-run RLE only when it actually
shortens the literal.
substitution is verified by reconstructing through the constructor
and comparing via
repr_eq(which accepts both==andisequaltohandle
NaN,-0.0, and custom equality).T(field = value, …)rendering whenno constructor recreates the object.
kwshowkeeps its existing fixedT(f1 = v1, …)shape and stabilityguarantees;
showrepr's output is heuristic and not pinned acrossminor releases. The README contrasts the two.
This builds on #18 and #19 which should be merged first.