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

Improper void value expression #2821

Closed
nirvdrum opened this issue Jan 7, 2023 · 5 comments · Fixed by #2823
Closed

Improper void value expression #2821

nirvdrum opened this issue Jan 7, 2023 · 5 comments · Fixed by #2823

Comments

@nirvdrum
Copy link
Collaborator

nirvdrum commented Jan 7, 2023

We have a parsing difference from MRI that results in an improper SyntaxError. It's related to returning from a begin block used in an assignment.

def m
  x = begin
    if true 
      :hi
    else
      return nil
    end
  end
end

puts m
> ruby -v void.rb
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [arm64-darwin21]
hi

> jt ruby -v void.rb
truffleruby 23.0.0-dev-57e53f8a*, like ruby 3.1.2, GraalVM CE JVM [aarch64-darwin]
truffleruby: void.rb:9: void value expression (SyntaxError)

This could possibly be an MRI bug and is almost certainly an application logic error. If the return expression isn't nested in a conditional like it is here, then MRI does raise the SyntaxError. E.g.:

def m
  x = begin
    return nil
  end
end

m
> ruby -v void2.rb
ruby 3.2.0 (2022-12-25 revision a528908271) [arm64-darwin21.5.0]
void2.rb: void2.rb:4: void value expression (SyntaxError)

> jt ruby -v void2.rb
truffleruby 23.0.0-dev-57e53f8a*, like ruby 3.1.2, GraalVM CE JVM [aarch64-darwin]
truffleruby: void2.rb:5: void value expression (SyntaxError)
@eregon
Copy link
Member

eregon commented Jan 7, 2023

Could you report this to https://bugs.ruby-lang.org/ to see if CRuby considers it a bug?

Also, is it literally if true in the code? If so, that might be pruned early by CRuby during parsing/compiling to bytecode and that might explain why the first snippet "accidentally" works on CRuby.

@eregon
Copy link
Member

eregon commented Jan 7, 2023

Re if true it doesn't seem to matter, I think we can ignore that.

Playing with it on CRuby I guess the error means return nil is a void-value expression (because it doesn't return a value) and so not a valid value for an assignment.

Then maybe the question is if one branch is void and the other not should it be an error or not.
Or maybe this whole pattern should really only be a warning and not an error.
So I think useful to ask on the CRuby tracker.

@eregon
Copy link
Member

eregon commented Jan 7, 2023

def m
  x = begin
    if rand < 0.5
      return :hi
    else
      return nil
    end
  end
end

puts m

is an error on CRuby. So I guess if all branch are void it complains.
JRuby has the same behavior as TruffleRuby, if any branch is void it complains.
Seems worth fixing for compatibility.
OTOH this seems needlessly expensive to check during parsing/translating, maybe we should not do this check for void expressions at all.

@eregon
Copy link
Member

eregon commented Jan 9, 2023

The method for this is org.truffleruby.parser.parser.ParserSupport#value_expr, I'll fix it.

@eregon
Copy link
Member

eregon commented Jan 9, 2023

#2823 and I think no need to ask CRuby about this, the CRuby behavior seems sensible, i.e., only error if none of the branch returns a value (i.e., is non-void, i.e., does not exit via break/return/etc)

sophie-kaleba pushed a commit to sophie-kaleba/truffleruby that referenced this issue Jan 16, 2023
sophie-kaleba pushed a commit to sophie-kaleba/truffleruby that referenced this issue Jan 17, 2023
sophie-kaleba pushed a commit to sophie-kaleba/truffleruby that referenced this issue Feb 16, 2023
eregon added a commit to ruby/spec that referenced this issue Feb 27, 2023
john-heinnickel pushed a commit to thermofisher-jch/truffleruby that referenced this issue Aug 16, 2023
seven1m pushed a commit to seven1m/ruby_spec that referenced this issue Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants