Skip to content
This repository has been archived by the owner on Sep 26, 2024. It is now read-only.

Fixes creating constraint string from type parameter symbol #44

Merged
merged 2 commits into from
Jan 25, 2022
Merged

Fixes creating constraint string from type parameter symbol #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7030b36
Fixes creating constraint string from type parameter symbol
javiercn Jan 24, 2022
64c9f3d
Fixes additional scenarios with generic type constraints
javiercn Jan 24, 2022
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
52 changes: 52 additions & 0 deletions ...rosoft.AspNetCore.Razor.Language/test/IntegrationTests/ComponentCodeGenerationTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3893,6 +3893,58 @@ public class Column<TItem> : ComponentBase
CompileToAssembly(generated);
}

[Fact]
public void CascadingGenericInference_Inferred_WithConstraints()
{
// Arrange
AdditionalSyntaxTrees.Add(Parse(@"
using Microsoft.AspNetCore.Components;

namespace Test
{
[CascadingTypeParameter(nameof(TItem))]
public class Grid<TItem> : ComponentBase
{
[Parameter] public RenderFragment ColumnsTemplate { get; set; }
}

public abstract partial class BaseColumn<TItem> : ComponentBase where TItem : class
{
[CascadingParameter]
internal Grid<TItem> Grid { get; set; }
}

public class Column<TItem> : BaseColumn<TItem>, IGridFieldColumn<TItem> where TItem : class
{
[Parameter]
public string FieldName { get; set; }
}

internal interface IGridFieldColumn<TItem> where TItem : class
{
}

public class WeatherForecast { }
}
"));

// Act
var generated = CompileToCSharp(@"
<Grid
TItem=""WeatherForecast""
Items=""@(Array.Empty<WeatherForecast>())"">
<ColumnsTemplate>
<Column Title=""Date"" FieldName=""Date"" Format=""d"" Width=""10rem"" />
</ColumnsTemplate>
</Grid>
");

// Assert
AssertDocumentNodeMatchesBaseline(generated.CodeDocument);
AssertCSharpDocumentMatchesBaseline(generated.CodeDocument);
CompileToAssembly(generated);
}

[Fact]
public void CascadingGenericInference_Partial_CreatesError()
{
Expand Down
80 changes: 80 additions & 0 deletions ...enerationTest/CascadingGenericInference_Inferred_WithConstraints/TestComponent.codegen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// <auto-generated/>
#pragma warning disable 1591
namespace Test
{
#line hidden
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components;
public partial class TestComponent : global::Microsoft.AspNetCore.Components.ComponentBase
{
#pragma warning disable 219
private void __RazorDirectiveTokenHelpers__() {
}
#pragma warning restore 219
#pragma warning disable 0414
private static System.Object __o = null;
#pragma warning restore 0414
#pragma warning disable 1998
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder)
{
__o = typeof(
#nullable restore
#line 2 "x:\dir\subdir\Test\TestComponent.cshtml"
WeatherForecast

#line default
#line hidden
#nullable disable
);
__o =
#nullable restore
#line 3 "x:\dir\subdir\Test\TestComponent.cshtml"
Array.Empty<WeatherForecast>()

#line default
#line hidden
#nullable disable
;
__builder.AddAttribute(-1, "ColumnsTemplate", (global::Microsoft.AspNetCore.Components.RenderFragment)((__builder2) => {
global::__Blazor.Test.TestComponent.TypeInference.CreateColumn_0(__builder2, -1, default(WeatherForecast), -1, "", -1, "", -1, "", -1, "");
#nullable restore
#line 5 "x:\dir\subdir\Test\TestComponent.cshtml"
__o = typeof(global::Test.Column<>);

#line default
#line hidden
#nullable disable
}
));
#nullable restore
#line 1 "x:\dir\subdir\Test\TestComponent.cshtml"
__o = typeof(global::Test.Grid<>);

#line default
#line hidden
#nullable disable
}
#pragma warning restore 1998
}
}
namespace __Blazor.Test.TestComponent
{
#line hidden
internal static class TypeInference
{
public static void CreateColumn_0<TItem>(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder, int seq, TItem __syntheticArg0, int __seq0, global::System.Object __arg0, int __seq1, global::System.String __arg1, int __seq2, global::System.Object __arg2, int __seq3, global::System.Object __arg3)
where TItem : class
{
__builder.OpenComponent<global::Test.Column<TItem>>(seq);
__builder.AddAttribute(__seq0, "Title", __arg0);
__builder.AddAttribute(__seq1, "FieldName", __arg1);
__builder.AddAttribute(__seq2, "Format", __arg2);
__builder.AddAttribute(__seq3, "Width", __arg3);
__builder.CloseComponent();
}
}
}
#pragma warning restore 1591
45 changes: 45 additions & 0 deletions ...odeGenerationTest/CascadingGenericInference_Inferred_WithConstraints/TestComponent.ir.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
Document -
NamespaceDeclaration - - Test
UsingDirective - (3:1,1 [12] ) - System
UsingDirective - (18:2,1 [32] ) - System.Collections.Generic
UsingDirective - (53:3,1 [17] ) - System.Linq
UsingDirective - (73:4,1 [28] ) - System.Threading.Tasks
UsingDirective - (104:5,1 [37] ) - Microsoft.AspNetCore.Components
ClassDeclaration - - public partial - TestComponent - global::Microsoft.AspNetCore.Components.ComponentBase -
DesignTimeDirective -
CSharpCode -
IntermediateToken - - CSharp - #pragma warning disable 0414
CSharpCode -
IntermediateToken - - CSharp - private static System.Object __o = null;
CSharpCode -
IntermediateToken - - CSharp - #pragma warning restore 0414
MethodDeclaration - - protected override - void - BuildRenderTree
Component - (0:0,0 [205] x:\dir\subdir\Test\TestComponent.cshtml) - Grid
ComponentChildContent - (80:3,4 [116] x:\dir\subdir\Test\TestComponent.cshtml) - ColumnsTemplate - context
HtmlContent - (97:3,21 [10] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (97:3,21 [10] x:\dir\subdir\Test\TestComponent.cshtml) - Html - \n
Component - (107:4,8 [65] x:\dir\subdir\Test\TestComponent.cshtml) - Column
ComponentAttribute - - Title - - AttributeStructure.DoubleQuotes
HtmlContent - (122:4,23 [4] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (122:4,23 [4] x:\dir\subdir\Test\TestComponent.cshtml) - Html - Date
ComponentAttribute - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml) - FieldName - FieldName - AttributeStructure.DoubleQuotes
HtmlContent - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml) - Html - Date
ComponentAttribute - - Format - - AttributeStructure.DoubleQuotes
HtmlContent - (153:4,54 [1] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (153:4,54 [1] x:\dir\subdir\Test\TestComponent.cshtml) - Html - d
ComponentAttribute - - Width - - AttributeStructure.DoubleQuotes
HtmlContent - (163:4,64 [5] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (163:4,64 [5] x:\dir\subdir\Test\TestComponent.cshtml) - Html - 10rem
HtmlContent - (172:4,73 [6] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (172:4,73 [6] x:\dir\subdir\Test\TestComponent.cshtml) - Html - \n
ComponentTypeArgument - (14:1,7 [15] x:\dir\subdir\Test\TestComponent.cshtml) - TItem
LazyIntermediateToken - (14:1,7 [15] x:\dir\subdir\Test\TestComponent.cshtml) - CSharp - WeatherForecast
ComponentAttribute - - Items - - AttributeStructure.DoubleQuotes
CSharpExpression - (39:2,7 [33] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (41:2,9 [30] x:\dir\subdir\Test\TestComponent.cshtml) - CSharp - Array.Empty<WeatherForecast>()
HtmlContent - (205:6,7 [2] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (205:6,7 [2] x:\dir\subdir\Test\TestComponent.cshtml) - Html - \n
NamespaceDeclaration - - __Blazor.Test.TestComponent
ClassDeclaration - - internal static - TypeInference - -
ComponentTypeInferenceMethod - - __Blazor.Test.TestComponent.TypeInference - CreateColumn_0
10 changes: 10 additions & 0 deletions ...erationTest/CascadingGenericInference_Inferred_WithConstraints/TestComponent.mappings.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Source Location: (14:1,7 [15] x:\dir\subdir\Test\TestComponent.cshtml)
|WeatherForecast|
Generated Location: (902:25,7 [15] )
|WeatherForecast|

Source Location: (41:2,9 [30] x:\dir\subdir\Test\TestComponent.cshtml)
|Array.Empty<WeatherForecast>()|
Generated Location: (1084:34,9 [30] )
|Array.Empty<WeatherForecast>()|

52 changes: 52 additions & 0 deletions ...enerationTest/CascadingGenericInference_Inferred_WithConstraints/TestComponent.codegen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// <auto-generated/>
#pragma warning disable 1591
namespace Test
{
#line hidden
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components;
public partial class TestComponent : global::Microsoft.AspNetCore.Components.ComponentBase
{
#pragma warning disable 1998
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder)
{
__builder.OpenComponent<global::Test.Grid<WeatherForecast>>(0);
__builder.AddAttribute(1, "Items",
#nullable restore
#line 3 "x:\dir\subdir\Test\TestComponent.cshtml"
Array.Empty<WeatherForecast>()

#line default
#line hidden
#nullable disable
);
__builder.AddAttribute(2, "ColumnsTemplate", (global::Microsoft.AspNetCore.Components.RenderFragment)((__builder2) => {
global::__Blazor.Test.TestComponent.TypeInference.CreateColumn_0(__builder2, 3, default(WeatherForecast), 4, "Date", 5, "Date", 6, "d", 7, "10rem");
}
));
__builder.CloseComponent();
}
#pragma warning restore 1998
}
}
namespace __Blazor.Test.TestComponent
{
#line hidden
internal static class TypeInference
{
public static void CreateColumn_0<TItem>(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder, int seq, TItem __syntheticArg0, int __seq0, global::System.Object __arg0, int __seq1, global::System.String __arg1, int __seq2, global::System.Object __arg2, int __seq3, global::System.Object __arg3)
where TItem : class
{
__builder.OpenComponent<global::Test.Column<TItem>>(seq);
__builder.AddAttribute(__seq0, "Title", __arg0);
__builder.AddAttribute(__seq1, "FieldName", __arg1);
__builder.AddAttribute(__seq2, "Format", __arg2);
__builder.AddAttribute(__seq3, "Width", __arg3);
__builder.CloseComponent();
}
}
}
#pragma warning restore 1591
32 changes: 32 additions & 0 deletions ...odeGenerationTest/CascadingGenericInference_Inferred_WithConstraints/TestComponent.ir.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
Document -
NamespaceDeclaration - - Test
UsingDirective - (3:1,1 [14] ) - System
UsingDirective - (18:2,1 [34] ) - System.Collections.Generic
UsingDirective - (53:3,1 [19] ) - System.Linq
UsingDirective - (73:4,1 [30] ) - System.Threading.Tasks
UsingDirective - (104:5,1 [39] ) - Microsoft.AspNetCore.Components
ClassDeclaration - - public partial - TestComponent - global::Microsoft.AspNetCore.Components.ComponentBase -
MethodDeclaration - - protected override - void - BuildRenderTree
Component - (0:0,0 [205] x:\dir\subdir\Test\TestComponent.cshtml) - Grid
ComponentChildContent - (80:3,4 [116] x:\dir\subdir\Test\TestComponent.cshtml) - ColumnsTemplate - context
Component - (107:4,8 [65] x:\dir\subdir\Test\TestComponent.cshtml) - Column
ComponentAttribute - - Title - - AttributeStructure.DoubleQuotes
HtmlContent - (122:4,23 [4] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (122:4,23 [4] x:\dir\subdir\Test\TestComponent.cshtml) - Html - Date
ComponentAttribute - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml) - FieldName - FieldName - AttributeStructure.DoubleQuotes
HtmlContent - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (139:4,40 [4] x:\dir\subdir\Test\TestComponent.cshtml) - Html - Date
ComponentAttribute - - Format - - AttributeStructure.DoubleQuotes
HtmlContent - (153:4,54 [1] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (153:4,54 [1] x:\dir\subdir\Test\TestComponent.cshtml) - Html - d
ComponentAttribute - - Width - - AttributeStructure.DoubleQuotes
HtmlContent - (163:4,64 [5] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (163:4,64 [5] x:\dir\subdir\Test\TestComponent.cshtml) - Html - 10rem
ComponentTypeArgument - (14:1,7 [15] x:\dir\subdir\Test\TestComponent.cshtml) - TItem
LazyIntermediateToken - (14:1,7 [15] x:\dir\subdir\Test\TestComponent.cshtml) - CSharp - WeatherForecast
ComponentAttribute - - Items - - AttributeStructure.DoubleQuotes
CSharpExpression - (39:2,7 [33] x:\dir\subdir\Test\TestComponent.cshtml)
LazyIntermediateToken - (41:2,9 [30] x:\dir\subdir\Test\TestComponent.cshtml) - CSharp - Array.Empty<WeatherForecast>()
NamespaceDeclaration - - __Blazor.Test.TestComponent
ClassDeclaration - - internal static - TypeInference - -
ComponentTypeInferenceMethod - - __Blazor.Test.TestComponent.TypeInference - CreateColumn_0
21 changes: 10 additions & 11 deletions src/Microsoft.CodeAnalysis.Razor/src/ComponentTagHelperDescriptorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,36 +270,35 @@ private void CreateTypeParameterProperty(TagHelperDescriptorBuilder builder, ITy
// things like constructor constraints and not null constraints in the
// type parameter so we create a single string representation of all the constraints
// here.
var constraintString = new StringBuilder();
if (typeParameter.ConstraintTypes.Any())
var list = new List<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove an allocation by lazily initializing this, assuming this code path is always called and not only if the parameter has some constraint. An alternative would to be add a AppendToStringBuilder that conditionally adds the commas to avoid the interim string allocation from string.Join that happens later on in this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not going to remove the allocation unless we have prove that it matters in a benchmark. This code used to work and we broke it for the sake of removing the allocation without a clear benefit. Let's keep the code simple and understandable (and hopefully bug free)

for (var i = 0; i < typeParameter.ConstraintTypes.Length; i++)
{
constraintString.Append(string.Join(", ", typeParameter.ConstraintTypes.Select(t => t.Name)));
list.Add(typeParameter.ConstraintTypes[i].Name);
}
if (typeParameter.HasConstructorConstraint)
{
constraintString.Append(", new()");
list.Add("new()");
}
if (typeParameter.HasNotNullConstraint)
{
constraintString.Append(", notnull");
list.Add("notnull");
}
if (typeParameter.HasReferenceTypeConstraint)
{
constraintString.Append(", class");
list.Add("class");
}
if (typeParameter.HasUnmanagedTypeConstraint)
{
constraintString.Append(", unmanaged");
list.Add("unmanaged");
}
if (typeParameter.HasValueTypeConstraint)
{
constraintString.Append(", struct");
list.Add("struct");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue was that each block after the one above assumed that there was already a constraint and pre-pended a comma.

Instead of doing that, we collect the constraints on a list and then join them together.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Good fix.

In the new baselines in this PR, I don't see any cases where constraints are joined with commas. Do we definitely have coverage of this?

Copy link
Member

Choose a reason for hiding this comment

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

Also the original issue that this fixes reports the following error:

error CS0449: The 'class', 'struct', 'unmanaged', 'notnull', and 'default' constraints cannot be combined or duplicated, and must be specified first in the constraints list.

The code above doesn't seem to ensure that class/struct/etc would appear first in the list. Is it possible that this line (line 276) should be moved to the end of the list, or is it impossible for us to ensure correct ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've changed this after the fact, I've been adding more tests


if (constraintString.Length > 0)
if (list.Count > 0)
{
constraintString.Insert(0, "where " + typeParameter.Name + " : ");
pb.Metadata[ComponentMetadata.Component.TypeParameterConstraintsKey] = constraintString.ToString();
pb.Metadata[ComponentMetadata.Component.TypeParameterConstraintsKey] = $"where {typeParameter.Name} : {string.Join(", ", list)}";
}

pb.Documentation = string.Format(CultureInfo.InvariantCulture, ComponentResources.ComponentTypeParameter_Documentation, typeParameter.Name, builder.Name);
Expand Down