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

iftype compile error at runtime #4329

Open
Liasain opened this issue Mar 17, 2023 · 7 comments
Open

iftype compile error at runtime #4329

Liasain opened this issue Mar 17, 2023 · 7 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Liasain
Copy link

Liasain commented Mar 17, 2023

use "debug"

actor Main
  new create(env: Env) =>
    Debug(foo[U8]())
    Debug(foo[U16]())
    
  fun foo[A: Any val](): String =>
    iftype A <: U16 then
      Debug("U16 is a match")
      return "u16"
    else
      Debug("no match")
    end
    "default"

Prints the follow on the Playground:

no match
default
U16 is a match

Compilation failed.

On my machine it prints the first part, then segfaults.
It only happens when you return from the iftype, and if something is returned later. Moving default to the else branch removes the error.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 17, 2023
@SeanTAllen SeanTAllen added help wanted Extra attention is needed bug Something isn't working needs investigation This needs to be looked into before its "ready for work" labels Mar 17, 2023
@jemc
Copy link
Member

jemc commented Mar 17, 2023

I think this is what needs to happen:

diff --git a/src/libponyc/codegen/gencontrol.c b/src/libponyc/codegen/gencontrol.c
index d36f7813..74531c8c 100644
--- a/src/libponyc/codegen/gencontrol.c
+++ b/src/libponyc/codegen/gencontrol.c
@@ -605,6 +605,7 @@ LLVMValueRef gen_return(compile_t* c, ast_t* ast)
     codegen_scope_lifetime_end(c);
     LLVMBuildRetVoid(c->builder);
   }
+  // Make a new basic block and set it to current
 
   codegen_debugloc(c, NULL);
   return GEN_NOVALUE;

@SeanTAllen
Copy link
Member

SeanTAllen commented Mar 19, 2023

I don't think the problem is in gen_return. "fixing" the issue there results in other issues.

Looking at iftype, it doesn't do the branches as branches, based on "is subtype" check, it drops an expression directly in which is just not correct. It will work if you have:

  fun foo[A: Any val](): String =>
    iftype A <: U16 then
      Debug("U16 is a match")
      return "u16"
    else
      Debug("no match")
      "default"
    end

but it won't for the example we have.

@Liasain
Copy link
Author

Liasain commented Mar 20, 2023

It seems like it's transformed to the equivalent of

  fun foo(): String =>
    Debug("U16 is a match")
    return "u16"
    "default"

which does not compile. I'm not sure how it should be working, or even if, but in that case it should be rejected. It would be nice if you could return from iftype though...

@SeanTAllen
Copy link
Member

So @Liasain, that would be the equivalent to what it transforms to if it transformed to source, but it doesn't so it ends up with bad LLVM ir. You can return from iftype, but you can't if there is code after the iftype. That's what we need to fix.

@SeanTAllen
Copy link
Member

We did an excellent discussion of this during sync and came up with some ideas, I will discuss them with Joe and we can discuss again at another sync to make a final determination on how we want to fix.

@SeanTAllen
Copy link
Member

@jemc and I discussed this today and we have a solution that I will probably implement.

Basically, gen_iftype doesn't create a scope for iftype'd code (but earlier steps act as though it does have a scope). We intend to create a scope and make iftype operate like if except that it will have a single branch that ends up in the compiled code with an "always true" condition + the scope that one intuitively expects from iftype BLAH then ... end

@SeanTAllen SeanTAllen added good first issue Good for newcomers and removed needs investigation This needs to be looked into before its "ready for work" discuss during sync Should be discussed during an upcoming sync labels Apr 1, 2023
@SeanTAllen SeanTAllen self-assigned this Apr 1, 2023
@SeanTAllen
Copy link
Member

I've assigned myself to this, but anyone who wants this is welcome to take it.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 1, 2023
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Apr 1, 2023
@SeanTAllen SeanTAllen removed their assignment Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants