apple / pkl

A configuration as code language with rich validation and tooling.
https://pkl-lang.org
Apache License 2.0
9.85k stars 258 forks source link

Remove hidden modifier from many reflect properties for better equality/rendering #470

Closed stackoverflow closed 2 months ago

stackoverflow commented 2 months ago

This is a double-edged sword. Quite a few of these (in other classes in this module) were made hidden to make the rendered output more readable and/or to prevent stack overflows. I'm not entirely sure whether this makes for better rendering and it only makes equality "come closer to correct." The hiddens you remove in this PR clearly don't introduce stack overflows, but it does seem a little arbitrary to just unhide what we happen to can without issues and leave other things hidden.

I think the deeper problem is that equality is somewhat broken and I'm wondering whether we shouldn't "just" fix that. I think we can't generally fix equality because one can always define recursive, non-converging infinite amends chains, which make the problem undecidable. One of the reasons for the choice to only observe visible properties for equality was due to cycles (any instance of which would otherwise be a stack overflow). Keeping a stack of seen objects (using native JVM object identity) should help and then we can always give it a maximum recursion depth.

WDYT?

I agree that conflating equality with rendering is not ideal, but the solution to that is beyond the scope of this PR. This is just a bugfix to an issue I created when I fixed the stack overflow rendering reflected values. As reflected values are not really configuration and mostly rendered for debug purposes I think it's fine to make everything we can not hidden. If/when we implemented a smart equality calculation we can unhide even more things. The important part here is making SourceLocation not hidden and thus, part of equality, as locations are always unique.