Skip to content
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.

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

Add EA layer for ASP.NET Core. #60948

Merged
merged 31 commits into from
May 5, 2022
Merged

Conversation

CyrusNajmabadi
Copy link
Member

This will allow ASP.NET to classify string literals that are passed to apis annotated with SomeMethod([StringSyntax("Route")] string route).

@@ -501,86 +501,9 @@ Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "VBCSCompilerCommandLine", "
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "VBCSCompiler-arm64", "src\Compilers\Server\VBCSCompiler-arm64\VBCSCompiler-arm64.csproj", "{DC8C78CC-B6FE-47BF-93B1-B65A1C67C08D}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.CodeAnalysis.ExternalAccess.AspNetCore", "src\Tools\ExternalAccess\AspNetCore\Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.csproj", "{8C21FF08-8BB5-43E4-AC10-74BD2CBEBC24}"
Copy link
Member Author

Choose a reason for hiding this comment

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

i didn't edit this file myself. VS did. i have no clue if all the changes are correct or not.

@@ -49,6 +49,7 @@
<InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServices.Implementation" />
<InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServices.VisualBasic" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.Remote.ServiceHub" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.AspNetCore" />
Copy link
Member Author

Choose a reason for hiding this comment

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

i generally followed the pattern of F#, updating our code to match waht we have fort hat.


namespace Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.EmbeddedLanguages
{
internal interface IAspNetCoreRouteEmbeddedLanguageClassifier
Copy link
Member Author

Choose a reason for hiding this comment

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

@JamesNK This is the type you would implement. You would do it as:

[Export(typeof(AspNetCoreRouteEmbeddedLanguageClassifier))]
class ActualAspNetCoreRouteEmbeddedLanguageClassifier : IAspNetCoreRouteEmbeddedLanguageClassifier
{
    // ...
}

@jasonmalinowski would james need a particular 'Shared' attribute here?

Copy link
Contributor

@mavasani mavasani Apr 26, 2022

Choose a reason for hiding this comment

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

All the ExternalAccess types that are exposed and allowed to be accessed by the partner need to be inside a special Api folder and also within a namespace name suffixed with .Api. For example, see everything inside https://github.com/dotnet/roslyn/tree/main/src/Workspaces/Core/Portable/ExternalAccess or https://github.com/dotnet/roslyn/tree/main/src/VisualStudio/Core/Def/ExternalAccess. This will ensure that the EA partner only accesses the types within X.Y.Z.Api namespace that they were given access to.

Copy link
Contributor

@mavasani mavasani Apr 26, 2022

Choose a reason for hiding this comment

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

@sharwell Do you know if we have a doc somewhere in the repo to guide how to add RestrictedIVT and patterns to follow to ensure the RestrictedInternalsVisibleToAnalyzer validates correct usage of restricted internal API surface? If not, we should definitely add one.

{
[ExportEmbeddedLanguageClassifierInternal(
nameof(AspNetCoreRouteEmbeddedLanguageClassifier), LanguageNames.CSharp, supportsUnannotatedAPIs: false, "Route"), Shared]
internal class AspNetCoreRouteEmbeddedLanguageClassifier : IEmbeddedLanguageClassifier
Copy link
Member Author

Choose a reason for hiding this comment

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

@JamesNK this is the type we export on your behalf that hooks you into roslyn. this allows us to change that general shape without breaking you. This type then imports the IAspNetCoreRouteEmbeddedLanguageClassifier value which you export to defer to you to actually do the work.

if we ever need to change the full end to end communication api, this allows us to stage the individual parts.

@CyrusNajmabadi
Copy link
Member Author

@sharwell @jasonmalinowski please LMK how i've done this wrong :)

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review April 26, 2022 00:08
@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners April 26, 2022 00:08
namespace Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.Internal.EmbeddedLanguages
{
[ExportEmbeddedLanguageClassifierInternal(
nameof(AspNetCoreRouteEmbeddedLanguageClassifier), LanguageNames.CSharp, supportsUnannotatedAPIs: false, "Route"), Shared]
Copy link
Member Author

Choose a reason for hiding this comment

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

from an RPS perspective, we should be ok here. We use these constants so taht our completion engine does not try to instantiate these providers unless it sees a StringSyntaxAttribute with "Route" as the argument. So in RPS tests we should not load anything in this dll.

(this is similar to how we make sure that in C# scenarios we're not loading VB features).

@@ -113,6 +113,7 @@
<RestrictedInternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServerClient.Razor" Partner="Razor" Key="$(RazorKey)" />
<RestrictedInternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServerClient.Razor.Test" Partner="Razor" Key="$(RazorKey)" />
<InternalsVisibleTo Include="IdeCoreBenchmarks" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.AspNetCore" />
Copy link
Contributor

@mavasani mavasani Apr 26, 2022

Choose a reason for hiding this comment

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

Same here, should be RestrictedInternalsVisibleTo similar to two lines above, otherwise this is just a regular IVT with access to full internal surface area, and having an ExternalAccess layer serves no purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good!

Copy link
Member Author

Choose a reason for hiding this comment

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

hrmm... i couldn't get thsi to work. are you sure that's the right pattern here? i'm creating a new EA lib that should have IVT to our libs. but it's that lib htat we can then audit to make sure it is not further exposing things outwards that it shoudln't.

Copy link
Member

Choose a reason for hiding this comment

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

➡️ RestrictedInternalsVisibleTo is only applicable to cases where we cannot create a new assembly. (marking unresolved just for visibility)

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-ide this is ready for another pass.

@JamesNK Here's the general approach you're going to need to take.

First, if you want to add colors to VS, especially colors that show up in 'Fonts & Colors' in 'Tools | Options', then you're going to need to author a VSIX and add ClassificationTypeDefinitions similar to what Roslyn does here: https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/Classification/ClassificationTypeDefinitions.cs

If you don't need this configurability, you can just use any of the predefined classification names VS always provides, like "keyword" (and this will likely be a good starting point just for testing htings out).

Second, to actually engage with our classifier, you just need to do the following:

[ExportAspNetCoreEmbeddedLanguageClassifier(Name = "WhateverYouWant", Language = LanguageNames.CSharp)]
class JNKAspNetClassifier : IAspNetCoreEmbeddedLanguageClassifier
{
    public void RegisterClassifications(AspNetCoreEmbeddedLanguageClassificationContext context)
    {
        var cancellationToken = context.CancellationToken;
        var semanticModel = context.SemanticModel;
        var syntaxToken = context.SyntaxToken;

        // whatever logic you want here.  go nuts.
        context.AddClassification("your-classification-type", theSpanToClassify); // do this as many times as you want.
    }
}

Note: you will put this code in a dll you know the user will be including in their project as an analyzer reference (for example Microsoft.AspNetCore.App.Analyzers.dll or Microsoft.AspNetCore.App.CodeFixes.dll).

The architecture here is that we are running these classifiers in our external process ("OOP") and we use the analyzer infrastructure to discover you JNKAspNetClassifier impl on the OOP side and instantiate it. Our classifier will then call into you for strings marked with the "Route" attribute name. If you need more names supported in the future, let us know. Currently there is no way to dynamically choose this attribute name. We will call into you through that RegisterClassifications method, providing you both with the syntax token for the string-literal that is being passed along to an api with the "Route" attribute, as well as the semantic model so you can look up semantics if you need them.

LMK what otehr info you need. Thanks!

// Following CWTs are used to cache the providers from projects' references,
// so we can avoid the slow path unless there's any change to the references.
private readonly ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, StrongBox<ImmutableArray<IAspNetCoreEmbeddedLanguageClassifier>>> _analyzerReferencesToClassifiersMap = new();
private readonly ConditionalWeakTable<AnalyzerReference, ClassifierExtensionProvider> _analyzerReferenceToProviderMap = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

pattern copied from completion where we do the same to load completion providers from metadata.

…reEmbeddedLanguageClassificationContext.cs

Co-authored-by: James Newton-King <[email protected]>
@CyrusNajmabadi
Copy link
Member Author

@sharwell for eyes.

// </auto-generated>
//------------------------------------------------------------------------------

namespace Microsoft.CodeAnalysis.ExternalAccess.AspNetCore {
Copy link
Member

Choose a reason for hiding this comment

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

😕 This file shouldn't be necessary (Arcade generates the designer files at build time)

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove in followup!

/// </summary>
[MetadataAttribute]
[AttributeUsage(AttributeTargets.Class)]
internal class ExportAspNetCoreEmbeddedLanguageClassifierAttribute : ExportAttribute
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal class ExportAspNetCoreEmbeddedLanguageClassifierAttribute : ExportAttribute
internal sealed class ExportAspNetCoreEmbeddedLanguageClassifierAttribute : ExportAttribute

@CyrusNajmabadi CyrusNajmabadi merged commit bac2055 into dotnet:main May 5, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the aspEA branch May 5, 2022 20:13
@ghost ghost added this to the Next milestone May 5, 2022
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants