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

[BUG] StringLiteral concat works only in narrow cases #9

Closed
akirchhoff-modular opened this issue May 3, 2023 · 8 comments
Closed
Labels
enhancement New feature or request mojo-repo Tag all issues with this label

Comments

@akirchhoff-modular
Copy link
Contributor

Reported by @dom96 on Discord.

  1. String literals do not persist across cells:

    Cell 1: var x = "hello".
    Cell 2: print(x)

    Expected result: "hello" printed
    Actual result: Blank line printed

  2. Concatenation with a string literal stored in a variable causes an error

    var x = "Hello"
    print(x)
    print("hi" + x)

    Expected result: "hihello" printed
    Actual result: Error: failed to legalize operation 'pop.string.concat' that was explicitly marked illegal

@Mogball Mogball added the bug Something isn't working label May 4, 2023
@Mogball Mogball changed the title Strings do not work as expected StringLiteral concat works only in narrow cases May 4, 2023
@Mogball
Copy link
Collaborator

Mogball commented May 4, 2023

We need to autopromote string literals to the String type, which properly supports runtime concatenation.

@Mogball Mogball added enhancement New feature or request and removed bug Something isn't working labels May 7, 2023
@Mogball Mogball changed the title StringLiteral concat works only in narrow cases [BUG] StringLiteral concat works only in narrow cases May 10, 2023
@Mogball
Copy link
Collaborator

Mogball commented May 11, 2023

Closing in favour of #100

@Mogball Mogball closed this as completed May 11, 2023
@akirchhoff-modular
Copy link
Contributor Author

@Mogball Is #100 sufficient to capture this? For example, it seems like let x: StringLiteral = "hello" and print(x) in separate cells should work, and would also have explicitly turned off auto-promotion so #100 would not be relevant. Am I missing something?

@Mogball
Copy link
Collaborator

Mogball commented May 22, 2023

Yes. StringLiteral.add would have to promote to a String

@akirchhoff-modular
Copy link
Contributor Author

I'm not talking about __add__, I'm talking just about having a StringLiteral variable in one cell and printing it in another. This is bullet point 1 in this issue -- I understand bullet point 2 is separate.

@Mogball
Copy link
Collaborator

Mogball commented May 22, 2023

Ah I see. That will probably not work in the notebook environment because string literals are pointers to static memory, which only lives as long as the jit'd object (I think). I'll file a new issue appropriately.

@akirchhoff-modular
Copy link
Contributor Author

Sounds good. If you could, please link the new issue if you file it publicly (or send me the issue privately otherwise, please).

@Mogball
Copy link
Collaborator

Mogball commented May 22, 2023

#232

@ematejska ematejska added the mojo-repo Tag all issues with this label label May 7, 2024
modularbot pushed a commit that referenced this issue May 29, 2024
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib+0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
modularbot pushed a commit that referenced this issue Jun 7, 2024
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib+0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

4 participants