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

Add ECR::Lexer::SyntaxException with location info #15222

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.

src/ecr/lexer.cr Outdated Show resolved Hide resolved
@Sija
Copy link
Contributor

Sija commented Dec 13, 2024

Any chances to include this into v1.15.0?

@straight-shoota straight-shoota added this to the 1.15.0 milestone Dec 16, 2024
@straight-shoota straight-shoota changed the title ECR syntax error raises exception that includes line/column information Add ECR::Lexer::SyntaxException with location info Dec 17, 2024
@straight-shoota straight-shoota merged commit 25086b9 into crystal-lang:master Dec 17, 2024
69 checks passed
@nobodywasishere nobodywasishere deleted the nobody/ecr-syntax-exception branch December 17, 2024 18:59
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
5 participants