Make Console.ReadKey() distinguish between CR and LF inputs (dotnet#3…

…7491) * termios: disable CR/LF mapping flags * fix mapping logic for '\n' characters * only disable cr/lf conversions on Console.ReadKey paths * add comments * address feedback * address feedback * move character conversions to the managed layer * fix issue * add key chord manual tests and doc manual testing * ensure terminal is initialized for Console.OpenStandardInput * Enable ICRNL when reading from UnixConsoleStream * add a manual test for OpenStandardInput * remove CR to LF conversion for Console.ReadLine * rename argument * add CR-to-LF transformation to Console.ReadOrPeek * use ConditionalFact attribute in test * remove dead code
elinor-fung · Jun 25, 2020 · a62bfcb · a62bfcb
1 parent f997ed7
commit a62bfcb
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 26 deletions.
diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs
@@ -13,7 +13,7 @@ internal static partial class Sys
         internal static extern unsafe int ReadStdin(byte* buffer, int bufferSize);
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleBeforeRead")]
-        internal static extern unsafe void InitializeConsoleBeforeRead(byte minChars = 1, byte decisecondsTimeout = 0);
+        internal static extern unsafe void InitializeConsoleBeforeRead(bool convertCrToNl = false, byte minChars = 1, byte decisecondsTimeout = 0);
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_UninitializeConsoleAfterRead")]
         internal static extern unsafe void UninitializeConsoleAfterRead();

diff --git a/src/libraries/Native/Unix/System.Native/pal_console.c b/src/libraries/Native/Unix/System.Native/pal_console.c
@@ -161,7 +161,7 @@ static bool TcSetAttr(struct termios* termios, bool blockIfBackground)
     return rv;
 }
 
-static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minChars, uint8_t decisecondsTimeout, bool blockIfBackground)
+static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minChars, uint8_t decisecondsTimeout, bool blockIfBackground, bool convertCrToNl)
 {
     if (!g_hasTty)
     {
@@ -180,8 +180,13 @@ static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minCha
 
     if (!forChild)
     {
-        termios.c_iflag &= (uint32_t)(~(IXON | IXOFF));
+        termios.c_iflag &= (uint32_t)(~(IXON | IXOFF | ICRNL | INLCR | IGNCR));
         termios.c_lflag &= (uint32_t)(~(ECHO | ICANON | IEXTEN));
+
+        if (convertCrToNl)
+        {
+            termios.c_iflag |= (uint32_t)ICRNL;
+        }
     }
 
     termios.c_cc[VMIN] = minChars;
@@ -222,13 +227,15 @@ void UninitializeTerminal()
     }
 }
 
-void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout)
+void SystemNative_InitializeConsoleBeforeRead(int32_t convertCrToNl, uint8_t minChars, uint8_t decisecondsTimeout)
 {
+    assert(convertCrToNl == 0 || convertCrToNl == 1);
+
     if (pthread_mutex_lock(&g_lock) == 0)
     {
         g_reading = true;
 
-        ConfigureTerminal(g_signalForBreak, /* forChild */ false, minChars, decisecondsTimeout, /* blockIfBackground */ true);
+        ConfigureTerminal(g_signalForBreak, /* forChild */ false, minChars, decisecondsTimeout, /* blockIfBackground */ true, convertCrToNl);
 
         pthread_mutex_unlock(&g_lock);
     }
@@ -263,7 +270,7 @@ void SystemNative_ConfigureTerminalForChildProcess(int32_t childUsesTerminal)
             g_hasCurrentTermios = false;
         }
 
-        ConfigureTerminal(g_signalForBreak, /* forChild */ childUsesTerminal, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ false);
+        ConfigureTerminal(g_signalForBreak, /* forChild */ childUsesTerminal, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ false, /* convertCrToNl */ false);
 
         // Redo "Application mode" when there are no more children using the terminal.
         if (!childUsesTerminal)
