[FUSE] Provide intellisense for @inject directives #10771

chsienki · 2024-08-20T21:46:58Z

With fuse enabled, it was noticed that typing @inject was not providing IntelliSense on the type.

The directive is @inject <type> <name> with type and name being two different DirectiveTokens. Due to this structure, it's effectively malformed until you begin typing the name. Previously in runtime we don't emit anything for a malformed inject directive. Until you start typing the name, there is no C# to actually map back to the razor document and provide IntelliSense with. Design time works around this by emitting the two tokens as separate design time only elements, so there is always some C# even when the directive is incomplete.

This PR handles malformed @inject directives, and effectively generates an 'error token' that means we can emit valid C# code in these cases. When the user has not yet typed a member name, but started the type name, we begin emitting the full code with a member called Member_<randomGeneratedId>. We still provide an RZ diagnostic, so the code will not compile, but we can still emit something that is valid so we can provide IntelliSense to the user.

I tried adding a malformed Component integration test, but design time is failing with missing pragmas. I tried the test on main, and it failed there too, so nothing to do with these changes. Will try and figure it out, but don't want to block on it.

Fixes https://devdiv.visualstudio.com/DevDiv/_queries/edit/2172170/

chsienki · 2024-08-22T18:13:09Z

...Files/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives_Runtime.codegen.cs

@@ -1,42 +1,42 @@
-#pragma checksum "TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "844eb91b909a14b78feddd5e6866563b5a75e021"
+#pragma checksum "TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml" "{8829d00f-11b8-4213-878b-770e8597ac16}" "c83d1c26cf039a87fc6aedc860fd9d28a34d96dfb2e405e6af3918602ca27755"


These files existed, because apparently the test did at one point, too, but they were super out of date, so I would just review these as newly added.

chsienki · 2024-08-22T19:13:48Z

@dotnet/razor-compiler for review please :)

…id c# identifier so we get intellisense

jjonescz · 2024-08-23T10:11:26Z

...osoft.CodeAnalysis.Razor.Compiler/src/Language/Extensions/DefaultTagHelperTargetExtension.cs

@@ -650,10 +646,13 @@ private string GetContent(HtmlContentIntermediateNode node)
    // Internal for testing
    internal static string GetDeterministicId(CodeRenderingContext context)


Should this be moved somewhere else? It's not used for tag helpers only anymore.

jjonescz · 2024-08-23T10:12:45Z

...Microsoft.AspNetCore.Razor.Test.Common/Language/IntegrationTests/RazorIntegrationTestBase.cs

