-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
repl: Mitigate vm #548 function redefinition issue #7624
Conversation
```js node 🙈 ₹ git:(upstream ⚡ repl-tmp-548) ./node > function name() { return "node"; }; undefined > name() 'node' > function name() { return "nodejs"; }; undefined > name() 'nodejs' > ```
@@ -328,6 +328,11 @@ function error_test() { | |||
// or block comment. https://github.com/nodejs/node/issues/3611 | |||
{ client: client_unix, send: 'a = 3.5e', | |||
expect: /^SyntaxError: Invalid or unexpected token/ }, | |||
// Mitigate #548 issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd prefer this be the full URL to the issue rather than just the issue number. (Some issues scattered throughout the code are in nodejs/node-archive (formerly joyent/node, I believe) rather than nodejs/node and using the full URL eliminates guesswork. (Not the comment 3 lines above contains a full issue URL.)
LGTM Btw, there’s one edge case with unexpected results that will enabled by this change: > function a() { return 42; } ()
undefined
> a
42 I think it’s okay to accept that as a side effect, though. |
Maybe add the edge case identified by @addaleax as a known-issues test? |
44ddbc7
to
f1eb84e
Compare
|
||
const expected = '1\n[Function a]\n'; | ||
const got = r.output.accumulator.join(''); | ||
assert.equal(got, expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict version
Change LGTM, pending CI. We should accept that this will change multiline strings and comments as well. |
@thefourtheye I am not quite sure what you meant. |
This will replace the definitions even if they are defined within comments or mutiline strings, right? Edit: nvm. This PR doesn't concern Strings and comments. LGTM stays. |
LGTM |
Landed in bb9eabe |
```js node 🙈 ₹ git:(upstream ⚡ repl-tmp-548) ./node > function name() { return "node"; }; undefined > name() 'node' > function name() { return "nodejs"; }; undefined > name() 'nodejs' > ``` Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Lance Ball <lball@redhat.com>
This isn't landing cleanly on v6.x Mind sending a backport PR to v6.x? Thanks! |
@evanlucas branch name? |
@princejwesley v6.x |
deps: update V8 to 6a72f3 Fixes nodejs#548 Makes nodejs#7624 unnecessary.
`test/known_issues/test-repl-function-redefinition-edge-case.js` had been introduced as a part of nodejs#7624 but the meat of the test became fixed in 007386e. Despite that, the test continued to fail since it was broken itself: there was a missing colon in the expected output. This commit adds the missing colon and moves the test from `test/known_issues` to `test/parallel`.
`test/known_issues/test-repl-function-redefinition-edge-case.js` had been introduced as a part of #7624 but the meat of the test became fixed in 007386e. Despite that, the test continued to fail since it was broken itself: there was a missing colon in the expected output. This commit adds the missing colon and moves the test from `test/known_issues` to `test/parallel`. PR-URL: #11772 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`test/known_issues/test-repl-function-redefinition-edge-case.js` had been introduced as a part of nodejs#7624 but the meat of the test became fixed in 007386e. Despite that, the test continued to fail since it was broken itself: there was a missing colon in the expected output. This commit adds the missing colon and moves the test from `test/known_issues` to `test/parallel`. PR-URL: nodejs#11772 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`test/known_issues/test-repl-function-redefinition-edge-case.js` had been introduced as a part of nodejs#7624 but the meat of the test became fixed in 007386e. Despite that, the test continued to fail since it was broken itself: there was a missing colon in the expected output. This commit adds the missing colon and moves the test from `test/known_issues` to `test/parallel`. PR-URL: nodejs#11772 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
repl
Description of change
Transformed
function XXX(...)
tovar XXX = function XXX(...)
in order to mitigate vm #548 function redefinition issueBefore
After