Fix completion at end of document #11343
Conversation
Was getting annoying test failures due to having no snippets set up in perfectly valid scenarios
| absoluteIndex > 0) | ||
| { | ||
| // If we're at the end of the file, we check if the previous token is an open angle (ie, we're in <$$[EOF]) | ||
| // because even though the tree won't match our expectations for an incomplete start tag, thats what it is. |
There was a problem hiding this comment.
Would it make sense to modify RazorSyntaxFacts.IsAnyStarTag instead?
There was a problem hiding this comment.
Even if we did that, it wouldn't solve the issue for this because the final check in this method checks the span of the start tag, and that span doesn't include the end of file. Unless the compiler changes the shape of the Razor syntax tree I think this is always going to need some kind of special-casing.
| } | ||
| snippetCompletionItemProvider.SnippetCache.Update(SnippetLanguage.Html, [ | ||
| new SnippetInfo("snippet1", "snippet1", "snippet1", string.Empty, SnippetLanguage.Html), | ||
| new SnippetInfo("snippet2", "snippet2", "snippet2", string.Empty, SnippetLanguage.Html)]); |
There was a problem hiding this comment.
I really don't like this actually. It's not at all clear not whey the test is specifying that "snippet1" and "snippet2" are not expected. Also we are hardcoding setup in Verify method.
I wouldn't have a problem with some sort of a common setup instead, but this looks wrong to me.
There was a problem hiding this comment.
All of our test setup is in this Verify method, and this change is simply making snippets part of the common setup. IMO tests should state the inputs, and expected outputs, and only do set up if there is something special they need. In a real user scenario there are snippets that are installed by default, therefore having snippets present by default in tests makes sense to me. If there was a test that specifically wanted to cover a scenario where there are no snippets, that should be the exception not the rule.
I've updated the tests to use a shared constant for each snippet, so there is no "magic string" effect in the tests, does that help?
There was a problem hiding this comment.
Thank you, that does make it easier to reason about it IMHO - I may have even made the snippet label collection private static readonly field.
I guess a logical question would be - should be we do the same thing with HTML labels then? (Not asking you to do either change in this PR)
There was a problem hiding this comment.
Yeah, probably. If we assume that any time Html is asked for completion items it will return "div" and "a", then a test that doesn't specify delegatedItemLabels could be passing by accident because it assumes that the delegated server wasn't called, but in actual fact there just wasn't a test setup for it.
Probably should have all tests explicitly verify whether they expect Html completion, C# completion, tag helper completion and/or snippet completion, and verify the presence/lack of items every time.
There was a problem hiding this comment.
I ended up reverting this change, as it was actually hiding a bug. I added it initially because I was annoyed at a test failing because the "Update" method on the snippet provider happened to not be called, and I thought it was a test setup issue, but turns out that actually signifies a product bug and should have failed the test, just ideally in a better way. I'm fixing that now by adding dummy delegated and snippet items that should never appear as results, and failing the test if they do.
| @@ -292,7 +292,7 @@ await _provider.GetCompletionListAsync( | |||
|
|
|||
| [Theory] | |||
| [InlineData("$$", true)] | |||
| [InlineData("<$$", true)] | |||
| [InlineData("<$$", false)] | |||
| [InlineData(">$$", true)] | |||
There was a problem hiding this comment.
This is a behavior change from the legacy editor. Legacy editor would've included snippets here. If we are trying to reduce friction while removing legacy editor, this might be a somewhat important scenario. E.g. previously (in legacy) you could type in "<" to invoke html completion, scroll or type more to select snippet, and tab to complete snippet. With this change snippets will be absent (and possibly they should've been present in more cases).
I want to make sure we realize there is a behavior change here that affects end users and are fine with it if we are to go through with this change.
There was a problem hiding this comment.
This PR is not introducing that behaviour change, it was introduced when we added snippets. We don't show snippets when bringing up completion inside a tag, legacy does. This PR doesn't change that, it just makes our experience the same whether typing at the end of a document or in the middle.
There was a problem hiding this comment.
In that case we probably should've had a test case like <$$ <div></div> to illustrate that. This was before my time as active contributor, I assume we discussed this behavior difference and are fine with it?
There was a problem hiding this comment.
I don't remember specifically but given LSP snippets and legacy snippets work in a fundamentally different way, I don't think it could have happened any other way. I think of it more like "show a subset of snippets as completion items", rather than "support snippets in Razor". If we were to show snippets inside elements in the LSP editor the user would end up with two <s at the start of their tag, as the snippet includes one.
There was a problem hiding this comment.
Yeah, there is special code in the legacy HTML to see if there is an "<" in front of snippet shortcut. I guess we can leave it as is unless there are user complaints (especially since we already shipped it like this and haven't heard any complaints AFAIK). @danroth27 and @leslierichardson95 FYI.
Fixes element completion at the end of a document.
Also closes #4622 by adding a test to prove it doesn't repro.