repl: Mitigate vm #548 function redefinition issue#7624
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
```js node 🙈 ₹ git:(upstream ⚡ repl-tmp-548) ./node > function name(){return "node"}; undefined > name() 'node' > function name(){return "nodejs"}; undefined > name() 'nodejs' > ```
test/parallel/test-repl.js Outdated
| // or block comment. https://github.com/nodejs/node/issues/3611 | ||
| {client: client_unix,send: 'a = 3.5e', | ||
| expect: /^SyntaxError:Invalidorunexpectedtoken/}, | ||
| // Mitigate #548 issue |
Member
There was a problem hiding this comment.
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.)
Member
addaleax commented Jul 9, 2016
LGTM Btw, there’s one edge case with unexpected results that will enabled by this change: >functiona(){return42;}()undefined>a42I think it’s okay to accept that as a side effect, though. |
Member
Trott commented Jul 10, 2016
Maybe add the edge case identified by @addaleax as a known-issues test? |
| const expected = '1\n[Function a]\n' | ||
| const got = r.output.accumulator.join(''); | ||
| assert.equal(got, expected); |
Contributor
thefourtheye commented Jul 10, 2016
Change LGTM, pending CI. We should accept that this will change multiline strings and comments as well. |
ContributorAuthor
@thefourtheye I am not quite sure what you meant. |
Contributor
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. |
Member
LGTM |
Member
lance commented Jul 15, 2016
Landed in bb9eabe |
addaleax referenced this pull request Jul 17, 2016
```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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Lance Ball <[email protected]>
Contributor
evanlucas commented Jul 19, 2016
This isn't landing cleanly on v6.x Mind sending a backport PR to v6.x? Thanks! |
ContributorAuthor
princejwesley commented Jul 19, 2016
@evanlucas branch name? |
Member
lance commented Jul 19, 2016
@princejwesley v6.x |
3 tasks
fhinkel added a commit to v8/node that referenced this pull request Sep 18, 2016
deps: update V8 to 6a72f3 Fixesnodejs#548 Makes nodejs#7624 unnecessary.
aqrln added a commit to aqrln/node that referenced this pull request Mar 9, 2017
`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`.
2 tasks
jasnell pushed a commit that referenced this pull request Mar 15, 2017
`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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
`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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
`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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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