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

ECR syntax error raises exception that includes line/column information #15222

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nobodywasishere
Copy link
Contributor

Closes #15220. Don't know if I should add a rescue to ECR.process_string that re-raises as a ECR::SyntaxException, for now though I think this works. Didn't end up going with Crystal::SyntaxException as that required pulling in the entire ast / parser / compiler files which seemed silly for a simple exception.

@Sija
Copy link
Contributor

Sija commented Nov 30, 2024

Would be great to have this reviewed and released as a part of Crystal 1.15.0

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: To simplify the implementation, we could introduce a custom raise method which raises the appropriate SyntaxException at the current location:

privat def raise(message)
  ::raise SyntaxException.new(message, @line_number, @column_number)
end

The existing calls like raise "Unexpected end of file inside <%= ..." would stay as they are, but now they'd target the instance method instead of the global ::raise.

@nobodywasishere
Copy link
Contributor Author

@straight-shoota I would prefer naming it something like syntax_error instead of overwriting a common top-level method personally, as it could lead to confusion. I can switch to a common method for raising syntax exceptions though.

@straight-shoota
Copy link
Member

We use instance methods to shadow ::raise in other places in the compiler as well, so it wouldn't be unprecedented.

But I'm fine either way.

Comment on lines -126 to +139
raise "Expecting '>' after '-%'" if current_char != '>'

if current_char != '>'
raise "Expecting '>' after '-%'"
end

Copy link
Contributor

@Sija Sija Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be reverted for a cleaner diff.

@Sija
Copy link
Contributor

Sija commented Dec 13, 2024

Any chances to include this into v1.15.0?

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.

ECR::Lexer#consume_control should raise Crystal::SyntaxException instead of Exception
3 participants