388 if should make local variable scope

[EruEri]

#388

• Create the If(IfCtx) in lib/skc_ast2hir/src/hir_maker_context.rs
• Push and pop context in lib/skc_ast2hir/src/convert_exprs.rs

[yhara]

thread 'main' panicked at 'must not happen', lib/skc_ast2hir/src/ctx_stack.rs:406:42

The panic is here:

            HirMakerContext::While(_) => panic!("must not happen"),

This assertion is not correct now because while may enclose if like this:

while x
  if y
    ...
  end
end

So I thought this should fix it but I got another error. I'll investigate it further.

diff --git a/lib/skc_ast2hir/src/ctx_stack.rs b/lib/skc_ast2hir/src/ctx_stack.rs
index 7dcaab2..da1ed2f 100644
--- a/lib/skc_ast2hir/src/ctx_stack.rs
+++ b/lib/skc_ast2hir/src/ctx_stack.rs
@@ -402,8 +402,10 @@ impl<'hir_maker> Iterator for LVarIter<'hir_maker> {
                 self.cur -= 1;
                 Some(scope)
             }
-            // ::new() never sets `While` to .cur
-            HirMakerContext::While(_) => panic!("must not happen"),
+            HirMakerContext::While(_) => {
+                self.cur -= 1;
+                self.next()
+            }
         }
     }
 }

[EruEri]

Maybe the WhileCtx should also contains a field

    pub lvars: HashMap<String, CtxLVar>,

I try in local with those change and the following example seems to work

let y = 10

while true
  if y == 10
    puts "10"
  end
end

[EruEri]

Also I don't really understand what is the use of the lvar ? Sorry if it's a dummy question

[yhara]

lvars holds information of Local VARiables defined in that scope. For example:

                    # ctx_stack:                                                  
                    # ToplevelCtx has                                                
let a = 1           #   a                                                         
class A             # ClassCtx                                               
  def foo           # MethodCtx has                                                  
    let b = 2       #   b                                                         
    while true      # WhileCtx has
      let c = 3     #   c
      if ...        # IfCtx has                                                      
        let d = 4   #   d       
        ...                  

Maybe the WhileCtx should also contains a field

Oh that's right - I made a ticket for that.

I got an error for this example. Maybe we need to push a IfCtx for else-clause too?

let x = 0
if x == 1
  # ...
else
  let y = 1
  p x + y
end

[EruEri]

The error that you get seems to append in the codegen stage

And it seems that it's because in gen_exprs.rs with the example above, Only the toplevel lvars are in ctx, so in the exemple above (x).

And it looks like the lvars in ctx are set once in skc_codegen/src/lib.rs

But I am not sure sure whether or not what I said is true. It's just my impression

[yhara]

The error that you get seems to append in the codegen stage

Yes. And local variables in a method are allocated here.
So when dropping the IfCtx we need to extract ctx.lvars and do either of these:

• a) store them in SkMethod.lvars.
• b) add .lvars to HirIfExpression.

Let me consider which would be better. (sorry this issue is not as easy as I thought!)

[EruEri]

No worries no worries, and at least it makes me explore the code deeper than just the ast2hir module

I think I will also try to implement one of your solution

[yhara]

At first a) looked easier to implement:

• CtxStack::declare_lvar is called for each let. So you can add the lvar both to the IfCtx and MethodCtx at once
• But, the problem is, the added lvar may be modified by set_lvar_captured

So I'd recommend b).

• Add .lvars to HirIfExpression
• Then you can collect lvars to alloca by traversing the body here.

[EruEri]

Thanks for your indication, I will try implement that

[EruEri]

I think the If should work, but I'm not sure if the way I wrote it is good, and in the hir_maker.rs#_set_default method I put an empty lvars instead of maybe find a way to compute the lvars

[yhara]

Good job! I've confirmed all tests in cargo test passes.

As for CI, cargo run -- run examples/ray.sk raises "variable r_len not found".
Maybe you should push and pop if_ctx only once per then clause, instead of each then_expr.

[yhara]

in the hir_maker.rs#_set_default method I put an empty lvars instead of maybe find a way to compute the lvars

_set_default is used when a method argument has a default value. For example,

class A
  def foo(a: Int = 1)
    ...

In this case _set_default returns some thing like

if <a is omitted>
  a = 1
else
  a = <get the given value for a>
end

No new lvars will be created in this if so passing empty vec is just fine.

[yhara]

[FYI] you can use .. when you don't need the value.

[EruEri]

I thought I would have needed to use the lvars in this match but I forget to use ..
I will fix that, Thank you

[EruEri]

Indeed, I shouldn't have pushed and poped IfCtx for each expression in then

I will fix that too, Thank you

[yhara]

-rw-r--r--@ 1 yhara  staff  2613657 12  9 21:16 ray.expected_out.ppm
-rw-r--r--@ 1 yhara  staff  1361160  4 30 23:33 ray.actual.ppm

examples/ray.ll generates much smaller output than expected. I'll investigate why.

[yhara]

examples/ray.ll generates much smaller output than expected. I'll investigate why.

