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

Fix error when raising a macro exception with empty message #8654

Merged
merged 2 commits into from
Feb 3, 2020

Conversation

jan-zajic
Copy link
Contributor

@jan-zajic jan-zajic commented Jan 6, 2020

Unmasked stacktrace:

Unhandled exception: Index out of bounds (IndexError)
from src/array.cr:0:13 in 'shift'
from src/compiler/crystal/semantic/exception.cr:127:28 in 'append_to_s'
from src/compiler/crystal/semantic/exception.cr:95:7 in 'to_s_with_source'
from src/compiler/crystal/exception.cr:12:7 in 'to_s'
from src/io.cr:184:5 in '<<'
from src/io.cr:241:5 in 'puts'
from src/compiler/crystal/command.cr:115:7 in 'run'
from src/compiler/crystal/command.cr:46:5 in 'run'
from src/compiler/crystal/command.cr:45:3 in 'run'
from src/compiler/crystal.cr:8:1 in '__crystal_main'
from src/crystal/main.cr:97:5 in 'main_user_code'
from src/crystal/main.cr:86:7 in 'main'
from src/crystal/main.cr:106:3 in 'main'
from __libc_start_main
from _start
from ???

Fixes #8631

@bcardiff
Copy link
Member

bcardiff commented Jan 6, 2020

There are a couple of specs that stress there is some information in the backtrace (search for backtrace_sample).

I'm not familiar with the current error reporting code but I would like that some spec is added for this fix.

@straight-shoota straight-shoota changed the title Fix #8631 Fix error when raising a macro exception with empty message Jan 6, 2020
@jan-zajic
Copy link
Contributor Author

But @bcardiff this fail only at compile time if you raise inside macro. If you raise "" outside of macro, it works just fine. Fix is in compiler/crystal/semantic/exception.cr. So i rather add spec to spec/compiler/semantic/macro_spec.cr, is it ok?

@bcardiff
Copy link
Member

bcardiff commented Jan 7, 2020

Sure @jan-zajic, that should work:

it "executes raise inside macro" do
ex = assert_error %(
macro foo
{{ raise "OH NO" }}
end
foo
), "OH NO"
ex.to_s.should_not contain("expanding macro")
end
it "executes raise inside macro, with node (#5669)" do
ex = assert_error %(
macro foo(x)
{{ x.raise "OH\nNO" }}
end
foo(1)
), "OH"
ex.to_s.should contain "NO"
ex.to_s.should_not contain("expanding macro")
end

But there needs to be more information about the backtrace. Even if the test becomes a bit fragile to maintain IMO

@straight-shoota
Copy link
Member

But there needs to be more information about the backtrace.

That would be nice, but is not a requirement for merging this bugfix.

@RX14 RX14 merged commit ec26d82 into crystal-lang:master Feb 3, 2020
@RX14 RX14 added this to the 0.33.0 milestone Feb 3, 2020
@straight-shoota straight-shoota deleted the feature/8631 branch February 3, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{% raise "" %} crashes
5 participants