From 65f2f9fc190379d2bbe227a66390059882bf1aeb Mon Sep 17 00:00:00 2001 From: Heejae Chang Date: Fri, 13 Feb 2015 04:34:18 -0800 Subject: [PATCH 1/2] preserve encoding over temporary storage little bit of refactoring over temporary stroage and text factory service to preserve encoding round trip --- .../Workspaces/EditorTextFactoryService.cs | 80 +++---------------- .../Test/Workspaces/TextFactoryTests.cs | 3 +- .../Text/Extensions.SnapshotSourceText.cs | 18 ++--- .../TemporaryStorageServiceFactory.cs | 70 +++++++++++++++- .../TextFactory/DesktopTextFactoryService.cs | 7 ++ .../Host/TextFactory/ITextFactoryService.cs | 10 +++ .../Host/TextFactory/TextFactoryService.cs | 7 ++ .../TemporaryStorageServiceTests.cs | 29 ++----- 8 files changed, 117 insertions(+), 107 deletions(-) diff --git a/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs b/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs index d4ddc5d6114d0..9948ada6223b8 100644 --- a/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs +++ b/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs @@ -1,10 +1,8 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Composition; using System.Diagnostics; using System.IO; -using System.Runtime.InteropServices; using System.Text; using System.Threading; using Microsoft.CodeAnalysis.Host; @@ -62,6 +60,14 @@ public EditorTextFactoryService( } } + public SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) + { + var buffer = CreateTextBuffer(reader, cancellationToken); + + // use the given encoding as it is. + return buffer.CurrentSnapshot.AsRoslynText(encoding); + } + private ITextBuffer CreateTextBuffer(TextReader reader, CancellationToken cancellationToken = default(CancellationToken)) { return _textBufferFactory.CreateTextBuffer(reader, _unknownContentType); @@ -72,80 +78,12 @@ private SourceText CreateTextInternal(Stream stream, Encoding encoding, Cancella cancellationToken.ThrowIfCancellationRequested(); stream.Seek(0, SeekOrigin.Begin); - // Detect text coming from temporary storage - var accessor = stream as ISupportDirectMemoryAccess; - if (accessor != null) - { - return CreateTextFromTemporaryStorage(accessor, (int)stream.Length, cancellationToken); - } - using (var reader = new StreamReader(stream, encoding, detectEncodingFromByteOrderMarks: true, bufferSize: 1024, leaveOpen: true)) { var buffer = CreateTextBuffer(reader, cancellationToken); return buffer.CurrentSnapshot.AsRoslynText(reader.CurrentEncoding ?? Encoding.UTF8); } } - - private unsafe SourceText CreateTextFromTemporaryStorage(ISupportDirectMemoryAccess accessor, int streamLength, CancellationToken cancellationToken) - { - char* src = (char*)accessor.GetPointer(); - Debug.Assert(*src == 0xFEFF); // BOM: Unicode, little endian - // Skip the BOM when creating the reader - using (var reader = new DirectMemoryAccessStreamReader(src + 1, streamLength / sizeof(char) - 1)) - { - var buffer = CreateTextBuffer(reader, cancellationToken); - return buffer.CurrentSnapshot.AsRoslynText(Encoding.Unicode); - } - } - - private unsafe class DirectMemoryAccessStreamReader : TextReader - { - private char* _position; - private readonly char* _end; - - public DirectMemoryAccessStreamReader(char* src, int length) - { - Debug.Assert(src != null); - Debug.Assert(length >= 0); - _position = src; - _end = _position + length; - } - - public override int Read() - { - if(_position >= _end) - { - return -1; - } - - return *_position++; - } - - public override int Read(char[] buffer, int index, int count) - { - if (buffer == null) - { - throw new ArgumentNullException(nameof(buffer)); - } - - if (index < 0 || index >= buffer.Length) - { - throw new ArgumentOutOfRangeException(nameof(index)); - } - - if (count < 0 || (index + count) > buffer.Length) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } - - count = Math.Min(count, (int)(_end - _position)); - if (count > 0) - { - Marshal.Copy((IntPtr)_position, buffer, index, count); - _position += count; - } - return count; - } - } } } + diff --git a/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs b/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs index af6b9c5b7d8a8..74ebe44a8aeb8 100644 --- a/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs +++ b/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs @@ -64,7 +64,6 @@ public void TestCreateFromTemporaryStorage() // Create a temporary storage location using (var temporaryStorage = temporaryStorageService.CreateTemporaryTextStorage(System.Threading.CancellationToken.None)) { - // Write text into it temporaryStorage.WriteTextAsync(text).Wait(); @@ -73,7 +72,7 @@ public void TestCreateFromTemporaryStorage() Assert.NotSame(text, text2); Assert.Equal(text.ToString(), text2.ToString()); - Assert.Equal(text2.Encoding, Encoding.Unicode); + Assert.Equal(text2.Encoding, null); } } diff --git a/src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs b/src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs index 93515ed37c67c..1fb9efe6f2adf 100644 --- a/src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs +++ b/src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs @@ -28,8 +28,8 @@ private class SnapshotSourceText : SourceText /// internal class ClosedSnapshotSourceText : SnapshotSourceText { - public ClosedSnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encoding) - : base(roslynSnapshot, encoding, containerOpt: null) + public ClosedSnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encodingOpt) + : base(roslynSnapshot, encodingOpt, containerOpt: null) { } } @@ -40,29 +40,27 @@ public ClosedSnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encoding) /// The ITextSnapshot backing the SourceText instance /// protected readonly ITextSnapshot RoslynSnapshot; - private readonly Encoding _encoding; + private readonly Encoding _encodingOpt; private readonly TextBufferContainer _containerOpt; private readonly int _reiteratedVersion; private LineInfo _lineInfo; - private SnapshotSourceText(ITextSnapshot editorSnapshot, Encoding encoding) + private SnapshotSourceText(ITextSnapshot editorSnapshot, Encoding encodingOpt) { Contract.ThrowIfNull(editorSnapshot); - Contract.ThrowIfNull(encoding); this.RoslynSnapshot = TextBufferMapper.ToRoslyn(editorSnapshot); _containerOpt = TextBufferContainer.From(editorSnapshot.TextBuffer); _reiteratedVersion = editorSnapshot.Version.ReiteratedVersionNumber; - _encoding = encoding; + _encodingOpt = encodingOpt; } - public SnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encoding, TextBufferContainer containerOpt) + public SnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encodingOpt, TextBufferContainer containerOpt) { Contract.ThrowIfNull(roslynSnapshot); - Contract.ThrowIfNull(encoding); this.RoslynSnapshot = roslynSnapshot; - _encoding = encoding; + _encodingOpt = encodingOpt; _containerOpt = containerOpt; } @@ -101,7 +99,7 @@ private static SnapshotSourceText CreateText(ITextSnapshot editorSnapshot) public override Encoding Encoding { - get { return _encoding; } + get { return _encodingOpt; } } public ITextSnapshot EditorSnapshot diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs index cfec88994bf77..f06ad656913ef 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs @@ -2,7 +2,9 @@ using System; using System.Composition; +using System.Diagnostics; using System.IO; +using System.Runtime.InteropServices; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -83,8 +85,10 @@ public SourceText ReadText(CancellationToken cancellationToken) using (Logger.LogBlock(FunctionId.TemporaryStorageServiceFactory_ReadText, cancellationToken)) { using (var stream = _memoryMappedInfo.CreateReadableStream()) + using (var reader = CreateTextReaderFromTemporaryStorage((ISupportDirectMemoryAccess)stream, (int)stream.Length, cancellationToken)) { - return _service._textFactory.CreateText(stream, _encoding, cancellationToken); + // we pass in encoding we got from original source text even if it is null. + return _service._textFactory.CreateText(reader, _encoding, cancellationToken); } } } @@ -135,6 +139,69 @@ public void WriteText(SourceText text, CancellationToken cancellationToken) // See commentary in ReadTextAsync for why this is implemented this way. return Task.Factory.StartNew(() => WriteText(text, cancellationToken), cancellationToken, TaskCreationOptions.None, TaskScheduler.Default); } + + private unsafe TextReader CreateTextReaderFromTemporaryStorage(ISupportDirectMemoryAccess accessor, int streamLength, CancellationToken cancellationToken) + { + char* src = (char*)accessor.GetPointer(); + + // BOM: Unicode, little endian + // Skip the BOM when creating the reader + Debug.Assert(*src == 0xFEFF); + + return new DirectMemoryAccessStreamReader(src + 1, streamLength / sizeof(char) - 1); + } + + private unsafe class DirectMemoryAccessStreamReader : TextReader + { + private char* _position; + private readonly char* _end; + + public DirectMemoryAccessStreamReader(char* src, int length) + { + Debug.Assert(src != null); + Debug.Assert(length >= 0); + + _position = src; + _end = _position + length; + } + + public override int Read() + { + if (_position >= _end) + { + return -1; + } + + return *_position++; + } + + public override int Read(char[] buffer, int index, int count) + { + if (buffer == null) + { + throw new ArgumentNullException(nameof(buffer)); + } + + if (index < 0 || index >= buffer.Length) + { + throw new ArgumentOutOfRangeException(nameof(index)); + } + + if (count < 0 || (index + count) > buffer.Length) + { + throw new ArgumentOutOfRangeException(nameof(count)); + } + + count = Math.Min(count, (int)(_end - _position)); + if (count > 0) + { + Marshal.Copy((IntPtr)_position, buffer, index, count); + _position += count; + } + + return count; + } + } } private class TemporaryStreamStorage : ITemporaryStreamStorage @@ -244,3 +311,4 @@ private async Task WriteStreamMaybeAsync(Stream stream, bool useAsync, Cancellat } } } + diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TextFactory/DesktopTextFactoryService.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TextFactory/DesktopTextFactoryService.cs index bc157beba3b25..ad81a858cd7e7 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TextFactory/DesktopTextFactoryService.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TextFactory/DesktopTextFactoryService.cs @@ -17,5 +17,12 @@ internal class DesktopTextFactoryService : ITextFactoryService cancellationToken.ThrowIfCancellationRequested(); return EncodedStringText.Create(stream, defaultEncoding); } + + public SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) + { + cancellationToken.ThrowIfCancellationRequested(); + return SourceText.From(reader.ReadToEnd(), encoding); + } } } + diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs index 54f8e246fc026..01c242425559d 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs @@ -28,5 +28,15 @@ internal interface ITextFactoryService : IWorkspaceService /// /// An IO error occurred while reading from the stream. SourceText CreateText(Stream stream, Encoding defaultEncoding, CancellationToken cancellationToken = default(CancellationToken)); + + + /// + /// Creates from a reader. + /// + /// The to read the text from. + /// Specifies an encoding for the SourceText. it could be even null + /// Cancellation token. + SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)); } } + diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/TextFactoryService.cs b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/TextFactoryService.cs index 3da9f9a33043f..c071ae1d98399 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/TextFactoryService.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/TextFactoryService.cs @@ -17,5 +17,12 @@ internal class TextFactoryService : ITextFactoryService cancellationToken.ThrowIfCancellationRequested(); return SourceText.From(stream, defaultEncoding); } + + public SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) + { + cancellationToken.ThrowIfCancellationRequested(); + return SourceText.From(reader.ReadToEnd(), encoding); + } } } + diff --git a/src/Workspaces/CoreTest/WorkspaceServiceTests/TemporaryStorageServiceTests.cs b/src/Workspaces/CoreTest/WorkspaceServiceTests/TemporaryStorageServiceTests.cs index 5797aef183fd8..500be12c29c35 100644 --- a/src/Workspaces/CoreTest/WorkspaceServiceTests/TemporaryStorageServiceTests.cs +++ b/src/Workspaces/CoreTest/WorkspaceServiceTests/TemporaryStorageServiceTests.cs @@ -18,7 +18,7 @@ namespace Microsoft.CodeAnalysis.UnitTests { public class TemporaryStorageServiceTests { - [Fact(Skip = "https://github.com/dotnet/roslyn/issues/380"), Trait(Traits.Feature, Traits.Features.Workspace)] + [Fact, Trait(Traits.Feature, Traits.Features.Workspace)] public void TestTemporaryStorageText() { var textFactory = new TextFactoryService(); @@ -79,6 +79,7 @@ private void TestTemporaryStorage(ITemporaryStorageService temporaryStorageServi Assert.NotSame(text, text2); Assert.Equal(text.ToString(), text2.ToString()); + Assert.Equal(text.Encoding, text2.Encoding); temporaryStorage.Dispose(); } @@ -325,7 +326,7 @@ public void StreamTest3() } } - [Fact(Skip = "https://github.com/dotnet/roslyn/issues/353"), Trait(Traits.Feature, Traits.Features.Workspace)] + [Fact, Trait(Traits.Feature, Traits.Features.Workspace)] public void TestTemporaryStorageTextEncoding() { var textFactory = new TextFactoryService(); @@ -333,33 +334,15 @@ public void TestTemporaryStorageTextEncoding() // test normal string var text = SourceText.From(new string(' ', 4096) + "public class A {}", Encoding.ASCII); - TestTemporaryStorageWithEncoding(service, text); + TestTemporaryStorage(service, text); // test empty string text = SourceText.From(string.Empty); - TestTemporaryStorageWithEncoding(service, text); + TestTemporaryStorage(service, text); // test large string text = SourceText.From(new string(' ', 1024 * 1024) + "public class A {}"); - TestTemporaryStorageWithEncoding(service, text); - } - - private void TestTemporaryStorageWithEncoding(ITemporaryStorageService temporaryStorageService, SourceText text) - { - // create a temporary storage location - var temporaryStorage = temporaryStorageService.CreateTemporaryTextStorage(System.Threading.CancellationToken.None); - - // write text into it - temporaryStorage.WriteTextAsync(text).Wait(); - - // read text back from it - var text2 = temporaryStorage.ReadTextAsync().Result; - - Assert.NotSame(text, text2); - Assert.Equal(text.ToString(), text2.ToString()); - Assert.Equal(text.Encoding, text2.Encoding); - - temporaryStorage.Dispose(); + TestTemporaryStorage(service, text); } } } From ab47cbdd63611fb3fd24fd224c2b9c998ebde6b4 Mon Sep 17 00:00:00 2001 From: Heejae Chang Date: Fri, 13 Feb 2015 17:39:25 -0800 Subject: [PATCH 2/2] PR.Feedback some code clean up based on PR feedback --- .../Workspaces/EditorTextFactoryService.cs | 3 +++ .../Test/Workspaces/TextFactoryTests.cs | 23 +++++++++++++++++++ .../Host/TextFactory/ITextFactoryService.cs | 5 ++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs b/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs index 9948ada6223b8..7814b8dc4f0a7 100644 --- a/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs +++ b/src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs @@ -32,6 +32,8 @@ public EditorTextFactoryService( public SourceText CreateText(Stream stream, Encoding defaultEncoding, CancellationToken cancellationToken = default(CancellationToken)) { + // this API is for a case where user wants us to figure out encoding from the given stream. + // if defaultEncoding is given, we will use it if we couldn't figure out encoding used in the stream ourselves. Debug.Assert(stream != null); Debug.Assert(stream.CanSeek); Debug.Assert(stream.CanRead); @@ -62,6 +64,7 @@ public EditorTextFactoryService( public SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { + // this API is for a case where user just wants to create a source text with explicit encoding. var buffer = CreateTextBuffer(reader, cancellationToken); // use the given encoding as it is. diff --git a/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs b/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs index 74ebe44a8aeb8..5a32f88e7ffbe 100644 --- a/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs +++ b/src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs @@ -76,6 +76,29 @@ public void TestCreateFromTemporaryStorage() } } + [Fact] + public void TestCreateFromTemporaryStorageWithEncoding() + { + var textFactory = CreateMockTextFactoryService(); + var temporaryStorageService = new TemporaryStorageServiceFactory.TemporaryStorageService(textFactory); + + var text = Text.SourceText.From("Hello, World!", Encoding.ASCII); + + // Create a temporary storage location + using (var temporaryStorage = temporaryStorageService.CreateTemporaryTextStorage(System.Threading.CancellationToken.None)) + { + // Write text into it + temporaryStorage.WriteTextAsync(text).Wait(); + + // Read text back from it + var text2 = temporaryStorage.ReadTextAsync().Result; + + Assert.NotSame(text, text2); + Assert.Equal(text.ToString(), text2.ToString()); + Assert.Equal(text2.Encoding, Encoding.ASCII); + } + } + private EditorTextFactoryService CreateMockTextFactoryService() { var mockTextBufferFactoryService = new Mock(); diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs index 01c242425559d..613be5c416395 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs @@ -31,10 +31,11 @@ internal interface ITextFactoryService : IWorkspaceService /// - /// Creates from a reader. + /// Creates from a reader with given . /// /// The to read the text from. - /// Specifies an encoding for the SourceText. it could be even null + /// Specifies an encoding for the SourceText. + /// it could be null. but if null is given, it won't be able to calculate checksum /// Cancellation token. SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)); }