nim-works / phy

compiler and vm experiments
MIT License
3 stars 2 forks source link

lang: add expression lists #50

Closed saem closed 1 month ago

saem commented 1 month ago

Summary

Add Exprs construct, which allows for expression lists, where a series of expressions can be treated as a single expression. This allows writing zero or more unit or void expressions which must be followed by a final expression of any type.

Details

Added the spec Exprs <exprs>+ construct support in the specification, as well as the implementation and associated tests. With the possibility of void expressions in non-trailing positions, sourceToIL translation will drop all trailing expressions after the void expression. This avoids L10 errors related to unreachable code following any void expression (e.g.: break, continue, return, etc).

As part of this change the following convenience routines were added to assist in Exprs and future changes:

N.B.: Since we neither have runtime effects (i.e.: echo) nor mutable local variables, some behaviour like multiple observable expressions cannot be tested as part of this change. Those tests will be added in future changes once we can sense such changes.


Note for Reviewers

zerbina commented 1 month ago

I've gathered a list of things that need testing, but where the tests don't require effect-having expressions:

saem commented 1 month ago

Right now the following:

(Exprs (Unreachable) (TupleCons) (FloatVal 1.0))

Fails because it results in the following L10, which has unreachable code errors:

(Unreachable)
(Drop (IntVal 0))
(FloatVal 1.0)

I'm conflicted as to what to do, some options I can think of:

  1. remove the Unreachable restriction
  2. remove the flag Stmt restriction
  3. have Exprs processing culling everything after Unreachable and consider that desirable behaviour
  4. accept this as a failing test and update the spec, if needed

My personal leanings are towards 4 at this point.

zerbina commented 1 month ago

The assertion message is maybe a bit unclear in this regard, but it triggers for all dead code situations, not just Unreachable. For example, the following code also trigger the assertion:

(ProcDecl (Ident "a") (UnitTy) (Params)
  (Stmts
    (Return)
    (TupleCons)))

Now, the assertion could be removed without causing any issues, as pass10 would just discard the dead code following the Return.

In general, I prefer aborting/crashing over silent error correction in case of easily detectable issues with the input, though I'm not sure whether dead code should really be considered an error in the L10 language (it's currently not described as such).

I'm conflicted as to what to do, some options I can think of:

  1. remove the Unreachable restriction
  2. remove the flag Stmt restriction
  3. have Exprs processing culling everything after Unreachable and consider that desirable behaviour
    1. accept this as a failing test and update the spec, if needed

My personal preference is towards number 3, though I think 4 would be fine too. If unreachable code being a hard error turns out to be too much of an ergonomic problem, then we can change it later on and allow void expression in non-trailing positions again.

saem commented 1 month ago

The assertion message is maybe a bit unclear in this regard, but it triggers for all dead code situations, not just Unreachable. For example, the following code also trigger the assertion:

(ProcDecl (Ident "a") (UnitTy) (Params)
  (Stmts
    (Return)
    (TupleCons)))

Ah, good point, I took the message too literally, a break, continue, raise, non-terminal call, etc would likely all result in the same.

Now, the assertion could be removed without causing any issues, as pass10 would just discard the dead code following the Return.

In general, I prefer aborting/crashing over silent error correction in case of easily detectable issues with the input, though I'm not sure whether dead code should really be considered an error in the L10 language (it's currently not described as such).

That's why I was conflicted, I thought the error/failure had benefit.

I'm conflicted as to what to do, some options I can think of:

  1. remove the Unreachable restriction
  2. remove the flag Stmt restriction
  3. have Exprs processing culling everything after Unreachable and consider that desirable behaviour
  4. accept this as a failing test and update the spec, if needed

My personal preference is towards number 3, though I think 4 would be fine too. If unreachable code being a hard error turns out to be too much of an ergonomic problem, then we can change it later on and allow void expression in non-trailing positions again.

Looking at it today with fresher eyes, I think 3 makes more sense, too. Since the higher level layer knows why it's eliminating the code (read: doing it with good reason) I think it's safe and L10 can catch all the issues where a mistake have been made.

saem commented 1 month ago

I didn't update the spec with non-trailing void expression post-expression elimination because it didn't seem like a particularly observable effect, but I could go either way.

zerbina commented 1 month ago

I didn't update the spec with non-trailing void expression post-expression elimination because it didn't seem like a particularly observable effect, but I could go either way.

I don't think it's necessary now. Maybe it'll be in the future, for things like exception tracking (if that feature makes the cut), but that's still a long time away.

saem commented 1 month ago

No idea, how I convinced myself that reporting an error for a trailing void made sense, good catch.

For the skipped sem check of post-void expressions, I was about to use a loop, but the template approach seems to read pretty cleanly. I also renamed the tests to drop _error from the file name because that's reserved for testing expected errors, instead of expected output that happens to be a runtime error.