Skip to content

Require public component parameters and remove private metadata workaround #915

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
merged 1 commit into from
Jul 31, 2019
Merged

Require public component parameters and remove private metadata workaround #915

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
32 changes: 14 additions & 18 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor/ComponentTagHelperDescriptorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Components;
using Microsoft.CodeAnalysis.CSharp;

namespace Microsoft.CodeAnalysis.Razor
{
Expand All @@ -18,10 +16,6 @@ internal class ComponentTagHelperDescriptorProvider : RazorEngineFeatureBase, IT
.WithGlobalNamespaceStyle(SymbolDisplayGlobalNamespaceStyle.Omitted)
.WithMiscellaneousOptions(SymbolDisplayFormat.FullyQualifiedFormat.MiscellaneousOptions & (~SymbolDisplayMiscellaneousOptions.UseSpecialTypes));

private static MethodInfo WithMetadataImportOptionsMethodInfo =
typeof(CSharpCompilationOptions)
.GetMethod("WithMetadataImportOptions", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);

public bool IncludeDocumentation { get; set; }

public int Order { get; set; }
Expand All @@ -40,9 +34,6 @@ public void Execute(TagHelperDescriptorProviderContext context)
return;
}

// We need to see private members too
compilation = WithMetadataImportOptionsAll(compilation);

var symbols = ComponentSymbols.Create(compilation);

var types = new List<INamedTypeSymbol>();
Expand Down Expand Up @@ -81,13 +72,6 @@ public void Execute(TagHelperDescriptorProviderContext context)
}
}

private Compilation WithMetadataImportOptionsAll(Compilation compilation)
{
var newCompilationOptions = (CSharpCompilationOptions)WithMetadataImportOptionsMethodInfo
.Invoke(compilation.Options, new object[] { /* All */ (byte)2 });
return compilation.WithOptions(newCompilationOptions);
}

private TagHelperDescriptor CreateShortNameMatchingDescriptor(ComponentSymbols symbols, INamedTypeSymbol type)
{
var builder = CreateDescriptorBuilder(symbols, type);
Expand Down Expand Up @@ -216,9 +200,9 @@ bool HasTypeParameter(ITypeSymbol type)
}

// We need to check for cases like:
// [Parameter] List<T> MyProperty { get; set; }
// [Parameter] public List<T> MyProperty { get; set; }
// AND
// [Parameter] List<string> MyProperty { get; set; }
// [Parameter] public List<string> MyProperty { get; set; }
//
// We need to inspect the type arguments to tell the difference between a property that
// uses the containing class' type parameter(s) and a vanilla usage of generic types like
Expand Down Expand Up @@ -333,6 +317,7 @@ private void CreateContextParameter(TagHelperDescriptorBuilder builder, string c
// a dictionary keyed on property name.
//
// We consider parameters to be defined by properties satisfying all of the following:
// - are public
Copy link
Author

@NTaylorMullen NTaylorMullen Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Requiring public wasn't initially mentioned in the component visibility issue but it felt "more right" to me in an effort to default to being more restrictive for RTM to avoid edge cases. Not to mention it'll be an analyzer error by default. Let me know if you feel differently.

Copy link
Member

Choose a reason for hiding this comment

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

sure

// - are visible (not shadowed)
// - have the [Parameter] attribute
// - have a setter, even if private
Expand Down Expand Up @@ -367,6 +352,12 @@ private void CreateContextParameter(TagHelperDescriptorBuilder builder, string c
}

var kind = PropertyKind.Default;
if (property.DeclaredAccessibility != Accessibility.Public)
{
// Not public
kind = PropertyKind.Ignored;
}

if (property.Parameters.Length != 0)
{
// Indexer
Expand All @@ -378,6 +369,11 @@ private void CreateContextParameter(TagHelperDescriptorBuilder builder, string c
// No setter
kind = PropertyKind.Ignored;
}
else if (property.SetMethod.DeclaredAccessibility != Accessibility.Public)
{
// No public setter
kind = PropertyKind.Ignored;
}

if (property.IsStatic)
{
Expand Down
80 changes: 80 additions & 0 deletions src/Razor/test/Microsoft.CodeAnalysis.Razor.Test/ComponentTagHelperDescriptorProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,86 @@ public class MyComponent : ComponentBase
Assert.Equal("System.String", attribute.TypeName);
}

[Fact]
public void Execute_IgnoresPrivateParameters_CreatesDescriptor()
{
// Arrange

var compilation = BaseCompilation.AddSyntaxTrees(Parse(@"
using Microsoft.AspNetCore.Components;

namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter]
private string MyProperty { get; set; }
}
}

"));

Assert.Empty(compilation.GetDiagnostics());

var context = TagHelperDescriptorProviderContext.Create();
context.SetCompilation(compilation);

var provider = new ComponentTagHelperDescriptorProvider();

// Act
provider.Execute(context);

// Assert
var components = ExcludeBuiltInComponents(context);
components = AssertAndExcludeFullyQualifiedNameMatchComponents(components, expectedCount: 1);
var component = Assert.Single(components);

Assert.Equal("TestAssembly", component.AssemblyName);
Assert.Equal("Test.MyComponent", component.Name);

Assert.Empty(component.BoundAttributes);
}

[Fact]
public void Execute_IgnoresParametersWithPrivateSetters_CreatesDescriptor()
{
// Arrange

var compilation = BaseCompilation.AddSyntaxTrees(Parse(@"
using Microsoft.AspNetCore.Components;

namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter]
public string MyProperty { get; private set; }
}
}

"));

Assert.Empty(compilation.GetDiagnostics());

var context = TagHelperDescriptorProviderContext.Create();
context.SetCompilation(compilation);

var provider = new ComponentTagHelperDescriptorProvider();

// Act
provider.Execute(context);

// Assert
var components = ExcludeBuiltInComponents(context);
components = AssertAndExcludeFullyQualifiedNameMatchComponents(components, expectedCount: 1);
var component = Assert.Single(components);

Assert.Equal("TestAssembly", component.AssemblyName);
Assert.Equal("Test.MyComponent", component.Name);

Assert.Empty(component.BoundAttributes);
}

[Fact] // bool properties support minimized attributes
public void Execute_BoolProperty_CreatesDescriptor()
{
Expand Down
24 changes: 12 additions & 12 deletions src/Razor/test/RazorLanguage.Test/IntegrationTests/ComponentCodeGenerationTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4574,7 +4574,7 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public string Message { get; set; }
}
}
"));
Expand Down Expand Up @@ -4603,7 +4603,7 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public string Message { get; set; }
}
}
"));
Expand Down Expand Up @@ -4634,7 +4634,7 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public string Message { get; set; }
}
}
"));
Expand Down Expand Up @@ -4664,9 +4664,9 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public EventCallback<string> MessageChanged { get; private set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; private set; }
[Parameter] public string Message { get; set; }
[Parameter] public EventCallback<string> MessageChanged { get; set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; set; }
}
}
"));
Expand Down Expand Up @@ -4699,9 +4699,9 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public EventCallback<string> MessageChanged { get; private set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; private set; }
[Parameter] public string Message { get; set; }
[Parameter] public EventCallback<string> MessageChanged { get; set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; set; }
}
}
"));
Expand Down Expand Up @@ -4734,9 +4734,9 @@ namespace Test
{
public class MyComponent : ComponentBase
{
[Parameter] public string Message { get; private set; }
[Parameter] public EventCallback<string> MessageChanged { get; private set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; private set; }
[Parameter] public string Message { get; set; }
[Parameter] public EventCallback<string> MessageChanged { get; set; }
[Parameter] public Expression<Action<string>> MessageExpression { get; set; }
}
}
"));
Expand Down