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

repl: Copying tab characters into the repl calls tabComplete #5958

Closed
wants to merge 1 commit into from
Closed

repl: Copying tab characters into the repl calls tabComplete #5958

wants to merge 1 commit into from

Conversation

ghaiklor
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

repl

Description of change

Issue - #5954

When you are copying tab-indented code into the repl, repl calls tabComplete function.
There is no actual pressing the button, so we need to ignore it.

@r-52 r-52 added lts-watch-v4.x repl Issues and PRs related to the REPL subsystem. labels Mar 30, 2016
@Fishrock123
Copy link
Contributor

@ghaiklor Think you could make a test for it? :D

@ghaiklor
Copy link
Contributor Author

@Fishrock123 yeah, I think I could :)

@addaleax
Copy link
Member

Not sure, but might it be a better idea to replace a tab with spaces – or even just a single space – in this scenario? I’d guess just omitting the tabs could mess with the input in unexpected ways, e.g. if someone copypastes tab-separated CSV into a REPL (and I imagine that happens from time to time) or something like that.

@Fishrock123
Copy link
Contributor

Not sure, but might it be a better idea to replace a tab with spaces

I'd agree. Ideally 2-4 spaces, but unsure if that is possible.

I’d guess just omitting the tabs could mess with the input in unexpected ways, e.g. if someone copypastes tab-separated CSV into a REPL

Hmmm true, it should probably check that we aren't in a string. I think there are existing checks for similar string things.

@ghaiklor
Copy link
Contributor Author

@Fishrock123 @addaleax I'm not sure if that possible.

If you are pasting some code to the REPL, for each char in that string emits keypress event separately. In result, when it sees tab character it immediately calls tabComplete() emitting the error above.

I was trying to achieve this by replacing parsed value like this

if (r[i] === '\t' && r[i + 1]) {
  r[i] = '  ';
}

but r[i] is read-only value. So if we want to achieve replacing tabs with spaces, I need to dig into string decoder.

@addaleax
Copy link
Member

@ghaiklor I’m not an expert on the readline/REPL stuff or anything, but if I read the code correctly, you could do something like

let v = r[i];
if (r[i] === '\t' && r[i + 1]) {
  v = ' ';
}

try {
  stream[ESCAPE_DECODER].next(v);
} 

in the inner loop?

@ghaiklor
Copy link
Contributor Author

@addaleax you've read my mind 👍
Just tested this locally and works. Running tests now...

@ghaiklor
Copy link
Contributor Author

@addaleax this test-case is failing 😄

rli.on('line', function(line) {
  assert.equal(line, 'foo');
  assert.strictEqual(called, false);
  called = true;
});
fi.emit('data', '\tfo\to\t');
fi.emit('data', '\n');
assert.ok(called);
rli.close();

Looking now...

@addaleax
Copy link
Member

I think it would be better to consider the test case incorrect here. What I imagine it’s supposed to check is that entering the sequence '\tfo\to\t' on a keyboard (i.e. keypress for keypress) keeps the word foo intact, not pasting the sequence as a whole. The test should pass when you re-write the bottom lines like this:

for (var character of '\tfo\to\t\n') {
  fi.emit('data', character);
}
assert.ok(called);
rli.close();

IMHO that change would be okay.

@addaleax
Copy link
Member

And btw, if it’s possible, I’d agree with @Fishrock123 in that 2 or 4 spaces would be optimal. Would there be any reasons against writing v = '  ' and doing something like this inside the try block?

for (var j = 0; j < v.length; j++) {
  stream[ESCAPE_DECODER].next(v[j]);
}

(Performance? Is that relevant here?)

@ghaiklor
Copy link
Contributor Author

@addaleax agreed. I don't see cases when user can type \tfo\to\t or similar.
Also, performance is not critical here, so we can insert 2 spaces.
Checking last changes and updating the branch...

@@ -920,8 +920,13 @@ function emitKeypressEvents(stream) {
var r = stream[KEYPRESS_DECODER].write(b);
if (r) {
for (var i = 0; i < r.length; i++) {
// TODO(ghaiklor): maybe makes sense to configure indent size here on pasting
var next = (r[i] === '\t' && typeof r[i + 1] === 'string') ? ' ' : r[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter says these lines are too long.

@ghaiklor
Copy link
Contributor Author

@addaleax didn't notice, thanks.

@benjamingr
Copy link
Member

We can only replace spaces with tabs if we're sure we're not inside a string literal.

@ghaiklor
Copy link
Contributor Author

@benjamingr I'm not sure how to do this. We can add flag isStringLiteral and toggle it when you got chunk with ' or ".

But, I think, fix become complex.

UPD:

var isStringLiteral = false;
if (r[i] === '\'' || r[i] === '"') isStringLiteral = !isStringLiteral;

And based on isStringLiteral value pushing spaces or tab symbol in the encoder.

@benjamingr
Copy link
Member

String literals can also start with a backtick, what about RegExp literals?

I was pointing out that the applied fix replacing tabs with spaces doesn't apply. I think we need to fix it at a different level.

What if we do not trigger autocomplete if more than one character was entered at once or if another character was entered synchronously or some other similar logic?

@ghaiklor
Copy link
Contributor Author

@benjamingr I was trying to achieve this, but unsuccessful. The problem is, when you paste code into the repl, it starts emits keypress events for each char as it was typing, not the pasting. So we can't know for sure if more than one character was entered.

So, fix should be somewhere in keypress logic, when repl start emits kepress events.

@benjamingr
Copy link
Member

You need to buffer it, for example consider something like:

let buffer = [], t;
process.on('character', c => { // maybe a very short setTimeout
    clearImmediate(t);
    t = setImmediate(() => {
        // if buffer contains only a single character here, autocomplete is fine, otherwise
        // in all likelihood something was pasted.
        for(const char of sync) other.emit(char);
        buffer = [];
    });
    buffer.push(c);
});

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

Trying to solve this in other ways. Here is what I've found:

Completer triggers in lib/repl.js only at one function and this function is passes as an argument into readline Interface.

  Interface.call(this, {
    input: self.inputStream,
    output: self.outputStream,
    completer: complete,
    terminal: options.terminal,
    historySize: options.historySize
  });

So, I suppose, there is no sense to fix this in repl.js. All what complete does is takes input text and try to build suggestions. Or... We can fix it in this function (repl.js):

  function complete(text, callback) {
    self.complete(text, callback);
  }

That's the function that passes into Interface. I think, we can add check for tabs here. I'll try this now.

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

Yeah, complete is a better place to fix this. I can write like this:

  function complete(text, callback) {
    if (textPasted) return callback(null, []);
    self.complete(text, callback);
  }

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

@Fishrock123 @addaleax what you think about this solution?

It's more cleaner. If there is no text, when autocomplete triggers, it skips all the completer logic. However, we can't replace it with spaces.

@benjamingr
Copy link
Member

This is better, replacing it with spaces was only a way to fix the autocomplete issue.

Mind adding a line explaining why that if was added there so future readers can understand the problem being solved? With a comment generally LGTM.

@addaleax
Copy link
Member

addaleax commented Apr 5, 2016

Are you sure this works as expected? When I paste var a = 'a b'; into my console (tab inside the string), I get var a = 'assertb'; as the result, so it seems like tab completion is still triggered?

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

@addaleax I'm expecting that there is not tab symbols inside. Though, I'd tried press Tab with empty line and there is no autocomplete.

Still looking... I'll try to fix issue with completer trigger then in readline interface.

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

completer triggers only once in lib/readline in _ttyWrite method:

      case 'tab':
        // If tab completion enabled, do that...
        if (typeof this.completer === 'function') {
          this._tabComplete();
          break;
        }

The problem here, that it triggers inside _ttyWrite method which accepts already parsed char and key:

Interface.prototype._ttyWrite = function(s, key) {
  // ...
}

_ttyWrite triggers only in keypress event, where is also parsed char and key. That is not enough information at this step for fixing the issue.

  function onkeypress(s, key) {
    self._ttyWrite(s, key);
  }

IMHO, there is only 2 places where we can fix this:

  • repl.js and its completer function
  • readline.js and its emitKeypressEvents(stream) function that is responsible for parsing chars;

@ghaiklor
Copy link
Contributor Author

ghaiklor commented Apr 5, 2016

Still have no solution 👎

I checked other languages and their REPL.

Python doesn't have auto-complete by tab at all.
Ruby has the same issue with tabs.
Maybe, there is no correct and really working solution ? 😄

@addaleax
Copy link
Member

addaleax commented Apr 5, 2016

What do you think of something like this: addaleax/node@03501bd

It’s an approach that should work well and could be refined to something usable (e.g. add comments, possibly use something like .setCompletionEnabled(boolean) to Interface, etc.)? It has the advantage of being close to what is intended, namely temporarily disabling auto-completion, and applies to general readline Interfaces, not only REPLs.

Also not sure about changing the interface of emitKeypressEvents, but given that it has literally just been documented (#6024) that should be okay. If not, we should be able to work around that by adding another helper function.

And btw, Python v3 seems to do have auto-complete available, just not Python v2 (at least on my Ubuntu 15.10) :)

@addaleax
Copy link
Member

addaleax commented Apr 5, 2016

btw²: I know my proposal contains mistakes, the identifiers are certainly not named optimally, etc., but I think the idea is clear enough to discuss it. And Python3 drops tabs within strings, so maybe that doesn’t count. 😉

@addaleax addaleax closed this May 13, 2016
@ghaiklor ghaiklor deleted the fix/5954 branch May 13, 2016 15:54
evanlucas pushed a commit that referenced this pull request May 17, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
@MylesBorins
Copy link
Contributor

@nodejs/lts @nodejs/ctc do we want to backport this in v4.5.0?

@Fishrock123
Copy link
Contributor

@thealphanerd Probably.

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@MylesBorins
Copy link
Contributor

@nodejs/lts I have backported this to v4.x-staging with the intent of shipping it with the v4.5.0 rc

please let me know if you have any concerns. There will also be ample time to raise concerns during the rc period

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * ignore negative allocation lengths (Anna Henningsen)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * backport 9c927d0f01 from V8 upstream (Myles Borins)
    #7451
  * cherry-pick 68e89fb from v8's upstream (Fedor Indutny)
    #3779

Semver Patch:

* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 13, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * ignore negative allocation lengths (Anna Henningsen)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post portem data to imrpove object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
PR-URL: #5958
Fixes: #5954
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post portem data to imrpove object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 15, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 16, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 16, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants