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

Tuple Literal Expansion #1320

Merged
merged 14 commits into from
Mar 20, 2020
Merged

Conversation

njordhov
Copy link
Contributor

@njordhov njordhov commented Mar 12, 2020

Implements tuple literals with curly braces as implicit syntax for tuples in Clarity (issue #1234).

Tuple literals are expanded into explicit tuple representation as if parsing (tuple (key1 value1) (key2 value2) ...), based on commit df96a70 from PR #1314, which implements a similar approach. This is a less ambitious and comprehensible implementation than PR #1314 but a suitable first step, allowing tuple literals with curly braces to immediately replace implicit tuples that use the legacy list-of-pairs syntax, as well as providing a compact alternative to explicit tuple expressions.

@njordhov
Copy link
Contributor Author

njordhov commented Mar 12, 2020

@kantai The tuple literal expansion currently happens in the token parser to keep changes to a minimum, but perhaps it should be lifted to the sugar expander?

@njordhov
Copy link
Contributor Author

njordhov commented Mar 12, 2020

Note: The PR obsoletes the get_definition_type_of_tuple_argument function and calls as tuples always have an explicit symbolic representation.

@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #1320 into master will increase coverage by 0.41%.
The diff coverage is 87.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
+ Coverage   84.61%   85.03%   +0.41%     
==========================================
  Files         155      164       +9     
  Lines       45545    49233    +3688     
==========================================
+ Hits        38539    41865    +3326     
- Misses       7006     7368     +362     
Impacted Files Coverage Δ
src/chainstate/stacks/db/blocks.rs 89.25% <ø> (+0.18%) ⬆️
src/chainstate/stacks/db/mod.rs 83.55% <0.00%> (-1.56%) ⬇️
src/chainstate/stacks/db/transactions.rs 96.90% <0.00%> (-0.29%) ⬇️
src/chainstate/stacks/mod.rs 65.79% <0.00%> (-1.05%) ⬇️
src/main.rs 0.00% <0.00%> (ø)
src/net/http.rs 89.47% <ø> (+0.53%) ⬆️
src/vm/analysis/contract_interface_builder/mod.rs 74.56% <0.00%> (-0.44%) ⬇️
src/vm/tests/mod.rs 100.00% <ø> (ø)
src/vm/analysis/errors.rs 66.01% <38.88%> (-4.89%) ⬇️
src/clarity.rs 58.40% <40.00%> (-0.37%) ⬇️
... and 140 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f07ce5...5b92360. Read the comment docs.

@njordhov njordhov requested a review from lgalabru March 16, 2020 16:14
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @njordhov, this looks great to me!

My only question is regarding separator syntax -- right now, the curly bracket syntax for a tuple looks like:

 { a (+ 1 b) c (* 3 2 b) foo bar }

While this is unambiguous, most other language would use some delimiters to make it more obvious when an item is key or a value, and which item it correspond to, e.g.:

 { a: (+ 1 b), c: (* 3 2 b), foo: bar }

@njordhov, @lgalabru: What do you think? Should we require something like , and : in the syntax?

@diwakergupta
Copy link
Member

I'm in favor of explicitly separating keys from values (:) and element separators (either ; or ,)

@lgalabru
Copy link
Contributor

+1 for introducing : and ,

@njordhov
Copy link
Contributor Author

njordhov commented Mar 17, 2020

My only question is regarding separator syntax
Should we require something like , and : in the syntax?

Clarity benefits from using a minimalist syntax. Syntax can be hard to remove or change later, making prudence a wise strategy, keeping our options open.

Colons after field names adds unnecessary complexity with little to gain. Allowing commas as field separators could be useful though.

right now, the curly bracket syntax for a tuple looks like:

 { a (+ 1 b) c (* 3 2 b) foo bar }

While this is unambiguous, most other language would use some delimiters to make it more obvious when an item is key or a value, and which item it correspond to

In other languages, the delimiters are typically necessary to avoid ambiguity.

The clean tuple literal syntax without separators is shared with Clojure. Although, for pragmatic reasons in accommodating developers from other languages, Clojure considers commas as whitespace so they can optionally be used to visually separate key/value pairs. It's a sensible decision that doesn't complicate tokenizing and parsing.

Just like in let bindings, I usually prefer separating key/value pairs with newline rather than commas, particularly when the values are list forms:

{ a (+ 1 b) 
  c (* 3 2 b) }

Avoiding dangling commas is practical for editing. If we allow commas as separators, they should be optional.

However, commas as separators can make sense for readability when the values are atoms on the same line:

{ id id, foo foo }

Although extra whitespace may serve the same function of visually grouping key/value pairs:

{ id id  foo foo }

If we allow commas as separators, I propose we put them to use to support a shorthand for the property when the name and value are the same symbol:

{ id, foo }

This is like Rust field init shorthand and ES6 Shorthand Property Names

Makes sense?

@jcnelson
Copy link
Member

Clarity benefits from using a minimalist syntax. Syntax can be hard to remove or change later, making prudence a wise strategy, keeping our options open.

Once launched, the Clarity syntax will be impossible to change without a hard fork. Plan accordingly.

@njordhov njordhov requested a review from kantai March 17, 2020 23:45
@kantai
Copy link
Member

kantai commented Mar 18, 2020

Colons after field names adds unnecessary complexity with little to gain. Allowing commas as field separators could be useful though.

right now, the curly bracket syntax for a tuple looks like:

 { a (+ 1 b) c (* 3 2 b) foo bar }

While this is unambiguous, most other language would use some delimiters to make it more obvious when an item is key or a value, and which item it correspond to

In other languages, the delimiters are typically necessary to avoid ambiguity.

Thank you for the thoughtful explanation @njordhov! This makes sense, however, since many (perhaps even most) Clarity developers will have less experience in LISP-variants, we think the more common colon and comma delimiter syntax will be a lot more familiar to developers coming from other languages.

So in that case, we want the curly bracket syntax to require colons and commas, and allow dangling commas. If you'd like to implement that in this PR, we'd appreciate it. Or if you'd prefer, I can merge this PR to a feature branch, and apply those updates before merging to master.

@njordhov
Copy link
Contributor Author

njordhov commented Mar 18, 2020

we think the more common colon and comma delimiter syntax will be a lot more familiar to developers coming from other languages.

Comma delimiters after tuple literal values are implemented in f0a4a64, including dangling commas, while d042077 allows decorative colons after the keys of tuple literals.

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks @njordhov! I think this just needs a few more test cases for some malformed tuple syntax, e.g.:

{ a: b, b: ,}

@njordhov njordhov requested a review from kantai March 18, 2020 23:21
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the tests -- this looks good to me @njordhov!

For now, letting the tuple delimiters be optional syntax is fine, but we may revisit this later on to make them required syntax.

Since we require two approvals for PRs on stacks-blockchain, we'll need one of @jcnelson or @lgalabru to review this, and then we can merge.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your help @njordhov! This PR is a nice upgrade, I left some comments / questions.

@njordhov njordhov requested a review from lgalabru March 19, 2020 21:25
Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you @njordhov!

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.

None yet

5 participants