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

Split output wrong for Node version 6.12.0 when trapping calls to RegExp.prototype.exec #17149

Closed
mtorp opened this issue Nov 20, 2017 · 4 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mtorp
Copy link

mtorp commented Nov 20, 2017

  • Version: v6.12.0
  • Platform: 4.10.0-38-generic rename node.js -> io.js #42~16.04.1-Ubuntu SMP Tue Oct 10 16:32:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem:

The following code snippet produces the output [ '.hover-yellow:hover' ] using Node version 6.12.0. The expected output is [ '.hover-yellow', ':hover' ] which is also the output produced by for example Node 7.10.0.

var orig = RegExp.prototype.exec;

RegExp.prototype.exec = function() {
    return Reflect.apply(orig, this, arguments);
};

var selector = ".hover-yellow:hover";
var r = /(?!^)(?=[.:])/g
console.log(selector.split(r));
@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency. labels Nov 20, 2017
@bnoordhuis
Copy link
Member

I can confirm. It's a more-or-less known bug in the v6.x implementation:

// TODO(adamk): this fast path is wrong with respect to this.global
// and this.sticky, but hopefully the spec will remove those gets
// and thus make the assumption of 'exec' having no side-effects
// more correct. Also, we doesn't ensure that 'exec' is actually
// a data property on RegExp.prototype.
var exec;
if (IS_REGEXP(this) && constructor === GlobalRegExp) {
exec = this.exec;
if (exec === RegExpSubclassExecJS) {
return %_Call(RegExpSplit, this, string, limit);
}
}

It's been fixed in newer V8 versions but in a way that can't be back-ported. A fix would have to be written from scratch.

@mscdex mscdex added the v6.x label Nov 20, 2017
@Announcement
Copy link

Announcement commented Nov 21, 2017

works just fine for me ;)

$ node -pe "'.hover-yellow:hover'.split(/(?!^)(?=[.:])/g)"

[ '.hover-yellow', ':hover' ]

$ node -pe 'process.version'

{ http_parser: '2.7.0',
  node: '10.0.0-pre',  
  v8: '6.4.332-node.0',
  uv: '1.16.1',        
  zlib: '1.2.11',      
  ares: '1.13.0',      
  modules: '61',       
  nghttp2: '1.25.0',   
  openssl: '1.0.2m',   
  icu: '60.1',         
  unicode: '10.0',     
  cldr: '32.0',        
  tz: '2017c' }

@Trott
Copy link
Member

Trott commented Nov 21, 2017

@Announcement The original poster wrote that the bug is in version 6.12.0 and was fixed by 7.10.0 at the latest. You're running 10.0.0-pre.

@apapirovski
Copy link
Member

There is no possible way this gets fixed in 6.x at this point. I'm going to close this out. Thanks for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants