tomoyanonymous / mimium-rs

minimal musical medium- an infrastructural language for sound and music.
Mozilla Public License 2.0
7 stars 1 forks source link

Cannot parse nested `if` #63

Closed yutannihilation closed 1 month ago

yutannihilation commented 1 month ago

For some reason, this code cannot be parsed.

fn foo(x) {
  if (x == 0.0) {
    0.0
  } else {
    if (x > 0.0) {
      1.0
    } else {
      -1.0
    }
  }
}

To my eyes, the implementation looks correct. If the expr_group parser can parse if (...) { ..., it should also able to parse if (...) { if (...) { ... recursively. But...

https://github.com/tomoyanonymous/mimium-rs/blob/5c59926c6a4082970a6c0a9a2eae27aa8890c67b/mimium-lang/src/compiler/parser.rs#L339-L349

yutannihilation commented 1 month ago

This works. So, it seems the problem is something related to handling of {.

fn foo(x) {
  if (x == 0.0) 0.0 else if (x > 0.0) 1.0 else -1.0
}
tomoyanonymous commented 1 month ago

https://github.com/tomoyanonymous/mimium-rs/blob/5c59926c6a4082970a6c0a9a2eae27aa8890c67b/mimium-lang/src/compiler/parser.rs#L338

It looks like the content of the block is not 'expr_group' but expr which doesn't contain if expression. I'm still struggling how not to cause an infinite loop in chumsky. I'm now working on implementing array access parser, so I'll take a look at that as well.

yutannihilation commented 1 month ago

Oh, I see, thanks. I think this is a minor issue, so you can concentrate on implementing array. We can fix this later.

I'm still struggling how not to cause an infinite loop in chumsky.

I see...

yutannihilation commented 1 month ago

One more problem I'm investigating is that nested if seems to cause some stack inconsistency. I'm not sure if this is reproducible on other platform than my powerless Linux laptop. This is also not a very high priority. Some other change might eventually fix this. But, let me report here before I forget.

fn phasor(freq){
  (self + freq)
}
fn if_(x) {
  if (1.0) {
    -1.0
  } else {
    1.0
  }
}
fn foo(freq){
  let p = phasor(freq)
  if (1.0) {
    0.0
  } else {
    if_(p)
  }
}

fn dsp(){
  foo(440.0)
}
double free or corruption (out)
Aborted (core dumped)
tomoyanonymous commented 1 month ago

The nested if with block can be parsed just by changing this from

 let block = block_parser(expr.clone()); 

to this.(It's fixed in #64)

 let block = block_parser(expr_group.clone()); 
yutannihilation commented 1 month ago

Confirmed the first code now can be parsed with #64! (bytecodegen fails for some reason) Thanks.

tomoyanonymous commented 1 month ago

One more problem I'm investigating is that nested if seems to cause some stack inconsistency. I'm not sure if this is reproducible on other platform than my powerless Linux laptop. This is also not a very high priority. Some other change might eventually fix this. But, let me report here before I forget.

This happens on macOS too. Something related to the implementation of SetState in VM...

tomoyanonymous commented 1 month ago

(bytecodegen fails for some reason)

This fails on the second code too. Multiple use of if in the single function was not considered now.

Branched block should have jmp instruction to the return block in the MIR level but now they do not have...

  block 0
  reg(0)  := load x, number
  reg(1)  := float 0
  reg(2)  := eq reg(0) reg(1)
  reg(3)  := jmpif reg(2) 1 2

  block 1
  reg(4)  := float 0

  block 2
  reg(5)  := load x, number
  reg(6)  := float 0
  reg(7)  := gt reg(5) reg(6)
  reg(8)  := jmpif reg(7) 3 4

  block 3
  reg(9)  := float 1

  block 4
  reg(10) := float 1
  reg(11) := negf reg(10)

  block 5
  reg(12) := phi reg(9) reg(11)

  block 6
  reg(13) := phi reg(4) reg(12)
  reg(14) := ret reg(13) number
yutannihilation commented 1 month ago

Ah, makes sense. So, currently, both then and else branches are executed in case of the condition is true, yet it doesn't make problems in most of the cases except when it has some side effects?

tomoyanonymous commented 1 month ago

I guess the then/else block jumps to the last block of function currently?

yutannihilation commented 1 month ago

Sorry, I was a bit confused. I see. Jmp is currently inserted at bytecode, not at MIR.

https://github.com/tomoyanonymous/mimium-rs/blob/5c59926c6a4082970a6c0a9a2eae27aa8890c67b/mimium-lang/src/compiler/bytecodegen.rs#L578

yutannihilation commented 1 month ago

(Just for note) It seems the segfault problem I reported above is unrelated to the nested-if problem. This is a bit more minimal version. It seems the problem happens when

fn some_self_fun() {
  self + 1.0
}
fn foo() {
  0.0
}
fn bar(){
  let _ = some_self_fun()

  if (1.0) {
    0.0
  } else {
    foo()
  }
}

fn dsp(){
  bar()
}
tomoyanonymous commented 1 month ago

This was fixed in #67 (including in the segfault on the last comment), thanks!

yutannihilation commented 1 month ago

I think this issue can be kept closed because the original issue is fixed. But, let me note that I couldn't confirm this point

including in the segfault on the last comment

I still see the code fails on the current dev branch (although the error message is different). Both on Linux and on Windows.

❯ cargo run --release -- tmp.mmm

...snip...

     Running `target/release/mimium-cli tmp.mmm`
[*] input device: default buffer size:Fixed(2048)
[*] output device defaultbuffer size:Fixed(2048)
[*] in:true out:true
Press Enter to exit
Segmentation fault (core dumped)

I'll investigate and file a new issue, hopefully soon...

tomoyanonymous commented 1 month ago

Oh, I have only checked with the option --output-format csv --times 10. On my macOS, it randomly crushes if the times is set around 100~200... (It definitely happens around SetState operation as long as I could catch)