Repository: microsoft/terminal (language: cpp)

Issue:
Terminal shows SOH and STX characters in Ruby's `pry` for some reason
### Windows Terminal version (or Windows build number)

Terminal 1.8.1521.0, Windows 10.0.19043.0

### Other Software

WSL2, running Manjaro Linux: https://github.com/sileshn/ManjaroWSL
Using Zsh and oh-my-zsh (but reproduced with Bash)
Fira Code Retina font (but run into this with other fonts)

```
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
```

```
Pry version 0.14.1 on Ruby 3.0.2
```

### Steps to reproduce

With everything installed properly (`pry` in `$PATH`):

```sh
➜  ~  pry
[1] pry(main)> ls Module
```

### Expected Behavior

No special characters or control codes should be shown:

```ruby
[1] pry(main)> ls Module
Module.methods: constants  nesting  used_modules
Module#methods:
  <              attr_writer              const_set              method_defined?             protected_method_defined?
  <=             autoload                 const_source_location  module_eval                 public_class_method
  <=>            autoload?                constants              module_exec                 public_constant
  ==             class_eval               define_method          name                        public_instance_method
  ===            class_exec               deprecate_constant     prepend                     public_instance_methods
  >              class_variable_defined?  freeze                 pretty_print                public_method_defined?
  >=             class_variable_get       include                pretty_print_cycle          remove_class_variable
  alias_method   class_variable_set       include?               private_class_method        remove_method
  ancestors      class_variables          included_modules       private_constant            singleton_class?
  attr           const_defined?           inspect                private_instance_methods    to_s
  attr_accessor  const_get                instance_method        private_method_defined?     undef_method
  attr_reader    const_missing            instance_methods       protected_instance_methods
[2] pry(main)>
```

### Actual Behavior

For some reason, special characters/control codes are being shown:

![image](https://user-images.githubusercontent.com/10534779/126884330-f4cceb7d-39bf-463c-8488-52616d12ea02.png)

```ruby
[1] pry(main)> ls Module
Module.methods: constants  nesting  used_modules
Module#methods:
  <              attr_writer              const_set              method_defined?             protected_method_defined?
  <=             autoload                 const_source_location  module_eval                 public_class_method
  <=>            autoload?                constants              module_exec                 public_constant
  ==             class_eval               define_method          name                        public_instance_method
  ===            class_exec               deprecate_constant     prepend                     public_instance_methods
  >              class_variable_defined?  freeze                 pretty_print                public_method_defined?
  >=             class_variable_get       include                pretty_print_cycle          remove_class_variable
  alias_method   class_variable_set       include?               private_class_method        remove_method
  ancestors      class_variables          included_modules       private_constant            singleton_class?
  attr           const_defined?           inspect                private_instance_methods    to_s
  attr_accessor  const_get                instance_method        private_method_defined?     undef_method
  attr_reader    const_missing            instance_methods       protected_instance_methods
[2] pry(main)>
```


Discussion / hints:
Another interesting thing, is in cases where the Terminal viewport is too small, the characters that get displayed are different: (`^A` and `^B` in this case)

```ruby
^A^B^A^BModule.methods^A^B^A^B: constants  nesting  used_modules
^A^B^A^BModule#methods^A^B^A^B:
  <
  <=
  <=>
  ==
  ===
  >
  >=
  alias_method
  ancestors
  attr
  attr_accessor
  attr_reader
  attr_writer
  autoload
  autoload?
  class_eval
  class_exec
  class_variable_defined?
  class_variable_get
  class_variable_set
  class_variables
  const_defined?
  const_get
  const_missing
  const_set
  const_source_location
  constants
```
Does this repro in the vintage console host?

I forget what the actual right behavior for SOH and STX are - I'd assume that we should just be eating those and doing nothing with them, but maybe they're getting stuck in the conpty buffer then getting round-tripped as U+FFFD. I haven't had enough coffee to remember - Usually @j4james is my expert on these things. 
> I forget what the actual right behavior for SOH and STX are - I'd assume that we should just be eating those and doing nothing with them

If we want to match the original DEC terminals, we should technically be eating most of the control characters which don't have defined behaviour, exactly like we do with `NUL`. That said, legacy DOS/Windows console apps would have expected those controls to be rendered, and we've already had complaints about the way we now drop `NUL` characters (see #8680).

In a perfect world, I'd eventually like for us to have different compatibility modes, so you could choose between things like strict VT, legacy ANSI.COM, or XTerm compatibility. In the meantime, though, I thought our current behavior was a reasonable compromise. We implement most of the controls that have defined meaning, but output those that aren't typically used in VT apps, so we still have a certain amount of legacy support.

But I don't have a strong opinion on this either way. If we decide we want to be strictly VT-compatible in this regard, I'd be happy to write a PR for it. Just be aware that it might trigger more bug reports from people running legacy apps.
> 
> 
> Does this repro in the vintage console host?

@zadjii-msft I wish I could tell you, but I can't run WSL in the Legacy Console:

![image](https://user-images.githubusercontent.com/10534779/128069745-fcc1f571-b568-4810-8052-580ed25cdb29.png)

Unless you meant, just running the whole thing in `cmd.exe`. In which case, `cmd.exe` can't even display some of the extended characters that otherwise work on Terminal 😅 

![image](https://user-images.githubusercontent.com/10534779/128069917-ffc91637-9559-4966-b31c-ca681734b3a1.png)

> 
> 
> > I forget what the actual right behavior for SOH and STX are - I'd assume that we should just be eating those and doing nothing with them
> 
> If we want to match the original DEC terminals, we should technically be eating most of the control characters which don't have defined behaviour, exactly like we do with `NUL`. That said, legacy DOS/Windows console apps would have expected those controls to be rendered, and we've already had complaints about the way we now drop `NUL` characters (see #8680).
> 
> In a perfect world, I'd eventually like for us to have different compatibility modes, so you could choose between things like strict VT, legacy ANSI.COM, or XTerm compatibility. In the meantime, though, I thought our current behavior was a reasonable compromise. We implement most of the controls that have defined meaning, but output those that aren't typically used in VT apps, so we still have a certain amount of legacy support.
> 
> But I don't have a strong opinion on this either way. If we decide we want to be strictly VT-compatible in this regard, I'd be happy to write a PR for it. Just be aware that it might trigger more bug reports from people running legacy apps.

@j4james Though I am not a contributor to Terminal, I think the approach of Compatibility Modes is a sound idea; I believe it's what other Terminal Emulators like iTerm2 do.
We will probably want to ignore more and more control characters that we can't do anything meaningful with _when VT mode is enabled_. Apps usually¹ opt in to this, so they can be coerced into behaving.

https://github.com/pry/pry/commit/97be53b426f7f0a762130b54623faa6946f2c09a introduced `\001` and `\002` for... some reason? To "escape" the control sequences.

It looks like they wrap color escapes in `\x1` and `\x2` so that they can later *remove* them if the user doesn't want color. That is not the best design, as it rather requires them to create a bunch of information that they then have to destroy!

¹ Of course, Terminal enables it by default. We likely do need some measure of "compatibility modes" like @j4james suggests.
the upstream bug is https://github.com/pry/pry/issues/2185, though, we'll likely want to address this on our own.

> ¹ Of course, Terminal enables it by default. We likely do need some measure of "compatibility modes" like j4james suggests.

~hot~ warm take: compatibility mode is conhost. If your app truly depends on being able to output SOH and STX into the output buffer, then use conhost, where VT won't be turned on by default. Otherwise, apps in VT mode<sup>2</sup> should do what VT need to do.


<sup>2</sup>: any vt-like mode. I don't think pure VT vs xterm vs _whatever_ mode is going to affect this one in the future.
> I don't think pure VT vs xterm vs _whatever_ mode is going to affect this one in the future.

Well the mode I had in mind that would be affected by this is what is usually referred to as _ANSIBBS_, which is based on the old DOS ANSI.SYS driver with a few extensions. It's what you'd need if you want to access a BBS (they still exist, just via telnet).

You can get by with a pure VT emulator, but a lot of stuff won't look right. And a key requirement is using codepage 437, and having access to the C0 range as printable characters (for things like the card suites). Obviously not going to happen anytime soon, but I'm hoping it might be feasible one day.
@DHowett Just gotta say, I really appreciate the collab with the Pry guys [here](https://github.com/pry/pry/pull/2209)! Thank y'all for helping uncover that this was also an issue in Pry...
When the fix for this issue will be applied?
Whenever someone writes the code 😜 If you'd like to contribute the fix yourself, I can try and help walk you through where I'd start. I'm pretty sure we should be able to just eat them in AdaptDispatch (somewhere)

Fix patch:
diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt
index 6acfcf21708..c1caa85f5db 100644
--- a/.github/actions/spelling/expect/expect.txt
+++ b/.github/actions/spelling/expect/expect.txt
@@ -38,6 +38,7 @@ ansicpg
 ANSISYS
 ANSISYSRC
 ANSISYSSC
+answerback
 antialiasing
 ANull
 anycpu
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index c7856418cca..48d4e277190 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -118,7 +118,7 @@ class ScreenBufferTests
 
     TEST_METHOD(EraseAllTests);
 
-    TEST_METHOD(OutputNULTest);
+    TEST_METHOD(InactiveControlCharactersTest);
 
     TEST_METHOD(VtResize);
     TEST_METHOD(VtResizeComprehensive);
@@ -1014,8 +1014,19 @@ void ScreenBufferTests::EraseAllTests()
         viewport.BottomInclusive()));
 }
 
-void ScreenBufferTests::OutputNULTest()
+void ScreenBufferTests::InactiveControlCharactersTest()
 {
+    // These are the control characters that don't write anything to the
+    // output buffer, and are expected not to move the cursor position.
+
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:ordinal", L"{0, 1, 2, 3, 4, 5, 6, 7, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 28, 29, 30, 31}")
+    END_TEST_METHOD_PROPERTIES()
+
+    unsigned ordinal;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"ordinal", ordinal));
+    const auto ch = static_cast<wchar_t>(ordinal);
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& si = gci.GetActiveOutputBuffer();
     auto& stateMachine = si.GetStateMachine();
@@ -1024,21 +1035,20 @@ void ScreenBufferTests::OutputNULTest()
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);
 
-    Log::Comment(NoThrowString().Format(
-        L"Writing a single NUL"));
-    stateMachine.ProcessString({ L"\0", 1 });
+    Log::Comment(L"Writing a single control character");
+    const auto singleChar = std::wstring(1, ch);
+    stateMachine.ProcessString(singleChar);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);
 
