Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/Microsoft.AspNet.Razor/Common/HashCodeCombiner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public HashCodeCombiner Add(object o)

public HashCodeCombiner Add<TValue>(TValue value, IEqualityComparer<TValue> comparer)
{
return Add(comparer.GetHashCode(value));
var hashCode = value != null ? comparer.GetHashCode(value) : 0;
return Add(hashCode);
}

public static HashCodeCombiner Start()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,18 +190,18 @@ public CSharpCodeWriter WriteReturn(string value, bool endLine)
/// <returns>The current instance of <see cref="CSharpCodeWriter"/>.</returns>
public CSharpCodeWriter WriteLineNumberDirective(SourceLocation location, string file)

Choose a reason for hiding this comment

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

We talked about the shared hate of this but in the value of less work 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

{
return WriteLineNumberDirective(location.LineIndex + 1, file);
}
if (location.FilePath != null)
{
file = location.FilePath;
}

public CSharpCodeWriter WriteLineNumberDirective(int lineNumber, string file)
{
if (!string.IsNullOrEmpty(LastWrite) &&
!LastWrite.EndsWith(NewLine, StringComparison.Ordinal))
{
WriteLine();
}

var lineNumberAsString = lineNumber.ToString(CultureInfo.InvariantCulture);
var lineNumberAsString = (location.LineIndex + 1).ToString(CultureInfo.InvariantCulture);
return Write("#line ").Write(lineNumberAsString).Write(" \"").Write(file).WriteLine("\"");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ public void RenderDesignTimeExpressionBlockChunk(ExpressionBlockChunk chunk)

var documentLocation = firstChild.Association.Start;
// This is only here to enable accurate formatting by the C# editor.
Writer.WriteLineNumberDirective(documentLocation.LineIndex + 1, Context.SourceFile);
Writer.WriteLineNumberDirective(documentLocation, Context.SourceFile);

// We build the padding with an offset of the design time assignment statement.
Writer.Write(_paddingBuilder.BuildExpressionPadding((Span)firstChild.Association, designTimeAssignment.Length))
Expand Down
16 changes: 16 additions & 0 deletions src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Microsoft.AspNet.Razor/RazorResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -410,4 +410,7 @@ Instead, wrap the contents of the block in "{{}}":
<data name="RewriterError_EmptyTagHelperBoundAttribute" xml:space="preserve">
<value>Attribute '{0}' on tag helper element '{1}' requires a value. Tag helper bound attributes of type '{2}' cannot be empty or contain only whitespace.</value>
</data>
<data name="SourceLocationFilePathDoesNotMatch" xml:space="preserve">
<value>Cannot perform '{1}' operations on '{0}' instances with different file paths.</value>
</data>
</root>
163 changes: 134 additions & 29 deletions src/Microsoft.AspNet.Razor/SourceLocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,65 @@
using System;
using System.Globalization;
using Microsoft.AspNet.Razor.Text;
using Microsoft.Internal.Web.Utils;

namespace Microsoft.AspNet.Razor
{
/// <summary>
/// A location in a Razor file.
/// </summary>
#if NET45
// No Serializable attribute in CoreCLR (no need for it anymore?)
[Serializable]
#endif
public struct SourceLocation : IEquatable<SourceLocation>, IComparable<SourceLocation>
{
public static readonly SourceLocation Undefined = CreateUndefined();
public static readonly SourceLocation Zero = new SourceLocation(0, 0, 0);
/// <summary>
/// An undefined <see cref="SourceLocation"/>.
/// </summary>
public static readonly SourceLocation Undefined =
new SourceLocation(absoluteIndex: -1, lineIndex: -1, characterIndex: -1);

/// <summary>
/// A <see cref="SourceLocation"/> with <see cref="AbsoluteIndex"/>, <see cref="LineIndex"/>, and
/// <see cref="CharacterIndex"/> initialized to 0.
/// </summary>
public static readonly SourceLocation Zero =
new SourceLocation(absoluteIndex: 0, lineIndex: 0, characterIndex: 0);

/// <summary>
/// Initializes a new instance of <see cref="SourceLocation"/>.
/// </summary>
/// <param name="absoluteIndex">The absolute index.</param>
/// <param name="lineIndex">The line index.</param>
/// <param name="characterIndex">The character index.</param>
public SourceLocation(int absoluteIndex, int lineIndex, int characterIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

what scenarios will continue to use this constructor? for example, could it be internal and used only in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this one all over the place - I'm being pretty lazy in this PR and specifically using the other ctor when creating inherited Chunks in Mvc. The existing code continues to use this ctor.

: this(filePath: null, absoluteIndex: absoluteIndex, lineIndex: lineIndex, characterIndex: characterIndex)
{
}

/// <summary>
/// Initializes a new instance of <see cref="SourceLocation"/>.
/// </summary>
/// <param name="filePath">The file path.</param>
/// <param name="absoluteIndex">The absolute index.</param>
/// <param name="lineIndex">The line index.</param>
/// <param name="characterIndex">The character index.</param>
public SourceLocation(string filePath, int absoluteIndex, int lineIndex, int characterIndex)
{
FilePath = filePath;
AbsoluteIndex = absoluteIndex;
LineIndex = lineIndex;
CharacterIndex = characterIndex;
}

/// <summary>
/// Path of the file.
/// </summary>
/// <remarks>When <c>null</c>, the parser assumes the location is in the file currently being processed.
/// </remarks>
public string FilePath { get; set; }

/// <remarks>Set property is only accessible for deserialization purposes.</remarks>
public int AbsoluteIndex { get; set; }

Expand All @@ -35,6 +75,7 @@ public SourceLocation(int absoluteIndex, int lineIndex, int characterIndex)
/// <remarks>Set property is only accessible for deserialization purposes.</remarks>
public int CharacterIndex { get; set; }

/// <inheritdoc />
public override string ToString()
{
return string.Format(
Expand All @@ -45,101 +86,165 @@ public override string ToString()
CharacterIndex);
}

/// <inheritdoc />
public override bool Equals(object obj)
{
return (obj is SourceLocation) && Equals((SourceLocation)obj);
}

/// <inheritdoc />
public override int GetHashCode()
{
// LineIndex and CharacterIndex can be calculated from AbsoluteIndex and the document content.
return AbsoluteIndex;
return HashCodeCombiner.Start()
.Add(FilePath, StringComparer.Ordinal)
.Add(AbsoluteIndex)
.CombinedHash;
}

/// <inheritdoc />
public bool Equals(SourceLocation other)
Copy link
Contributor

Choose a reason for hiding this comment

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

[NotNull]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not on value types.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

{
return
return string.Equals(FilePath, other.FilePath, StringComparison.Ordinal) &&
AbsoluteIndex == other.AbsoluteIndex &&
LineIndex == other.LineIndex &&
CharacterIndex == other.CharacterIndex;
}

/// <inheritdoc />
public int CompareTo(SourceLocation other)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how attached we are to these operators - they are scarcely used in our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also confirmed with Todd that these operators aren't used in tooling.

Copy link
Contributor

Choose a reason for hiding this comment

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

[NotNull]

{
var filePathOrdinal = string.Compare(FilePath, other.FilePath, StringComparison.Ordinal);
if (filePathOrdinal != 0)
{
return filePathOrdinal;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good


return AbsoluteIndex.CompareTo(other.AbsoluteIndex);
}

public static SourceLocation Advance(SourceLocation left, string text)
/// <summary>
/// Advances the <see cref="SourceLocation"/> by the length of the <paramref name="text" />.
/// </summary>
/// <param name="left">The <see cref="SourceLocation"/> to advance.</param>
/// <param name="text">The <see cref="string"/> to advance <paramref name="left"/> by.</param>
/// <returns>The advanced <see cref="SourceLocation"/>.</returns>
public static SourceLocation Advance(SourceLocation left, [NotNull] string text)
{
var tracker = new SourceLocationTracker(left);
tracker.UpdateLocation(text);
return tracker.CurrentLocation;
}

public static SourceLocation Add(SourceLocation left, SourceLocation right)
/// <summary>
/// Adds two <see cref="SourceLocation"/>s.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns>A <see cref="SourceLocation"/> that is the sum of the left and right operands.</returns>
/// <exception cref="ArgumentException">if the <see cref="FilePath"/> of the left and right operands
/// are different.</exception>
Copy link
Contributor

Choose a reason for hiding this comment

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

add , and neither is null

public static SourceLocation operator +(SourceLocation left, SourceLocation right)
Copy link
Contributor

Choose a reason for hiding this comment

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

[NotNull] for both parameters; generally public constructors and methods in this PR need a sweep

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind

{
if (!string.Equals(left.FilePath, right.FilePath, StringComparison.Ordinal) &&
left.FilePath != null &&
right.FilePath != null)
{
// Throw if FilePath for left and right are different, and neither is null.
throw new ArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

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

did you say + and - are rarely used? wondering because (A - B) + C will always throw now. this is why I thought we should allow one FilePath to be null even if the other isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ is used in a couple of places, - isn't used anywhere in our code (we don't know if tooling \ other code outside of Mvc & Razor rely on this). I guess I can change it to support one file path scenarios.

RazorResources.FormatSourceLocationFilePathDoesNotMatch(nameof(SourceLocation), "+"),
nameof(right));
}

var resultFilePath = left.FilePath ?? right.FilePath;
if (right.LineIndex > 0)
{
// Column index doesn't matter
return new SourceLocation(
resultFilePath,
left.AbsoluteIndex + right.AbsoluteIndex,
left.LineIndex + right.LineIndex,
right.CharacterIndex);
}
else
{
return new SourceLocation(
resultFilePath,
left.AbsoluteIndex + right.AbsoluteIndex,
left.LineIndex + right.LineIndex,
left.CharacterIndex + right.CharacterIndex);
}
}

public static SourceLocation Subtract(SourceLocation left, SourceLocation right)
/// <summary>
/// Subtracts two <see cref="SourceLocation"/>s.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns>A <see cref="SourceLocation"/> that is the difference of the left and right operands.</returns>
/// <exception cref="ArgumentException">if the <see cref="FilePath"/> of the left and right operands
/// are different.</exception>
public static SourceLocation operator -(SourceLocation left, SourceLocation right)
{
return new SourceLocation(
left.AbsoluteIndex - right.AbsoluteIndex,
left.LineIndex - right.LineIndex,
left.LineIndex != right.LineIndex ? left.CharacterIndex : left.CharacterIndex - right.CharacterIndex);
}
if (!string.Equals(left.FilePath, right.FilePath, StringComparison.Ordinal))
{
throw new ArgumentException(
RazorResources.FormatSourceLocationFilePathDoesNotMatch(nameof(SourceLocation), "-"),
nameof(right));
}

private static SourceLocation CreateUndefined()
{
var sl = new SourceLocation();
sl.AbsoluteIndex = -1;
sl.LineIndex = -1;
sl.CharacterIndex = -1;
return sl;
var characterIndex = left.LineIndex != right.LineIndex ?
left.CharacterIndex : left.CharacterIndex - right.CharacterIndex;

return new SourceLocation(
filePath: null,
absoluteIndex: left.AbsoluteIndex - right.AbsoluteIndex,
lineIndex: left.LineIndex - right.LineIndex,
characterIndex: characterIndex);
}

/// <summary>
/// Determines whether the first operand is less than the second operand.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns><c>true</c> if <paramref name="left"/> is less than <paramref name="right"/>.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest duplicating the <exception/> documentation from CompareTo() and Equals() in the methods that use them

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind

public static bool operator <(SourceLocation left, SourceLocation right)
{
return left.CompareTo(right) < 0;
}

/// <summary>
/// Determines whether the first operand is greater than the second operand.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns><c>true</c> if <paramref name="left"/> is greater than <paramref name="right"/>.</returns>
public static bool operator >(SourceLocation left, SourceLocation right)
{
return left.CompareTo(right) > 0;
}

/// <summary>
/// Determines whether the operands are equal.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns><c>true</c> if <paramref name="left"/> and <paramref name="right"/> are equal.</returns>
public static bool operator ==(SourceLocation left, SourceLocation right)
{
return left.Equals(right);
}

/// <summary>
/// Determines whether the operands are not equal.
/// </summary>
/// <param name="left">The left operand.</param>
/// <param name="right">The right operand.</param>
/// <returns><c>true</c> if <paramref name="left"/> and <paramref name="right"/> are not equal.</returns>
public static bool operator !=(SourceLocation left, SourceLocation right)
{
return !left.Equals(right);
}

public static SourceLocation operator +(SourceLocation left, SourceLocation right)
{
return Add(left, right);
}

public static SourceLocation operator -(SourceLocation left, SourceLocation right)
{
return Subtract(left, right);
}
}
}
6 changes: 5 additions & 1 deletion src/Microsoft.AspNet.Razor/Text/SourceLocationTracker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ private void UpdateInternalState()

private void RecalculateSourceLocation()
{
_currentLocation = new SourceLocation(_absoluteIndex, _lineIndex, _characterIndex);
_currentLocation = new SourceLocation(
_currentLocation.FilePath,

Choose a reason for hiding this comment

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

Missing test that validates SourceLocationTracker flows the file path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test covering SourceLocation.Advance verifies this.

_absoluteIndex,
_lineIndex,
_characterIndex);
}

public static SourceLocation CalculateNewLocation(SourceLocation lastPosition, string newContent)
Expand Down
Loading