It seems that examples/ray.out crashes on allocating memory at certain point (that's why the output is small.)
I'll check the difference of generated .lls.

[EruEri]

I reviewed myself, the pending review, do you think it maybe what caused the issues ?
Since the block of code that I highlight allocated llvm_obj

[yhara]

Ok I've found what is changed. Before this PR, nine variables are alloca'ed here:

define %Void* @Plane_intersect(%Plane* %self, %Ray* %ray, %Isect* %isect) {
  br label %alloca

alloca:                                           ; preds = %0
  %d = alloca %Float*, align 8
  %m = alloca %Float*, align 8
  %abs = alloca %Float*, align 8
  %v = alloca %Float*, align 8
  %t = alloca %Float*, align 8
  %d2 = alloca %Float*, align 8
  %f = alloca %Float*, align 8
  %n = alloca %Float*, align 8
  %d3 = alloca %Float*, align 8
  br label %alloca_End

With this PR, only three of them are alloca'ed

define %Void* @Plane_intersect(%Plane* %self, %Ray* %ray, %Isect* %isect) {
  br label %alloca

alloca:                                           ; preds = %0
  %v = alloca %Float*, align 8
  %t = alloca %Float*, align 8
  %d = alloca %Float*, align 8
  br label %alloca_End

and the rest are alloca'ed just before %IfBegin.

...
"Invoke_Float#/_end":                             ; preds = %"Invoke_Float#/"
  store %Float* %result69, %Float** %t, align 8
  %m = alloca %Float*, align 8                    
  %n = alloca %Float*, align 8
  %abs = alloca %Float*, align 8
  %d2 = alloca %Float*, align 8
  %f = alloca %Float*, align 8
  %d3 = alloca %Float*, align 8
  br label %IfBegin

Since alloca allocates new memory on stack, calling it in a loop causes stack overflow. In other words, alloca should be called only once for each lvar, at the beginning of the llvm function for example.

[EruEri]

Sorry for the late reply, I'm pretty busy right now but as soon as I have the time I will try to fix the allocation
Thank you for the hints

[EruEri]

I think that the allocation should work but I not quite sure about my implementation choice Since now .
ToplevelCtx, ClassCtx, MethodCtx and LambdaCtx have the new field which also collects the lvars and it seems weird since the field lvars also collects the local variables but I need a seperated map since for exemple

if x == 1
  puts "10"
else
  let y = 1
  p x + y
end

if we add y to the lvars of the closest scope that collect lvars (in this case ToplevelCtx)
y will pass the typecheck even if it's used after the if
This is why I need a another map but It's seen a little duplicated and also it's not very convenient since just before the allocation we need to do something like that

let mut lvars = extract_lvars(&mut toplevel_ctx.lvars);
let mut lvars_collected = extract_lvars(&mut toplevel_ctx.lvars_collected);
lvars.append(&mut lvars_collected);

// Use lvars

[yhara]

but It's seen a little duplicated and also it's not very convenient since just before the allocation we need to do something like that

Hmm, another possible approach is to collect lvars in skc_codegen instead of skc_ast2hir.

• What we want is "a list of lvars used in this LLVM function"
• We need this list on creating a LLVM function i.e. gen_llvm_func_body
• Before this PR, lvars here does not include lvars local to if or while clauses
• By traversing the body, you can find all the HirIfExpressions there. So if HirIfExpression contains the list of lvars in it, we can collect additional lvars to alloca.

body looks a little bit complicated but most of the cases are not relevant this time.

• SkMethodBody::Normal - A method written in Shiika. We should traverse exprs in this case.
• SkMethodBody::RustLib - A method written in Rust, therefore does not have Shiika lvars.
• SkMethodBody::New - A new method. It just calls initialize method, so does not have Shiika lvars.
• SkMethodBody::Getter, SkMethodBody::Setter - Just gets or sets the value of an instance variable. Does not have Shiika lvars.
• Right(exprs) - A lambda (do...end). Lambdas are also converted into LLVM functions. We should traverse exprs in this case.

[EruEri]

I traversed the body and it seems to work but I have 1 question
Do we need to collect lvars in HirExpressionBase::HirLambdaExpr in the exprs fieds ? I don't think so since I believe lambda expression are latter turn into llvm_function so the collection of lvars will be apply here. But I'm not sure

[EruEri]

Also if what I have done for the if is correct can I also collect lvars for the while ?

[yhara]

Congrats for CI pass! 🎉

Do we need to collect lvars in HirExpressionBase::HirLambdaExpr in the exprs fieds ? I don't think so since I believe lambda expression are latter turn into llvm_function so the collection of lvars will be apply here.

That's right, you can skip traversing HirLambdaExpr.

Also if what I have done for the if is correct can I also collect lvars for the while ?

Yes, please!

[EruEri]

I think there is a new issue now.

In this example

# e.sk

while true
  let y = 10
  p y
end

let y = "Hello world"
puts y

the compilation will fail since the llvm will see 2 variables with the same name but 2 differents types

[EruEri]

the command shiika run e.sk fail with this error message

error: Explicit load/store type does not match pointee type of pointer operand (Producer: 'LLVM12.0.1' Reader: 'LLVM 12.0.1')
1 error generated.

[yhara]

I think there is a new issue now.

Oh, there are two options to fix this:

• a) disallow such variable declaration (ala C#)
• b) support this by implicitly renaming lvars (eg. y@1 and y@2)

In either case, let's create a new github issue for this.

[EruEri]

Oh, there are two options to fix this:

* a) disallow such variable declaration (ala C#)

* b) support this by implicitly renaming lvars (eg. `y@1` and `y@2`)

In either case, let's create a new github issue for this.

Ok, sound good to be

So excepted that, I think the lvars in If and whlie should work properly.

But I wonder in rust, if a function return unit, it's better to discard the result or create a "() variable"

 function();
// or 

let () = function();

[yhara]

So excepted that, I think the lvars in If and whlie should work properly.

I've merged this PR. Thank you for your contribution!

But I wonder in rust, if a function return unit, it's better to discard the result or create a "() variable"

Usually you don't need to create a variable for ()s. In contrast, rustc shows warning if the function returns a Result.

function(); // warning

In this case you could define a variable to intentionally discard the error:

let _ = function();

but in most cases you'll just use ? operator to forward the error to the caller.

function()?;

[EruEri]

Thanks to you too for answering my questions

It was nice to contribute to shiika and if I'm able to, I will try contribute to other issues on shiika