+
+        public void Configure(RazorCodeGenerationOptionsBuilder options)
+        {
+            options.SuppressUniqueIds = "test";


Consider making this something more recognizable like "TestSuppressedUniqueId"

jjonescz · 2024-08-23T10:14:07Z

...crosoft.AspNetCore.Razor.Language/test/IntegrationTests/ComponentDirectiveIntegrationTest.cs

+    public void SupportsIncompleteInjectDirectives()
+    {
+        var component = CompileToComponent("""
+            @inject 


It's not clear to me how this resolves the issue. The issue talks about IntelliSense on the type part and when user writes @inject and space we still don't generate anything, i.e., nothing has really changed for this sceario, how could the issue be fixed?

I am also confused here.

That's always been true. You don't get IntelliSense until you type the first character today. Once you type the first character, you get it, but in fuse you never get it until you start typing the name:

Not-fuse:

Fuse (broken):

Fuse (fixed):

Note, this is same behavior you get today in C#, if you're e.g. writing a new property:

In C#, can't you manually bring up completion though? What's the equivalent behavior in razor currently?

Ah ok, so if you invoke IntelliSense in either the C# or non-fuse razor case, you just get an empty IntelliSense box.

In the PR right now you get nothing, because we have nothing emitted. In design time that works by mapping to just a totally empty space. We can probably do that here too to get exactly the same behavior.

Hmm, actually this is proving much harder than expected. Not necessarily in generating the code but trying to figure out what exactly we should generate to make IntelliSense happy. I'll chat with @davidwengier next week and try and figure it out.

~~Should we just take this PR as-is as it makes most of the scenario better, and file a follow up issue to fix the empty space part?~~

Lol, wrote this then almost immediately figured it out. Update incoming.

333fred · 2024-08-23T17:05:44Z

...osoft.CodeAnalysis.Razor.Compiler/src/Language/Components/ComponentInjectIntermediateNode.cs

-            defaultValue: true);
+        var memberName = MemberName ?? "Member_" + DefaultTagHelperTargetExtension.GetDeterministicId(context);
+
+        if (!context.Options.DesignTime || !IsMalformed)


Will it mess up the existing design-time experience if we just unconditionally write this property?

No, it shouldn't, I've just been avoiding making any changes to the design time where possible.

333fred · 2024-08-23T17:09:06Z

...crosoft.AspNetCore.Razor.Language/test/IntegrationTests/ComponentDirectiveIntegrationTest.cs

+    public void SupportsIncompleteInjectDirectives()
+    {
+        var component = CompileToComponent("""
+            @inject 


I am also confused here.

chsienki · 2024-08-23T22:42:04Z

@333fred @jjonescz Ok, updated it so you get IntelliSense if you request it even before you've typed a type name at all.

jjonescz · 2024-08-26T06:50:07Z

...Files/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives_Runtime.codegen.cs

@@ -36,6 +36,14 @@ public class TestFiles_IntegrationTests_CodeGenerationIntegrationTest_Incomplete
 #line default
 #line hidden
         Member_test { get; private set; }


Why is this test and not __UniqueIdSuppressedForTesting__?

Wen have two places where we set it. A whole bunch of other places were using the other one, so it made the diff huge. I'll submit a follow up PR to make them consistent.

chsienki commented Aug 22, 2024

View reviewed changes

chsienki force-pushed the fuse_fixes branch 2 times, most recently from 1d7e136 to 025684c Compare August 22, 2024 18:47

chsienki changed the title ~~Fuse fixes~~ [FUSE] Provide intellisense for @inject directives Aug 22, 2024

chsienki marked this pull request as ready for review August 22, 2024 19:13

chsienki requested review from a team as code owners August 22, 2024 19:13

chsienki added 3 commits August 22, 2024 14:16

When @Inject is missing the member name, generate a syntactically val…

b80d39f

…id c# identifier so we get intellisense

Add tests and update baselines

4cd7c94

Minor cleanup

c9b4aea

chsienki force-pushed the fuse_fixes branch from 025684c to c9b4aea Compare August 22, 2024 21:17

Fixup

df8c74a

chsienki force-pushed the fuse_fixes branch from 9bf7957 to df8c74a Compare August 22, 2024 22:00

davidwengier removed the request for review from a team August 23, 2024 01:41

jjonescz reviewed Aug 23, 2024

View reviewed changes

333fred reviewed Aug 23, 2024

View reviewed changes

chsienki added 2 commits August 23, 2024 11:11

Use a more unique test prefix

41b5534

Emit an empty section when there is no typename

2e88d74

333fred approved these changes Aug 23, 2024

View reviewed changes

jjonescz approved these changes Aug 26, 2024

View reviewed changes

chsienki merged commit aa024eb into dotnet:main Aug 26, 2024
12 checks passed

dotnet-policy-service bot added this to the Next milestone Aug 26, 2024

chsienki mentioned this pull request Aug 26, 2024

Suppress unique ids #10791

Merged

dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024

This was referenced Aug 29, 2024

[Automated] PRs inserted in VS build main-35228.257 #10810

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35228.301 #10812

Closed

dotnet-bot mentioned this pull request Sep 14, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35313.170 #10886

Closed

ryzngard mentioned this pull request Oct 1, 2024

Merge main into features/extract-to-component #10948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FUSE] Provide intellisense for @inject directives #10771

[FUSE] Provide intellisense for @inject directives #10771

chsienki commented Aug 20, 2024 •

edited

Loading

chsienki Aug 22, 2024

chsienki commented Aug 22, 2024

jjonescz Aug 23, 2024

jjonescz Aug 23, 2024

jjonescz Aug 23, 2024

333fred Aug 23, 2024

chsienki Aug 23, 2024

333fred Aug 23, 2024

chsienki Aug 23, 2024

chsienki Aug 23, 2024 •

edited

Loading

333fred Aug 23, 2024

chsienki Aug 23, 2024

333fred Aug 23, 2024

chsienki commented Aug 23, 2024

jjonescz Aug 26, 2024

chsienki Aug 26, 2024

		@@ -1,42 +1,42 @@
		#pragma checksum "TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "844eb91b909a14b78feddd5e6866563b5a75e021"
		#pragma checksum "TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml" "{8829d00f-11b8-4213-878b-770e8597ac16}" "c83d1c26cf039a87fc6aedc860fd9d28a34d96dfb2e405e6af3918602ca27755"

		@@ -650,10 +646,13 @@ private string GetContent(HtmlContentIntermediateNode node)
		// Internal for testing
		internal static string GetDeterministicId(CodeRenderingContext context)

[FUSE] Provide intellisense for @inject directives #10771

[FUSE] Provide intellisense for @inject directives #10771

Conversation

chsienki commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

chsienki commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented Aug 20, 2024 •

edited

Loading

chsienki Aug 23, 2024 •

edited

Loading