modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo
Other
21.59k stars 2.52k forks source link

[BUG]: Why does setenv take StringRef and not String #1331

Closed skyhook88 closed 5 months ago

skyhook88 commented 5 months ago

Bug description

would like to set envioment variables for dynamic content not known at compile time but this does not seem to be the case.

Steps to reproduce

from os import setenv

def main(): var val_test:String="4" val_test+="2" setenv("MYENV_VALUE",val_test)

System information

NA
Sharktheone commented 5 months ago

I think, you can do this like so:

let success = setenv("MYENV_VALUE", StringRef(val_test._as_ptr()))

But I'm unsure, if there is any better way because this seems quite hacky.

skyhook88 commented 5 months ago

Thanks @Sharktheone

I agree is hacky but I know the team will have something they are working on to make this awsome.

I gave your suggestiona try but I get random garbage when testing the print output and I also tried by setting len just in case, also mentioned by the docs. I'm not sure if this is a bug / machine specific issue or I'm just not using correctly.

    var val_test:String="4"+"2"
    print("val_test:",val_test)

    var ref_test:StringRef = StringRef(ptr=pylib._as_ptr(),len=len(val_test))
    print("ref_test:",ref_test)

output : val_test: 42 ref_test: ú

output: val_test: 42 ref_test: �9

Sharktheone commented 5 months ago

Indeed, it does something weird. For me it looks like, it uses the next string it can get.

    let val_test:String="4"+"2"
    print("val_test:",val_test)

    let ref_test:StringRef = StringRef(val_test._as_ptr())
    print("ref_test:",ref_test)

This somehow has this output for me

val_test: 42
ref_test: ref_te

So the _as_ptr() method somehow gets the string from the print statement and not from the val_test variable. I guess, we found another bug. Maybe we should open another issue.

skyhook88 commented 5 months ago

regarding the raising an addional issue, I think we just don't know enough - hopefully the documentation will be fleshed out some more with examples. using ._steal_ptr() did the trick for a rough workaround.

Sharktheone commented 5 months ago

Okay, i wasn't sure, if it might be a problem to steal the ptr, but good, when it worked.

JoeLoser commented 5 months ago

I think it's reasonable to have setenv take a StringRef. You can still make the content you want to set for your environment variable dynamic (using String) and then construct a StringRef on-the-fly when needed when calling setenv. To the code example earlier, @Sharktheone, your issue was not keeping val_test alive along enough; this is the data backing the StringRef. Here's an example with working code:

fn main():
  let val_test:String="4"+"2"
  print("val_test:",val_test)

  let ref_test:StringRef = StringRef(val_test._as_ptr())
  print("ref_test:",ref_test)
  _ = val_test ^ # this is a way to lifetime-extend the `val_test` so it stays alive long enough since it's backing the `ref_test` `StringRef` variable

In this example above, the output is:

val_test: 42
ref_test: 42

As-is, there's nothing to fix from the library side regarding this as far as I can tell - so I'm going to close this issue. Happy to continue the discussion however, or feel free to re-open if you think there is something to do in the library here to make this better.