Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Add line mappings to project TagHelper attribute values. #244

Merged
merged 1 commit into from
Dec 22, 2014
Merged

Add line mappings to project TagHelper attribute values. #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 16 additions & 3 deletions ...crosoft.AspNet.Razor/Generator/Compiler/CodeBuilder/CSharp/CSharpTagHelperCodeRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Globalization;
using System.Linq;
using Microsoft.AspNet.Razor.TagHelpers;
using Microsoft.AspNet.Razor.Text;

namespace Microsoft.AspNet.Razor.Generator.Compiler.CSharp
{
Expand Down Expand Up @@ -265,7 +266,7 @@ private void RenderBoundHTMLAttributes(IDictionary<string, Chunk> chunkAttribute

// We aren't a bufferable attribute which means we have no Razor code in our value.
// Therefore we can just use the "textValue" as the attribute value.
RenderRawAttributeValue(textValue, attributeDescriptor);
RenderRawAttributeValue(textValue, attributeValueChunk.Start, attributeDescriptor);
}

// End the assignment to the attribute.
Expand Down Expand Up @@ -421,13 +422,25 @@ private void RenderBufferedAttributeValue(TagHelperAttributeDescriptor attribute
});
}

private void RenderRawAttributeValue(string value, TagHelperAttributeDescriptor attributeDescriptor)
private void RenderRawAttributeValue(string value,
SourceLocation documentLocation,
TagHelperAttributeDescriptor attributeDescriptor)
{
RenderAttributeValue(
attributeDescriptor,
valueRenderer: (writer) =>
{
writer.Write(value);
if (_context.Host.DesignTimeMode)
{
using (new CSharpLineMappingWriter(writer, documentLocation, value.Length))
{
writer.Write(value);
}
}
else
{
writer.Write(value);
}
});
}

Expand Down
6 changes: 5 additions & 1 deletion src/Microsoft.AspNet.Razor/Generator/TagHelperCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.AspNet.Razor.Generator.Compiler;
using Microsoft.AspNet.Razor.Parser.SyntaxTree;
using Microsoft.AspNet.Razor.Parser.TagHelpers;
using Microsoft.AspNet.Razor.TagHelpers;
using Microsoft.AspNet.Razor.Text;

