Skip to content

preserve encoding over temporary storage #482

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.

Sign up for GitHub

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

Merged
heejaechang merged 4 commits into from
Feb 14, 2015
Merged

preserve encoding over temporary storage #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65f2f9f
preserve encoding over temporary storage
heejaechang Feb 13, 2015
ab47cbd
PR.Feedback
heejaechang Feb 14, 2015
e7719dc
Merge branch 'master' of https://github.com/dotnet/roslyn
heejaechang Feb 14, 2015
23a1004
Merge branch 'master' of https://github.com/dotnet/roslyn
heejaechang Feb 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 12 additions & 71 deletions src/EditorFeatures/Core/Implementation/Workspaces/EditorTextFactoryService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -34,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);
Expand Down Expand Up @@ -62,6 +62,15 @@ 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.
return buffer.CurrentSnapshot.AsRoslynText(encoding);
}

private ITextBuffer CreateTextBuffer(TextReader reader, CancellationToken cancellationToken = default(CancellationToken))
{
return _textBufferFactory.CreateTextBuffer(reader, _unknownContentType);
Expand All @@ -72,80 +81,12 @@ private SourceText CreateTextInternal(Stream stream, Encoding encoding, Cancella
cancellationToken.ThrowIfCancellationRequested();

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you comment this function? I find it confusing that an Encoding is passed in, but then the code calls "detectEncodingFromBytOrderMarks" and then calls AsRoslynText, not using the encoding passed in, but instead reader.CurrentEncoding or Encoding.UTF8.

It seems like someone can be explicit about the encoding, but still have that overridden by the system.

@heejaechang heejaechang Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

default encoding is something it use if it can't figure out Encoding from stream. that is why it is called default encoding. I will add more comments.

But I agree it is confusing. I took me sometime to figure out what that actually mean.

@heejaechang heejaechang Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New API I added. CreateText(TextReader reader ... ) is the API people should use if they want to set encoding explicitly. this API doesnt have ambiguity where given Encoding and encoding embedded in the stream is different.

the first API is intended to be used in a situation where you just open a file (FileStream) and want to create a SourceText. the default encoding given is basically saying, if you can't figure out Encoding from the stream I gave you, assume it is the default encoding.

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;
}
}
}
}

24 changes: 23 additions & 1 deletion src/EditorFeatures/Test/Workspaces/TextFactoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,29 @@ 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();

// Read text back from it
var text2 = temporaryStorage.ReadTextAsync().Result;

Assert.NotSame(text, text2);
Assert.Equal(text.ToString(), text2.ToString());
Assert.Equal(text2.Encoding, null);

@pharring pharring Feb 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add another test (or enhance this one) to test non-null encodings too.

foreach(var encoding in new Encoding[] {
    null,
    Encoding.ASCII,
    Encoding.UTF8,
    Encoding.Unicode,
    Encoding.BigEndianUnicode})
{
    var text = Text.SourceText.From("Hello, World!", encoding);
    // etc.

@tmeschter tmeschter Feb 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please do that.

@heejaechang heejaechang Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}
}

[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();

Expand All @@ -73,7 +95,7 @@ public void TestCreateFromTemporaryStorage()

Assert.NotSame(text, text2);
Assert.Equal(text.ToString(), text2.ToString());
Assert.Equal(text2.Encoding, Encoding.Unicode);
Assert.Equal(text2.Encoding, Encoding.ASCII);
}
}

Expand Down
18 changes: 8 additions & 10 deletions src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ private class SnapshotSourceText : SourceText
/// </summary>
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)
{
}
}
Expand All @@ -40,29 +40,27 @@ public ClosedSnapshotSourceText(ITextSnapshot roslynSnapshot, Encoding encoding)
/// The ITextSnapshot backing the SourceText instance
/// </summary>
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;
}

Expand Down Expand Up @@ -101,7 +99,7 @@ private static SnapshotSourceText CreateText(ITextSnapshot editorSnapshot)

public override Encoding Encoding
{
get { return _encoding; }
get { return _encodingOpt; }
}

public ITextSnapshot EditorSnapshot
Expand Down
70 changes: 69 additions & 1 deletion ...Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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);

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How are you certain that this will be true?

@heejaechang heejaechang Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because we are the one who write in and read it and no-one else has access to it.


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
Expand Down Expand Up @@ -244,3 +311,4 @@ private async Task WriteStreamMaybeAsync(Stream stream, bool useAsync, Cancellat
}
}
}

7 changes: 7 additions & 0 deletions src/Workspaces/Core/Desktop/Workspace/Host/TextFactory/DesktopTextFactoryService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

11 changes: 11 additions & 0 deletions src/Workspaces/Core/Portable/Workspace/Host/TextFactory/ITextFactoryService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,16 @@ internal interface ITextFactoryService : IWorkspaceService
/// </exception>
/// <exception cref="IOException">An IO error occurred while reading from the stream.</exception>
SourceText CreateText(Stream stream, Encoding defaultEncoding, CancellationToken cancellationToken = default(CancellationToken));


/// <summary>
/// Creates <see cref="SourceText"/> from a reader with given <paramref name="encoding"/>.
/// </summary>
/// <param name="reader">The <see cref="TextReader"/> to read the text from.</param>
/// <param name="encoding">Specifies an encoding for the <see cref="SourceText"/>SourceText.
/// it could be null. but if null is given, it won't be able to calculate checksum</param>
/// <param name="cancellationToken">Cancellation token.</param>
SourceText CreateText(TextReader reader, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken));
}
}

7 changes: 7 additions & 0 deletions src/Workspaces/Core/Portable/Workspace/Host/TextFactory/TextFactoryService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

@jaredpar jaredpar Feb 13, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need this token? Is it just coming as a part of an interface contract?

@mattwar mattwar Feb 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The text reader could represent a large file that get's read in a loop. We need ability to quickly abort work.

@jaredpar jaredpar Feb 13, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mattwar but that token isn't cancelling the read, it just cancels before we start the read. Unless I'm missing the calling context.

@heejaechang heejaechang Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ya, but it is interface, there could be an implementer that read through chunk of string and check cancellation.

return SourceText.From(reader.ReadToEnd(), encoding);
}
}
}

Loading