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

Can we avoid try catch with forOf? #1773

Closed
pflannery opened this Issue Feb 26, 2015 · 25 comments

Comments

Projects
None yet
8 participants
@pflannery
Contributor

pflannery commented Feb 26, 2015

currently this:

let counter = 0;
let testMap = [];
for (var key of testMap.keys()) {
    counter++;
}

produces this:

  "use strict";
  var counter = 0;
  var testMap = [];
  var $__3 = true;
  var $__4 = false;
  var $__5 = undefined;
  try {
    for (var $__1 = void 0,
        $__0 = (testMap.keys())[$traceurRuntime.toProperty(Symbol.iterator)](); !($__3 = ($__1 = $__0.next()).done); $__3 = true) {
      var key = $__1.value;
      {
        counter++;
      }
    }
  } catch ($__6) {
    $__4 = true;
    $__5 = $__6;
  } finally {
    try {
      if (!$__3 && $__0.return != null) {
        $__0.return();
      }
    } finally {
      if ($__4) {
        throw $__5;
      }
    }
  }

is there a way to opt out of this try catch?
I'm guessing the only way is write the for statement manually for the iteration

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv Feb 26, 2015

Collaborator

The try/catch is needed to handle the return() part.

I agree it is super sucky but the spec mandates it.

I don't think there is a way around this :'(

Collaborator

arv commented Feb 26, 2015

The try/catch is needed to handle the return() part.

I agree it is super sucky but the spec mandates it.

I don't think there is a way around this :'(

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv Feb 26, 2015

Collaborator
Collaborator

arv commented Feb 26, 2015

@mnieper

This comment has been minimized.

Show comment
Hide comment
@mnieper

mnieper Feb 26, 2015

There's probably a way to reformulate the transpiled output, but I don't see any reformulation that looks prettier. The point is that the loop body could throw, the iterator could throw and the return function of the iterator could throw.

If Traceur had a sloppy mode, one could think of disabling all exception handling in this mode. But enable such a mode at the global level of a program sounds hazardous.

mnieper commented Feb 26, 2015

There's probably a way to reformulate the transpiled output, but I don't see any reformulation that looks prettier. The point is that the loop body could throw, the iterator could throw and the return function of the iterator could throw.

If Traceur had a sloppy mode, one could think of disabling all exception handling in this mode. But enable such a mode at the global level of a program sounds hazardous.

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 26, 2015

Contributor

imo we should be able to define our own safe guards for the iterator and anything that happens in the body just like we would a for statement. This would be better than Traceur trying to doing it for us. A good source map will help diagnose back to the source if anything fails.

Also what is this return()? is that not something we too can handle ourselves if we expect a return method to be called?

Contributor

pflannery commented Feb 26, 2015

imo we should be able to define our own safe guards for the iterator and anything that happens in the body just like we would a for statement. This would be better than Traceur trying to doing it for us. A good source map will help diagnose back to the source if anything fails.

Also what is this return()? is that not something we too can handle ourselves if we expect a return method to be called?

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 26, 2015

Contributor

just to add this is what babel outputs

Contributor

pflannery commented Feb 26, 2015

just to add this is what babel outputs

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv Feb 26, 2015

Collaborator

@pflannery That is pretty much what we used to output before we added support for return() which is required for the spec.

Try this:

function* f() {
  try {
    yield 1;
    yield 2;
  } finally {
    console.log('Cleanup');
  }
}

for (var x of f()) {
  console.log(x);
  throw 42;
}

Notice how we print Cleanup in this case.

@sebmck FYI

Collaborator

arv commented Feb 26, 2015

@pflannery That is pretty much what we used to output before we added support for return() which is required for the spec.

Try this:

function* f() {
  try {
    yield 1;
    yield 2;
  } finally {
    console.log('Cleanup');
  }
}

for (var x of f()) {
  console.log(x);
  throw 42;
}

Notice how we print Cleanup in this case.

@sebmck FYI

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 26, 2015

@arv Yeah this was discussed in babel/babel#838, still not sure if the performance penalty is worth is which is unfortunate since I'm a massive believer in spec compliancy. Currently iterator.return() is only called on break.

Edit: Although thinking back on it this would be a scenario for loose mode and the default behaviour should be the try-catch. Thanks for the heads up!

kittens commented Feb 26, 2015

@arv Yeah this was discussed in babel/babel#838, still not sure if the performance penalty is worth is which is unfortunate since I'm a massive believer in spec compliancy. Currently iterator.return() is only called on break.

Edit: Although thinking back on it this would be a scenario for loose mode and the default behaviour should be the try-catch. Thanks for the heads up!

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 26, 2015

Contributor

I'm still of the opinion that this kind of scenario should be left to the developer to handle when applicable and not constantly by a transpiler.
Closing\disposing just seems like what a developer should ensure and when an unexpected error does occur its down to the garbage collector to ensure all resources are reclaimed.

Also is there a way to detect forOf in native? or is it just a matter of a try/catch test over a forOf statement. Because it would be nice if Traceur at runtime didn't polyfill when forOf is native.

Contributor

pflannery commented Feb 26, 2015

I'm still of the opinion that this kind of scenario should be left to the developer to handle when applicable and not constantly by a transpiler.
Closing\disposing just seems like what a developer should ensure and when an unexpected error does occur its down to the garbage collector to ensure all resources are reclaimed.

Also is there a way to detect forOf in native? or is it just a matter of a try/catch test over a forOf statement. Because it would be nice if Traceur at runtime didn't polyfill when forOf is native.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 26, 2015

Member

There's no way this should be handled by a developer. @arv's test case gives a clear example of behavior that a proper transpiler must support.

Member

domenic commented Feb 26, 2015

There's no way this should be handled by a developer. @arv's test case gives a clear example of behavior that a proper transpiler must support.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 26, 2015

@pflannery Wait, a transpiler shouldn't be concerned about spec compliancy? I don't understand.

kittens commented Feb 26, 2015

@pflannery Wait, a transpiler shouldn't be concerned about spec compliancy? I don't understand.

@johnjbarton

This comment has been minimized.

Show comment
Hide comment
@johnjbarton

johnjbarton Feb 26, 2015

Contributor

@pflannery A dev concerned with this level of detail avoid for-of in the rare performance sensitive case. Making these kinds of things optional is too much work for the benefit, compared to other issues.

Contributor

johnjbarton commented Feb 26, 2015

@pflannery A dev concerned with this level of detail avoid for-of in the rare performance sensitive case. Making these kinds of things optional is too much work for the benefit, compared to other issues.

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 27, 2015

Contributor

@johnjbarton I'm happy to write for statements manually for the iteration. Would be awesome if native would be used when available instead of poly-filling

Wait, a transpiler shouldn't be concerned about spec compliancy? I don't understand.
@sebmck I've not said that at all.

Contributor

pflannery commented Feb 27, 2015

@johnjbarton I'm happy to write for statements manually for the iteration. Would be awesome if native would be used when available instead of poly-filling

Wait, a transpiler shouldn't be concerned about spec compliancy? I don't understand.
@sebmck I've not said that at all.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 27, 2015

@pflannery

I'm still of the opinion that this kind of scenario should be left to the developer to handle when applicable and not constantly by a transpiler.

kittens commented Feb 27, 2015

@pflannery

I'm still of the opinion that this kind of scenario should be left to the developer to handle when applicable and not constantly by a transpiler.

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 27, 2015

Contributor

@sebmck now your just being childish.

Contributor

pflannery commented Feb 27, 2015

@sebmck now your just being childish.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 27, 2015

@pflannery Sorry, was just quoting what lead me to that assumption.

kittens commented Feb 27, 2015

@pflannery Sorry, was just quoting what lead me to that assumption.

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 27, 2015

Contributor

@sebmck ah ok no worries

Contributor

pflannery commented Feb 27, 2015

@sebmck ah ok no worries

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 27, 2015

Contributor

@arv I wanted to amend the transformForOfStatement so that the runtime generated code checks to see if the iterator has a return() implementation and if so then wrap the for-of in a try-catch, otherwise transform without try-catch. Because currently it's rethrowing the exception regardless.

This is what I want to PR

var ${iter} = (${tree.collection})[$traceurRuntime.toProperty(Symbol.iterator)]();
var ${normalCompletion} = true;
if (${iter}.return != null) {
  var ${throwCompletion} = false;
  var ${exception} = undefined;
  try {
    ${innerStatement}
  } catch (${ex}) {
    ${throwCompletion} = true;
    ${exception} = ${ex};
  } finally {
    try {
      if (!${normalCompletion}) {
        ${iter}.return();
      }
    } finally {
      if (${throwCompletion}) {
        throw ${exception};
      }
    }
  }
} else {
  ${innerStatement}
}

I think this leaves people to handle their own exceptions and still very closely meets the spec.

@arv what say you? you ok with me PR'ing this?

Contributor

pflannery commented Feb 27, 2015

@arv I wanted to amend the transformForOfStatement so that the runtime generated code checks to see if the iterator has a return() implementation and if so then wrap the for-of in a try-catch, otherwise transform without try-catch. Because currently it's rethrowing the exception regardless.

This is what I want to PR

var ${iter} = (${tree.collection})[$traceurRuntime.toProperty(Symbol.iterator)]();
var ${normalCompletion} = true;
if (${iter}.return != null) {
  var ${throwCompletion} = false;
  var ${exception} = undefined;
  try {
    ${innerStatement}
  } catch (${ex}) {
    ${throwCompletion} = true;
    ${exception} = ${ex};
  } finally {
    try {
      if (!${normalCompletion}) {
        ${iter}.return();
      }
    } finally {
      if (${throwCompletion}) {
        throw ${exception};
      }
    }
  }
} else {
  ${innerStatement}
}

I think this leaves people to handle their own exceptions and still very closely meets the spec.

@arv what say you? you ok with me PR'ing this?

@mnieper

This comment has been minimized.

Show comment
Hide comment
@mnieper

mnieper Feb 27, 2015

@pflannery When do you want to check whether the iterator has a return() implementation? The iterator could have no return property when the loop begins but could have gained one somewhere in the middle of the loop.

mnieper commented Feb 27, 2015

@pflannery When do you want to check whether the iterator has a return() implementation? The iterator could have no return property when the loop begins but could have gained one somewhere in the middle of the loop.

@pflannery

This comment has been minimized.

Show comment
Hide comment
@pflannery

pflannery Feb 27, 2015

Contributor

@mnieper your right I didnt think about iterators being that wild. Would be nice if the spec had things like Symbol.DisposableIterator, Symbol.ThrowableIterator to let compilers know what they need to do :'(..

Contributor

pflannery commented Feb 27, 2015

@mnieper your right I didnt think about iterators being that wild. Would be nice if the spec had things like Symbol.DisposableIterator, Symbol.ThrowableIterator to let compilers know what they need to do :'(..

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx Jun 30, 2017

Does "loose mode" disable the try-catch?

If not, is there any other way short of modifying the code for the transpiler manually?

Because the try-catch in for-of loops really messes with the ability to debug -- when an error occurs, I do not get any of the local variables in the watch because it's thrown much higher in the call-stack. Sometimes a dozen levels higher.

I know there's a "break on all exceptions" toggle in Chrome, but that's also a problem because some libraries use try-catch to detect eg. browser abilities, meaning you then have to wade through those as well.

It seems there really should be a configuration option to disable the try-catch. The default config can have it enabled, to match the spec, but for the majority of people the try-catch isn't necessary and they'd benefit more from having normal debugging abilities.

Venryx commented Jun 30, 2017

Does "loose mode" disable the try-catch?

If not, is there any other way short of modifying the code for the transpiler manually?

Because the try-catch in for-of loops really messes with the ability to debug -- when an error occurs, I do not get any of the local variables in the watch because it's thrown much higher in the call-stack. Sometimes a dozen levels higher.

I know there's a "break on all exceptions" toggle in Chrome, but that's also a problem because some libraries use try-catch to detect eg. browser abilities, meaning you then have to wade through those as well.

It seems there really should be a configuration option to disable the try-catch. The default config can have it enabled, to match the spec, but for the majority of people the try-catch isn't necessary and they'd benefit more from having normal debugging abilities.

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx Jun 30, 2017

Nevermind, it seems that the "loose" config-option does exactly what I wanted. : )

For people wondering, here is how you enable it:

  • In your babel config's "plugins" array, add ["babel-plugin-transform-es2015-for-of", {loose: true}] instead of just the string.

If you're using the es2015 preset instead of the individual plugins, you'd instead add the {loose: true} option to the preset's entry line, eg. ["babel-preset-es2015", {loose: true}]. (although this then enables loose-mode for some other plugins as well, so make sure these other changes won't cause problems in your project)