namespace Microsoft.AspNet.Razor.Generator
{
Expand Down Expand Up @@ -60,10 +62,12 @@ public override void GenerateStartBlockCode(Block target, CodeGeneratorContext c
attribute.Value.Accept(codeGenerator);

var chunks = codeGenerator.Context.CodeTreeBuilder.CodeTree.Chunks;
var first = chunks.FirstOrDefault();

attributes[attribute.Key] = new ChunkBlock
{
Children = chunks
Children = chunks,
Start = first == null ? SourceLocation.Zero : first.Start
};

// Reset the code tree builder so we can build a new one for the next attribute
Expand Down
35 changes: 31 additions & 4 deletions src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ private static KeyValuePair<string, SyntaxTreeNode> ParseSpan(
Kind = span.Kind
};
var htmlSymbols = span.Symbols.OfType<HtmlSymbol>().ToArray();
var capturedAttributeValueStart = false;
var attributeValueStartLocation = span.Start;
Copy link
Contributor

Choose a reason for hiding this comment

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

where exactly will this point when capturedAttributeValueStart == false and when that's true?
I suspect it'll point to the start of the attribute name in the false case and the @ in the true case. I hope that's not correct...

Copy link
Author

Choose a reason for hiding this comment

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

You were correct, addressed this.

var symbolOffset = 1;
string name = null;

Expand All @@ -175,13 +177,24 @@ private static KeyValuePair<string, SyntaxTreeNode> ParseSpan(
{
var symbol = htmlSymbols[i];

if (name == null && symbol.Type == HtmlSymbolType.Text)
if (afterEquals)
{
name = symbol.Content;
// When symbols are accepted into SpanBuilders, their locations get altered to be offset by the
// parent which is why we need to mark our start location prior to adding the symbol.
// This is needed to know the location of the attribute value start within the document.
if (!capturedAttributeValueStart)
{
capturedAttributeValueStart = true;

attributeValueStartLocation = span.Start + symbol.Start;
}

builder.Accept(symbol);
}
else if (afterEquals)
else if (name == null && symbol.Type == HtmlSymbolType.Text)
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: is there a case where this condition and afterEquals are both true? put another way, did changing the order of these blocks make any difference?

Copy link
Author

Choose a reason for hiding this comment

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

If the code block is <foo ="something" /> then both will be true at some point, but in that case we don't want to capture any name because it'll be invalid (since there's no name)

{
builder.Accept(symbol);
name = symbol.Content;
attributeValueStartLocation = SourceLocation.Advance(span.Start, name);
}
else if (symbol.Type == HtmlSymbolType.Equals)
{
Expand All @@ -193,22 +206,36 @@ private static KeyValuePair<string, SyntaxTreeNode> ParseSpan(
// TODO: Handle malformed tags, if there's an '=' then there MUST be a value.
// https://github.com/aspnet/Razor/issues/104

SourceLocation symbolStartLocation;

// Check for attribute start values, aka single or double quote
if (IsQuote(htmlSymbols[i + 1]))
{
// Move past the attribute start so we can accept the true value.
i++;
symbolStartLocation = htmlSymbols[i + 1].Start;
}
else
{
symbolStartLocation = symbol.Start;

// Set the symbol offset to 0 so we don't attempt to skip an end quote that doesn't exist.
symbolOffset = 0;
}

attributeValueStartLocation = symbolStartLocation +
span.Start +
new SourceLocation(absoluteIndex: 1,
lineIndex: 0,
characterIndex: 1);
afterEquals = true;
}
}

// After all symbols have been added we need to set the builders start position so we do not indirectly
// modify each symbol's Start location.
builder.Start = attributeValueStartLocation;

return CreateMarkupAttribute(name, builder, attributeValueTypes);
}

Expand Down
19 changes: 17 additions & 2 deletions test/Microsoft.AspNet.Razor.Test/Generator/CSharpTagHelperRenderingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,14 @@ public static TheoryData DesignTimeTagHelperTestData
generatedAbsoluteIndex: 475,
generatedLineIndex: 15,
characterOffsetIndex: 14,
contentLength: 11)
contentLength: 11),
BuildLineMapping(documentAbsoluteIndex: 57,
documentLineIndex: 2,
documentCharacterOffsetIndex: 28,
generatedAbsoluteIndex: 927,
generatedLineIndex: 33,
generatedCharacterOffsetIndex: 31,
contentLength: 4)
}
},
{
Expand All @@ -188,7 +195,14 @@ public static TheoryData DesignTimeTagHelperTestData
generatedAbsoluteIndex: 475,
generatedLineIndex: 15,
characterOffsetIndex: 14,
contentLength: 11)
contentLength: 11),
BuildLineMapping(documentAbsoluteIndex: 189,
documentLineIndex: 6,
documentCharacterOffsetIndex: 40,
generatedAbsoluteIndex: 1599,
generatedLineIndex: 44,
generatedCharacterOffsetIndex: 40,
contentLength: 4)
}
},
{
Expand Down Expand Up @@ -218,6 +232,7 @@ public static TheoryData DesignTimeTagHelperTestData
BuildLineMapping(218, 9, 13, 1356, 56, 12, 27),
BuildLineMapping(346, 12, 1754, 68, 0, 48),
BuildLineMapping(440, 15, 46, 2004, 78, 6, 8),
BuildLineMapping(457, 15, 63, 2267, 85, 40, 4),
Copy link
Contributor

Choose a reason for hiding this comment

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

looks relatively skimpy. how about one or two tests of the specific line mapping generation?

Copy link
Author

Choose a reason for hiding this comment

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

What would those tests do? We're already validating the raw attribute value use-case (only one with custom projections) 3x over and the other attribute values in pretty much every test.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • one concern is that attributes don't seem to be used much in the tag helper design-time tests. do the tests include for example <helper html-attribute=@value />, <helper bound-attribute=@value />, <helper html-attribute='@value' />, or similar cases?
  • another concern is handling more complex expressions. for example does Razor handle <helper @checked/> or <helper html-attribute='string@(value)string' /> at this point?

Copy link
Author

Choose a reason for hiding this comment

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

Yup, we cover both of these and more in the ComplexTagHelpers.cshtml / ComplexTagHelpers.DesignTime.cs test bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

BuildLineMapping(501, 16, 31, 2384, 88, 6, 30),
BuildLineMapping(568, 17, 30, 2733, 97, 0, 10),
BuildLineMapping(601, 17, 63, 2815, 103, 0, 8),
Expand Down