Dont escape newline if in comment #75
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adjusts the lexer so that escaped newlines are only skipped outside of line comment regions, preventing unintended behavior when parsing comments.
- Add a check to skip escape handling if the current region begins with
// - Update the conditional to include the region handler check
src/compiling/lexing/lexer.rs
Outdated
| // Reaction stores the reaction of the region handler | ||
| // Have we just opened or closed some region? | ||
| let reaction = if lex_state.is_escaped { | ||
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().unwrap().begin != "//" { |
There was a problem hiding this comment.
Using unwrap() here can lead to a panic if get_region() returns None. Consider handling the Option explicitly or using expect() with a clear error message.
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().unwrap().begin != "//" { | |
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().expect("Failed to retrieve region: get_region() returned None").begin != "//" { |
There was a problem hiding this comment.
while a valid concern, it has been accessed the same way a few lines before for 10 months now. besides, i don't think that it could possibly be None in this position
src/compiling/lexing/lexer.rs
Outdated
| // Reaction stores the reaction of the region handler | ||
| // Have we just opened or closed some region? | ||
| let reaction = if lex_state.is_escaped { | ||
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().unwrap().begin != "//" { |
There was a problem hiding this comment.
[nitpick] The string literal "//" is a magic value. Consider defining a constant for the line comment delimiter to improve readability and reduce duplication.
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().unwrap().begin != "//" { | |
| let reaction = if lex_state.is_escaped && lex_state.region_handler.get_region().unwrap().begin != LINE_COMMENT_DELIMITER { |
There was a problem hiding this comment.
This seems to be a valid review because Heraclitus is not only for Amber and // is Amber-specific.
Ph0enixKM
left a comment
Ph0enixKM
left a comment
There was a problem hiding this comment.
I think that Copilot is right here. Heraclitus is a library not only for Amber.
The proper solution I believe would be to add a field to this struct called ignore_escape. If we set that flag to true for comments then the escape key should not be affecting the parser to omit the end of comment
but it literally is though? the only language that is built on it is Amber, and so far most Heraclitus issues have been related to Amber
i agree, that's a more proper way to approach this, regardless of the previous statement |
|
@b1ek, even if Heraclitus isn’t necessarily used by other languages, it remains a core component of our compiler that is language agnostic. This separation makes it easier to maintain it |
Ph0enixKM
left a comment
Ph0enixKM
left a comment
There was a problem hiding this comment.
It's almost perfect. I'd leave that test though.
this is for amber-lang/amber#733
also @Ph0enixKM since this is a security issue its best if you release the new version immediately once this is merged