More info can be found here (though may be outdated): http://web.archive.org/web/20150215181702/http://babeljs.io/docs/usage/loose

Venryx commented Jun 30, 2017

Nevermind, it seems that the "loose" config-option does exactly what I wanted. : )

For people wondering, here is how you enable it:

  • In your babel config's "plugins" array, add ["babel-plugin-transform-es2015-for-of", {loose: true}] instead of just the string.

If you're using the es2015 preset instead of the individual plugins, you'd instead add the {loose: true} option to the preset's entry line, eg. ["babel-preset-es2015", {loose: true}]. (although this then enables loose-mode for some other plugins as well, so make sure these other changes won't cause problems in your project)

More info can be found here (though may be outdated): http://web.archive.org/web/20150215181702/http://babeljs.io/docs/usage/loose

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv Jun 30, 2017

Collaborator

Babel != Traceur

Collaborator

arv commented Jun 30, 2017

Babel != Traceur

@ddcq

This comment has been minimized.

Show comment
Hide comment
@ddcq

ddcq Mar 5, 2018

I've got a way to produce the simpliest javascript code with Babel. "loose" config-option must be true.

let counter = 0;
let testMap = [];
for (var key of [...testMap.keys()]) {
    counter++;
}

it produce this code.

"use strict";

var counter = 0;
var testMap = [];

