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

fix rescue’s default behavior #687

Closed
wants to merge 3 commits into from
Closed

fix rescue’s default behavior #687

wants to merge 3 commits into from

Conversation

3cp
Copy link
Contributor

@3cp 3cp commented Jan 8, 2015

When no type is given, “rescue” should catch StandardError by default,
NOT all types of Exceptions.

I am surprised rubyspec's corelib/language/rescue_spec.rb did not test this.

Chunpeng Huo added 2 commits January 8, 2015 23:42
When no type is given, “rescue” should catch StandardError by default,
*NOT* all types of Exceptions.
@3cp
Copy link
Contributor Author

3cp commented Jan 9, 2015

There is some weird bug on my last commit, fails on mri 2.0 and 2.2 (the only two run mspec on travis?)
The compiled javascript is broken on spec/corelib/language/precedence_spec.rb

I can confirm this is the only test having trouble.

  it "? : has higher precedence than rescue" do
    (true ? oops : 0 rescue 10).should == 10
  end

@3cp
Copy link
Contributor Author

3cp commented Jan 9, 2015

anyway, found another bug related.
try in http://opalrb.org/try/

a = true ? 2 : 0 rescue 10
puts a

nothing printed due to a missing return in compiled js.

EDIT:

a = true ? 2 : 0 rescue 10                         # fail
a = (true ? 2 : 0) rescue 10                       # pass
a = begin; true ? 2 : 1; rescue; 10; end           # fail
a = begin; (true ? 2 : 1); rescue; 10; end         # fail

@3cp
Copy link
Contributor Author

3cp commented Jan 9, 2015

one more serious bug on rescue.
still exists in my branch.

begin
  raise IOError, 'foo'
rescue RangeError              # this one is correct
rescue TypeError               # miss a return
rescue IOError                 # following two lines disappear in js
  puts "I got #{$!.message}"
end
/* Generated by Opal 0.7.0.beta2 */
(function(Opal) {
  Opal.dynamic_require_severity = "error";
  var self = Opal.top, $scope = Opal, nil = Opal.nil, $breaker = Opal.breaker, $slice = Opal.slice;

  Opal.add_stubs(['$raise']);
  try {
  return self.$raise($scope.get('IOError'), "foo")
  } catch ($err) {if (Opal.rescue($err, [$scope.get('RangeError')])) {
    return nil
    }else if (Opal.rescue($err, [$scope.get('TypeError')])) {
    nil
    }else { throw $err; }
  }
})(Opal);

@elia
Copy link
Member

elia commented Jan 20, 2015

Got caught by the latter a few days ago, anyway I'm (slowly) working to bring in CRuby tests, maybe they cover these cases.

elia added a commit that referenced this pull request Jan 2, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
elia added a commit that referenced this pull request Jan 2, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
elia added a commit that referenced this pull request Jan 2, 2016
elia added a commit that referenced this pull request Jan 2, 2016
@elia
Copy link
Member

elia commented Jan 2, 2016

Thanks! merged in f1c7e73

@huochunpeng I also fixed the last bug you pointed out in #1269

@3cp
Copy link
Contributor Author

3cp commented Jan 3, 2016

@elia, the bug on one line rescue is still not resolved. You may want to open another ticket.

a = true ? 2 : 0 rescue 10                         # fail
a = (true ? 2 : 0) rescue 10                       # pass
a = begin; true ? 2 : 1; rescue; 10; end           # fail
a = begin; (true ? 2 : 1); rescue; 10; end         # fail

To illustrate

a = true ? 2 : 0 rescue 10

Compile result doesn't give variable "a" any val because of missing return in try block.

/* Generated by Opal 0.10.0.dev */
(function(Opal) {
  Opal.dynamic_require_severity = "error";
  var OPAL_CONFIG = { method_missing: true, arity_check: false, freezing: true, tainting: true };
  var $a, self = Opal.top, $scope = Opal, nil = Opal.nil, $breaker = Opal.breaker, $slice = Opal.slice, a = nil;

  return a = (function() {try {(function() {if ((($a = true) !== nil && (!$a.$$is_boolean || $a == true))) {
    return 2
    } else {
    return 0
  }; return nil; })() } catch ($err) { if (Opal.rescue($err, [$scope.get('StandardError')])) {return 10}else { throw $err; } }})()
})(Opal);
a = (true ? 2 : 0) rescue 10

Extra brackets yield correct result

 /* Generated by Opal 0.10.0.dev */
(function(Opal) {
  Opal.dynamic_require_severity = "error";
  var OPAL_CONFIG = { method_missing: true, arity_check: false, freezing: true, tainting: true };
  var $a, self = Opal.top, $scope = Opal, nil = Opal.nil, $breaker = Opal.breaker, $slice = Opal.slice, a = nil;

  return a = (function() {try {return ((function() {if ((($a = true) !== nil && (!$a.$$is_boolean || $a == true))) {
    return 2
    } else {
    return 0
  }; return nil; })()) } catch ($err) { if (Opal.rescue($err, [$scope.get('StandardError')])) {return 10}else { throw $err; } }})()
})(Opal);

@elia
Copy link
Member

elia commented Jan 3, 2016

@huochunpeng thanks!
yes, a separate issue would be better, do you mind opening it with a link to this one in the description?

@3cp 3cp mentioned this pull request Jan 3, 2016
elia added a commit that referenced this pull request Jan 4, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
elia added a commit that referenced this pull request Jan 6, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
elia added a commit that referenced this pull request Jan 6, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
elia added a commit that referenced this pull request Jan 9, 2016
Previously only the first two rescues were being compiled.

ref: #687 (comment)
iliabylich pushed a commit to iliabylich/opal that referenced this pull request Jan 20, 2016
Previously only the first two rescues were being compiled.

ref: opal#687 (comment)
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

Successfully merging this pull request may close these issues.

3 participants