@@ -372,7 +379,7 @@ void SystemNative_GetControlCharacters(
 
 int32_t SystemNative_StdinReady()
 {
-    SystemNative_InitializeConsoleBeforeRead(1, 0);
+    SystemNative_InitializeConsoleBeforeRead(1, 0, 0);
     struct pollfd fd = { .fd = STDIN_FILENO, .events = POLLIN };
     int rv = poll(&fd, 1, 0) > 0 ? 1 : 0;
     SystemNative_UninitializeConsoleAfterRead();
@@ -408,7 +415,7 @@ int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak)
 
     if (pthread_mutex_lock(&g_lock) == 0)
     {
-        if (ConfigureTerminal(signalForBreak, /* forChild */ false, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ true))
+        if (ConfigureTerminal(signalForBreak, /* forChild */ false, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ true, /* convertCrToNl */ false))
         {
             g_signalForBreak = signalForBreak;
             rv = 1;

diff --git a/src/libraries/Native/Unix/System.Native/pal_console.h b/src/libraries/Native/Unix/System.Native/pal_console.h
@@ -92,7 +92,7 @@ PALEXPORT int32_t SystemNative_StdinReady(void);
 /**
  * Configures the terminal for System.Console Read.
  */
-PALEXPORT void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout);
+PALEXPORT void SystemNative_InitializeConsoleBeforeRead(int32_t convertCrToNl, uint8_t minChars, uint8_t decisecondsTimeout);
 
 /**
  * Configures the terminal after System.Console Read.

diff --git a/src/libraries/System.Console/src/System/ConsoleKeyInfo.cs b/src/libraries/System.Console/src/System/ConsoleKeyInfo.cs
@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.Diagnostics;
+
 namespace System
 {
     public readonly struct ConsoleKeyInfo

diff --git a/src/libraries/System.Console/src/System/ConsolePal.Unix.cs b/src/libraries/System.Console/src/System/ConsolePal.Unix.cs
@@ -129,15 +129,6 @@ public static ConsoleKeyInfo ReadKey(bool intercept)
             bool previouslyProcessed;
             ConsoleKeyInfo keyInfo = StdInReader.ReadKey(out previouslyProcessed);
 
-            // Replace the '\n' char for Enter by '\r' to match Windows behavior.
-            if (keyInfo.Key == ConsoleKey.Enter && keyInfo.KeyChar == '\n')
-            {
-                bool shift   = (keyInfo.Modifiers & ConsoleModifiers.Shift)   != 0;
-                bool alt     = (keyInfo.Modifiers & ConsoleModifiers.Alt)     != 0;
-                bool control = (keyInfo.Modifiers & ConsoleModifiers.Control) != 0;
-                keyInfo = new ConsoleKeyInfo('\r', keyInfo.Key, shift, alt, control);
-            }
-
             if (!intercept && !previouslyProcessed && keyInfo.KeyChar != '\0')
             {
                 Console.Write(keyInfo.KeyChar);
@@ -1219,11 +1210,19 @@ private void AddKey(TermInfo.Database db, string extendedName, ConsoleKey key, b
         /// <returns>The number of bytes read, or a negative value if there's an error.</returns>
         internal static unsafe int Read(SafeFileHandle fd, byte[] buffer, int offset, int count)
         {
-            fixed (byte* bufPtr = buffer)
+            Interop.Sys.InitializeConsoleBeforeRead(convertCrToNl: true);
+            try
+            {
+                fixed (byte* bufPtr = buffer)
+                {
+                    int result = Interop.CheckIo(Interop.Sys.Read(fd, (byte*)bufPtr + offset, count));
+                    Debug.Assert(result <= count);
+                    return result;
+                }
+            }
+            finally
             {
-                int result = Interop.CheckIo(Interop.Sys.Read(fd, (byte*)bufPtr + offset, count));
-                Debug.Assert(result <= count);
-                return result;
+                Interop.Sys.UninitializeConsoleAfterRead();
             }
         }
 

diff --git a/src/libraries/System.Console/src/System/IO/StdInReader.cs b/src/libraries/System.Console/src/System/IO/StdInReader.cs
@@ -221,9 +221,10 @@ private int ReadOrPeek(bool peek)
             if (_availableKeys.Count > 0)
             {
                 ConsoleKeyInfo keyInfo = peek ? _availableKeys.Peek() : _availableKeys.Pop();
-                if (!IsEol(keyInfo.KeyChar))
+                char keyChar = (keyInfo.KeyChar == '\r') ? '\n' : keyInfo.KeyChar; // Map CR chars to LF
+                if (!IsEol(keyChar))
                 {
-                    return keyInfo.KeyChar;
+                    return keyChar;
                 }
             }
 
@@ -251,7 +252,12 @@ internal ConsoleKey GetKeyFromCharValue(char x, out bool isShift, out bool isCtr
                 case '\t':
                     return ConsoleKey.Tab;
 
+                case '\r':
+                    return ConsoleKey.Enter;
+
                 case '\n':
+                    // Windows compatibility; LF is Ctrl+Enter
+                    isCtrl = true;
                     return ConsoleKey.Enter;
 
                 case (char)(0x1B):
@@ -412,6 +418,7 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed)
                 }
 
                 MapBufferToConsoleKey(out key, out ch, out isShift, out isAlt, out isCtrl);
+
                 return new ConsoleKeyInfo(ch, key, isShift, isAlt, isCtrl);
             }
             finally

diff --git a/src/libraries/System.Console/tests/ManualTests/ManualTests.cs b/src/libraries/System.Console/tests/ManualTests/ManualTests.cs
@@ -38,7 +38,7 @@ public static void ReadLine_BackSpaceCanMoveAccrossWrappedLines()
         [ConditionalFact(nameof(ManualTestsEnabled))]
         public static void InPeek()
         {
-            Console.WriteLine("Please type \"peek\" (without the quotes). You shouldn't see it as you type:");
+            Console.WriteLine("Please type \"peek\" (without the quotes). You should see it as you type:");
             foreach (char c in new[] { 'p', 'e', 'e', 'k' })
             {
                 Assert.Equal(c, Console.In.Peek());
@@ -67,6 +67,48 @@ public static void ReadKey()
             AssertUserExpectedResults("\"console\" correctly not echoed as you typed it");
         }
 
+        [ConditionalTheory(nameof(ManualTestsEnabled))]
+        [InlineData('\x01', ConsoleKey.A, ConsoleModifiers.Control)]
+        [InlineData('\x01', ConsoleKey.A, ConsoleModifiers.Control | ConsoleModifiers.Alt)]
+        [InlineData('\r', ConsoleKey.Enter, (ConsoleModifiers)0)]
+        [InlineData('\n', ConsoleKey.Enter, ConsoleModifiers.Control)]
+        public static void ReadKey_KeyChords(char keyChar, ConsoleKey key, ConsoleModifiers modifiers)
+        {
+            var expected = new ConsoleKeyInfo(keyChar, key, 
+                control: modifiers.HasFlag(ConsoleModifiers.Control),
+                alt: modifiers.HasFlag(ConsoleModifiers.Alt),
+                shift: modifiers.HasFlag(ConsoleModifiers.Shift));
+
+            Console.Write($"Please type key chord {RenderKeyChord(expected)}: ");
+            var actual = Console.ReadKey(intercept: true);
+            Console.WriteLine();
+
+            Assert.Equal(expected.Key, actual.Key);
+            Assert.Equal(expected.Modifiers, actual.Modifiers);
+            Assert.Equal(expected.KeyChar, actual.KeyChar);
+
+            static string RenderKeyChord(ConsoleKeyInfo key)
+            {
+                string modifiers = "";
+                if (key.Modifiers.HasFlag(ConsoleModifiers.Control)) modifiers += "Ctrl+";
+                if (key.Modifiers.HasFlag(ConsoleModifiers.Alt)) modifiers += "Alt+";
+                if (key.Modifiers.HasFlag(ConsoleModifiers.Shift)) modifiers += "Shift+";
+                return modifiers + key.Key;
+            }
+        }
+
+        [ConditionalFact(nameof(ManualTestsEnabled))]
+        public static void OpenStandardInput()
+        {
+            Console.WriteLine("Please type \"console\" (without the quotes). You shouldn't see it as you type:");
+            var stream = Console.OpenStandardInput();
+            var textReader = new System.IO.StreamReader(stream);
+            var result = textReader.ReadLine();
+
+            Assert.Equal("console", result);
+            AssertUserExpectedResults("\"console\" correctly not echoed as you typed it");
+        }
+
         [ConditionalFact(nameof(ManualTestsEnabled))]
         public static void ConsoleOutWriteLine()
         {
@@ -168,8 +210,19 @@ public static void EncodingTest()
         private static void AssertUserExpectedResults(string expected)
         {
             Console.Write($"Did you see {expected}? [y/n] ");
-            Assert.Equal(ConsoleKey.Y, Console.ReadKey().Key);
+            ConsoleKeyInfo info = Console.ReadKey();
             Console.WriteLine();
+
+            switch (info.Key)
+            {
+                case ConsoleKey.Y or ConsoleKey.N:
+                    Assert.Equal(ConsoleKey.Y, info.Key);
+                    break;
+
+                default:
+                    AssertUserExpectedResults(expected);
+                    break;
+            };
         }
     }
 }
diff --git a/src/libraries/System.Console/tests/ManualTests/Readme.md b/src/libraries/System.Console/tests/ManualTests/Readme.md
@@ -0,0 +1,9 @@
+# System.Console manual tests
+
+For verifying console functionality that cannot be run as fully automated.
+To run the suite, follow these steps:
+
+1. Build the CLR and libraries.
+2. Using a terminal, navigate to the current folder.
+3. Enable manual testing by defining the `MANUAL_TESTS` environment variable (e.g. on bash `export MANUAL_TESTS=true`).
+4. Run `dotnet test` and follow the instructions in the command prompt.