var _arr = [].concat(testMap.keys());

for (var _i = 0; _i < _arr.length; _i++) {
    var key = _arr[_i];
    counter++;
}

Work on a copy of the array ensure hat you can't have side effects if you modify the array in the for loop.

ddcq commented Mar 5, 2018

I've got a way to produce the simpliest javascript code with Babel. "loose" config-option must be true.

let counter = 0;
let testMap = [];
for (var key of [...testMap.keys()]) {
    counter++;
}

it produce this code.

"use strict";

var counter = 0;
var testMap = [];

var _arr = [].concat(testMap.keys());

for (var _i = 0; _i < _arr.length; _i++) {
    var key = _arr[_i];
    counter++;
}

Work on a copy of the array ensure hat you can't have side effects if you modify the array in the for loop.

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv Mar 5, 2018

Collaborator

Both for-of and Maps are natively supported by ALL modern browsers. You might want to consider not transpiling these for both better runtime and download performance.

Collaborator

arv commented Mar 5, 2018

Both for-of and Maps are natively supported by ALL modern browsers. You might want to consider not transpiling these for both better runtime and download performance.

@ddcq

This comment has been minimized.

Show comment
Hide comment
@ddcq

ddcq Mar 6, 2018

7% of users continue to browse the Internet with the old Internet Explorer 11. My clients can't just avoid 7% of their clients. But your point of view is correct for non-B2B2C websites.
Android browser compatibility minimum version is 5.1.

ddcq commented Mar 6, 2018

7% of users continue to browse the Internet with the old Internet Explorer 11. My clients can't just avoid 7% of their clients. But your point of view is correct for non-B2B2C websites.
Android browser compatibility minimum version is 5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment