janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.43k stars 221 forks source link

Improve consistency of b-tuples and p-tuples #1474

Closed pyrmont closed 1 month ago

pyrmont commented 1 month ago

This PR improves the consistency of how Janet handles bracket-delimited tuples (b-tuples) and parenthesis-delimited tuples (p-tuples). The primary change is that b-tuple literals (e.g. [:foo :bar]) are compiled into b-tuples. In connection with this, several functions in core that return tuples are changed to return b-tuples rather than p-tuples.

Background

B-tuples and p-tuples were originally equivalent (and interchangeable) expressions of immutable indexed data structures. The key difference was that literal p-tuples would be interpreted by the compiler as being function calls. Over time this has changed; most notably when aa5c987 changed the semantics of tuple equality to address #1073. After this change, b-tuples were no longer equal to p-tuples that contained equivalent values.

Unfortunately, this has led to a status quo that is confused and confusing. B-tuple literals in source code are compiled into p-tuples (and printed as such) and are not equal to tuples created with the tuple/brackets function . You can see this with the following code:

(def t [:foo :bar])
# => (:foo :bar)
(tuple/type t)
# => :parens
(= [:foo] (tuple/brackets :foo)
# => false

This PR changes the results of the above code to:

(def t [:foo :bar])
# => [:foo :bar]
(tuple/type t)
# => :brackets
(= [:foo] (tuple/brackets :foo)
# => true

Implementation

This PR largely completes the logic of treating b-tuples and p-tuples separately. In summary, it does the following:

  1. The Janet compiler is changed to compile b-tuple literals into b-tuples.

  2. Macros in boot.janet that took advantage of the (counterintuitive) fact that b-tuple literals are compiled into p-tuples have been changed.

  3. A new function, tuple/toggle, is introduced which can toggle a tuple between different types.

  4. Functions in core which return an indexed collection of results are changed to return b-tuples rather than p-tuples. Notably, functions which take an indexed collection and return a new tuple are not changed to return b-tuples (e.g. take, partition, etc). Arguably, these should also be changed so that the only function that produces p-tuples are tuple and (perhaps) tuple/slice.

It is left as a suggestion that tuple/slice be updated to take as an additional argument that would either set the type of tuple produced or at least made to be consistent with the input collection (if the input collection is a tuple).

sogaiu commented 1 month ago

I like the improved consistency.

ianthehenry commented 1 month ago

This is a verrrry breaking change, so I dunno how realistic it would be to actually merge, but I am very in favor of changing the "default" bracketedness.

But I think the current state of this PR is confusing though -- I would expect [1 2 3] and (tuple 1 2 3) to mean exactly the same thing; I think of square brackets as just syntax sugar for the tuple function and I think it should stay that way. I'd just exactly invert the current semantics: all tuples are :brackets by default; you have to go out of your way to create a :parens tuple. I would get rid of the tuple/brackets function altogether (as it's redundant with tuple), and add a new tuple/form function instead to create parenthesized tuples.

(This is slightly more breaking as there are places that might use (tuple 'def 'x 1) in macros instead of ['def 'x 1], but I think that the consistency between tuple and square bracket syntax is more important.)

But this is a big change that has the potential to break any code that defines macros. It will definitely break some code that I've written, although I don't think very much -- I tend to prefer quasiquote even if it means unquoting everything. But I dunno how to coordinate e.g. updating spork, which depends on this behavior.

(I am also very in favor of tuple/slice preserving the bracketedness of its input.)

pyrmont commented 1 month ago

@ianthehenry I like the idea of simpler changes (@bakpakin can vouch that my 'solutions' are almost always over-engineered) but doesn't what you're suggesting still break macros? The compiler doesn't compile b-tuples into function calls so if b-tuple literals compile to b-tuples, then macros of the form ['def 'x 1] will just emit tuples rather than functions.

ianthehenry commented 1 month ago

Yep! My proposal is less backwards-compatible, as it would break code that uses (tuple 'def 'x 1) in a macro definition, in addition to code that uses ['def 'x 1]. But I think it's more important that those two expressions mean the same thing. I don't think there's any way to change the default bracketedness without breaking lots of code.

pyrmont commented 1 month ago

Ah, right, sorry—my mistake! I misread what you wrote as being a way to be more backwards-compatible. So to write macros, you'd have to use either (quasi-)quoted p-tuples or the proposed tuple/form function?

bakpakin commented 1 month ago

I probably not going to merge this due for backwards compatibility sake. I suppose you can argue that one way is better than the other, but the distinction between bracketed and unbracketed tuples is just a little oddity of the language that I'm not going to remove that really only matters in macros or anything that generates code fed to 'compile'. Basically, once you have created you tuple it should not matter if the bracket flag is set or not in most cases. So breaking the behavior here seems pointless.

Also toggle doesn't seem particularly useful either for the above reason

pyrmont commented 1 month ago

Doesn't the fact that tuple types change equality and hashing results mean it does matter what the flag is?

bakpakin commented 1 month ago

Yes it matters only that we are consistent and strongly prefer one or the other to avoid equality issues, which is why we try to generate p-tuples all the time unless you explicitly ask for a bracketed tuple. Currently only two ways to get a bracketed tuple:

ianthehenry commented 1 month ago

I assumed the motivation for this change was to change the way that Janet prints (most) tuples:

# before
repl:1:> [1 2 3]
(1 2 3)

# after
repl:1:> [1 2 3]
[1 2 3]

Which is a minor UX improvement. Otherwise the default doesn't really matter (assuming you mostly define macros out of quasiquote, which is not the case in boot.janet at least).

pyrmont commented 1 month ago

Thanks everyone for the feedback. I disagree about the acceptableness of the status quo but understand the desire to minimise breaking changes.