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

Syntax errors for jumps within eval #2731

Closed
andrykonchin opened this issue Apr 24, 2024 · 9 comments
Closed

Syntax errors for jumps within eval #2731

andrykonchin opened this issue Apr 24, 2024 · 9 comments

Comments

@andrykonchin
Copy link
Member

andrykonchin commented Apr 24, 2024

For some operators (break, next, redo) when jump is incorrect (e.g. in a method body) SyntaxError might be not returned when scopes parsing option is not an empty Array (what is a case to implement Kernel#eval):

Prism.parse called with empty scopes, so syntax error is returned:

Prism.parse("def m; break; end", scopes: [])
=>
#<Prism::ParseResult:0x000000010da92000
 @comments=[],
 @data_loc=nil,
 @errors=[#<Prism::ParseError @type=:invalid_block_exit @message="Invalid break" @location=#<Prism::Location @start_offset=7 @length=5 start_line=1> @level=:syntax>],
 @magic_comments=[],
 @source=#<Prism::Source:0x000000010da726b0 @offsets=[0], @source="def m; break; end", @start_line=1>,
 @value=
  @ ProgramNode (location: (1,0)-(1,17))
  ├── locals: []
  └── statements:
      @ StatementsNode (location: (1,0)-(1,17))
      └── body: (length: 1)
          └── @ DefNode (location: (1,0)-(1,17))
              ├── name: :m
              ├── name_loc: (1,4)-(1,5) = "m"
              ├── receiver: ∅
              ├── parameters: ∅
              ├── body:
              │   @ StatementsNode (location: (1,7)-(1,12))
              │   └── body: (length: 1)
              │       └── @ BreakNode (location: (1,7)-(1,12))
              │           ├── arguments: ∅
              │           └── keyword_loc: (1,7)-(1,12) = "break"
              ├── locals: []
              ├── def_keyword_loc: (1,0)-(1,3) = "def"
              ├── operator_loc: ∅
              ├── lparen_loc: ∅
              ├── rparen_loc: ∅
              ├── equal_loc: ∅
              └── end_keyword_loc: (1,14)-(1,17) = "end",
 @warnings=[]>

Prism.parse called with not empty scopes, but no syntax error returned:

Prism.parse("def m; break; end", scopes: [[]])
=>
#<Prism::ParseResult:0x00000001104e8b10
 @comments=[],
 @data_loc=nil,
 @errors=[],
 @magic_comments=[],
 @source=#<Prism::Source:0x00000001107fb258 @offsets=[0], @source="def m; break; end", @start_line=1>,
 @value=
  @ ProgramNode (location: (1,0)-(1,17))
  ├── locals: []
  └── statements:
      @ StatementsNode (location: (1,0)-(1,17))
      └── body: (length: 1)
          └── @ DefNode (location: (1,0)-(1,17))
              ├── name: :m
              ├── name_loc: (1,4)-(1,5) = "m"
              ├── receiver: ∅
              ├── parameters: ∅
              ├── body:
              │   @ StatementsNode (location: (1,7)-(1,12))
              │   └── body: (length: 1)
              │       └── @ BreakNode (location: (1,7)-(1,12))
              │           ├── arguments: ∅
              │           └── keyword_loc: (1,7)-(1,12) = "break"
              ├── locals: []
              ├── def_keyword_loc: (1,0)-(1,3) = "def"
              ├── operator_loc: ∅
              ├── lparen_loc: ∅
              ├── rparen_loc: ∅
              ├── equal_loc: ∅
              └── end_keyword_loc: (1,14)-(1,17) = "end",
 @warnings=[]>

Related issue #1604

@kddnewton
Copy link
Collaborator

This is by design. When you pass scopes, we assume you are parsing an eval. When parsing an eval, a syntax error is not raised for those jumps. Instead, it relies on the compiler to evaluate whether or not they are valid. This matches CRuby semantics.

@andrykonchin
Copy link
Member Author

andrykonchin commented Apr 24, 2024

Hm, but SyntaxError is raised in CRuby:

$ ruby -v test.rb
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin23]
(eval at test.rb:1): (eval at test.rb:1):1: Invalid break (SyntaxError)

test.rb:

eval("def m; break; end")

Ah, you mean it's a responsibility of compiler. Got it 👍

@kddnewton
Copy link
Collaborator

Yeah, unfortunately. Consider for instance the difference between 0 rescue next and eval("0 rescue next"). The second raises Can't escape from eval with next.

@eregon
Copy link
Member

eregon commented Apr 25, 2024

Is it mostly about different error messages? I'm not sure if that difference is important enough to have this behavior differ with scopes.
For the def m; break; end case it seems to behave the same within or outside eval in CRuby, so it feels like Prism could always give a syntax error for that one, no?
It's not very important, but it would avoid duplicating this logic in the compiler/translator, where it feels wrong/annoying to do so.

@eregon
Copy link
Member

eregon commented Apr 25, 2024

Some other examples:
def m; retry; end Prism seems to always error for this, eval or not, good.
module M; yield; end Prism seems to only handle (= error) it if not in eval. This feels very much like a syntactic restriction, so would be nicer if Prism handled it always.

@kddnewton
Copy link
Collaborator

I'll try to find which tests were failing, but when I implemented this I got a couple more test-all tests passing. There are some things that are relying on this behavior.

@kddnewton
Copy link
Collaborator

Ahh, it was the ERB tests. We need this to not error out in case you're parsing an ERB file

@eregon
Copy link
Member

eregon commented May 3, 2024

We need this to not error out in case you're parsing an ERB file

I'm not following, could you give a concrete example?
Which specific jump should not error when parsing an ERB file?
And how comes there is no error from compile.c then, i.e. how come one can distinguish between parser and compile.c in that case? (eval does both).

@eregon
Copy link
Member

eregon commented May 3, 2024

Ah maybe it's related to Ripper? I've seen https://bugs.ruby-lang.org/issues/20460 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants