DanielXMoore / Civet

A TypeScript superset that favors more types and less typing
https://civet.dev
MIT License
1.33k stars 28 forks source link

Ampersand fixes #1233

Closed edemaine closed 1 month ago

edemaine commented 1 month ago

Fixes #1231

I'm a little worried about if/else. I'm sure how to feel about this example:

=>
  if cond
    &+1
  else
    &+2
// ↓↓↓ (currently)
() => {
  if (cond) {
    return $ => $ + 1;
  } else {
    return $1 => $1 + 2;
  }
};
// OR (this PR)
() => {
  return $ => { if (cond) {
    return $+1
  }
  else {
    return $+2
  }}
}

If you can think of a better rule for if/else, please let me know. (We could require there to be no indentation, i.e. bare, but that feels a bit limiting; see https://github.com/DanielXMoore/Civet/issues/1231#issuecomment-2098783437 )

STRd6 commented 1 month ago

I think most of this is pretty good but I'm not sure that & should lift above statements even when inside the blocks of an if/else.

When the if/else block is inside an expression like the example from #1231:

e.children = e.children.map
  if & is e.block then block else &

Then it may make sense to me to lift them but still seems to be a bit ambiguous.

One example illustrating some of the strangeness:

if f &
  &

The & in the block would be lifted above the f(&)

edemaine commented 1 month ago

The Civet source code already uses this helpful pattern a few times (from #1228):

https://github.com/DanielXMoore/Civet/blob/7d3554cc5b27f65e72bafffd5b31ddd7f7becdc7/source/parser/declaration.civet#L348

https://github.com/DanielXMoore/Civet/blob/7d3554cc5b27f65e72bafffd5b31ddd7f7becdc7/source/parser/declaration.civet#L355

https://github.com/DanielXMoore/Civet/blob/7d3554cc5b27f65e72bafffd5b31ddd7f7becdc7/source/parser/declaration.civet#L363

Do you think that & should be lifted out of a ternary ?:, but not out of if/then/else? This feels asymmetric, which is why I originally added lifting out of if/then/else. But if/then/else is way more powerful than ?: (in particular, multiple indented lines isn't really what we imagined lifting & out of), so I'm not sure. Some options for restrictions:

  1. Lift out of then/else clause that's one expression (what's currently implemented by this PR)
  2. Lift out of then/else clause that's a bare one-liner (if x then y else z without newlines or indentation)
  3. Lift out of then/else clause when the if is expressionized (I believe it would have a StatementExpression parent at this stage, so we could detect it). This would include the example with map, but I believe it would not include the example in my first message (that's an implicit return, not expressionization).
  4. Never lift out of then/else clause (current behavior). In this case, we should consider not lifting out of ?: either...

We can combine these. E.g., 1+2+3 would be the most restrictive, other than the full restriction of 4. I think ?: corresponds most closely to either 1+2 or 1+2+3.

STRd6 commented 1 month ago

I think lifting out of ternary ? : is fine because it is an expression. Even lifting out of the if & condition makes sense. But lifting out of a full statement within a containing block seems like too much.

Lifting out of if/else when it is in a StatementExpression might be ok but I think for now I'd lean towards Option 4. Keeping ternary lifting seems fine because it is an expression and not a statement.

Also interested in exploring further if we can find a consistent rule but for now "never lifts above a statement" seems relatively clear to understand.

edemaine commented 1 month ago

Agreed. However, your point about if & makes me think of another option:

  1. Lift out of then/else clause only if the if condition also has a lifted &.

What do you think?

I think this can be implemented because we process & left to right, so we can check whether this if has already been "activated" by a lifted condition.

edemaine commented 1 month ago

Yet another idea: if we wanted to enable something like & + if cond then &/2 else &/3, we could generalize to:

  1. Lift out of then/else clause if this would end up lifting to an ancestor to which another & has already been lifted.

The idea for both Options 5 and 6 is, if you have other &s already lifting to an upper level, then it's less surprising for other &s to refer to the same &.

edemaine commented 1 month ago

I went ahead and implemented the more conservative Option 5. (Not sure why there's an unnecessary IIFE wrapper there though...) Also happy to revert both if/else changes if you'd rather play it safe.