-    Log::Comment(NoThrowString().Format(
-        L"Writing many NULs"));
-    stateMachine.ProcessString({ L"\0\0\0\0\0\0\0\0", 8 });
+    Log::Comment(L"Writing many control characters");
+    const auto manyChars = std::wstring(8, ch);
+    stateMachine.ProcessString(manyChars);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);
 
-    Log::Comment(NoThrowString().Format(
-        L"Testing a single NUL followed by real text"));
-    stateMachine.ProcessString({ L"\0foo", 4 });
+    Log::Comment(L"Testing a single control character followed by real text");
+    stateMachine.ProcessString(singleChar + L"foo");
     VERIFY_ARE_EQUAL(3, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);
 
@@ -1046,9 +1056,8 @@ void ScreenBufferTests::OutputNULTest()
     VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);
 
-    Log::Comment(NoThrowString().Format(
-        L"Writing NULs in between other strings"));
-    stateMachine.ProcessString({ L"\0foo\0bar\0", 9 });
+    Log::Comment(L"Writing controls in between other strings");
+    stateMachine.ProcessString(singleChar + L"foo" + singleChar + L"bar" + singleChar);
     VERIFY_ARE_EQUAL(6, cursor.GetPosition().x);
     VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);
 }
diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp
index 3f191e34198..56eb7b0f091 100644
--- a/src/terminal/parser/OutputStateMachineEngine.cpp
+++ b/src/terminal/parser/OutputStateMachineEngine.cpp
@@ -44,10 +44,10 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
 {
     switch (wch)
     {
-    case AsciiChars::NUL:
-        // microsoft/terminal#1825 - VT applications expect to be able to write NUL
-        // and have _nothing_ happen. Filter the NULs here, so they don't fill the
-        // buffer with empty spaces.
+    case AsciiChars::ENQ:
+        // GH#11946: At some point we may want to add support for the VT
+        // answerback feature, which requires responding to an ENQ control
+        // with a user-defined reply, but until then we just ignore it.
         break;
     case AsciiChars::BEL:
         _dispatch->WarningBell();
@@ -79,9 +79,24 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
     case AsciiChars::SO:
         _dispatch->LockingShift(1);
         break;
-    default:
+    case AsciiChars::SUB:
+        // The SUB control is used to cancel a control sequence in the same
+        // way as CAN, but unlike CAN it also displays an error character,
+        // typically a reverse question mark.
+        _dispatch->Print(L'\u2E2E');
+        break;
+    case AsciiChars::DEL:
+        // The DEL control can sometimes be translated into a printable glyph
+        // if a 96-character set is designated, so we need to pass it through
+        // to the Print method. If not translated, it will be filtered out
+        // there.
         _dispatch->Print(wch);
         break;
+    default:
+        // GH#1825, GH#10786: VT applications expect to be able to write other
+        // control characters and have _nothing_ happen. We filter out these
+        // characters here, so they don't fill the buffer.
+        break;
     }
 
     _ClearLastChar();


Test patch: