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

Make completion service public #8170

Merged
merged 2 commits into from May 4, 2016

Conversation

Projects
None yet
8 participants
@mattwar
Copy link
Contributor

mattwar commented Jan 26, 2016

This change makes the completion API public.

Lots of refactoring.

if (_importedProviders == null)
{
var language = this.GetLanguageName();
var mefExporter = (IMefHostExportProvider)_workspace.Services.HostServices;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Jan 26, 2016

Contributor

What if this type doesn't implement that interface?

This comment has been minimized.

@mattwar

mattwar Jan 26, 2016

Contributor

It always does now. We have this dependency in a few places already.

@@ -22,5 +22,10 @@ internal abstract class CompletionListProvider
/// only do minimal textual checks to determine if they should be presented.
/// </summary>
public abstract bool IsTriggerCharacter(SourceText text, int characterPosition, OptionSet options);

public virtual bool IsSnippet

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Jan 26, 2016

Contributor

"IsSnippet" seems really weird to ask a provider. "Hey provider, are you a snippet?" "Why yes i am!" Maybe ProvideSnippets? or IsSnippetProvider?

public CompletionProviderMetadata(IDictionary<string, object> data)
: base(data)
{
this.Role = (string)data.GetValueOrDefault("Role")

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Jan 26, 2016

Contributor

how do we know this is safe to cast to string?

This comment has been minimized.

@mattwar

mattwar Jan 26, 2016

Contributor

Because that's how its defined?

{
public string Name { get; }
public string Language { get; }
public string Role { get; set; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Jan 26, 2016

Contributor

Should someone be able to export a provider for multiple roles?

This comment has been minimized.

@mattwar

mattwar Jan 26, 2016

Contributor

I don't know. I was trying to work in the ones we have defined, and I believe they are only one role each.

@Pilchie

This comment has been minimized.

Copy link
Member

Pilchie commented Jan 26, 2016

Pretty sure @DustinCampbell and @rchande will want to look at this too.

@Pilchie Pilchie added the Area-IDE label Feb 29, 2016

@@ -20,7 +20,7 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
/// <summary>
/// Represents an assembly built by compiler.
/// </summary>
internal sealed class SourceAssemblySymbol : MetadataOrSourceAssemblySymbol, IAttributeTargetSymbol
internal sealed class SourceAssemblySymbol : MetadataOrSourceAssemblySymbol, ISourceAssemblySymbol, IAttributeTargetSymbol

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

Is this supposed to be part of your change?

/// </summary>
public bool IncludesCommitKey { get; }

private CommitChange(ImmutableArray<TextChange> textChanges, int? newPosition, bool includesCommit)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

What's the formatting story? Is the service expected to give you the formatted text changes? Or oes the controller format things?

return new CommitChange(changes, this.NewPosition, this.IncludesCommitKey);
}

public CommitChange WithNewPosition(int? position)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

this name should be "newPosition". Especially in the new immutable world, we want parameternames/propertynames to be consistent.

return new CommitChange(textChanges, newPosition, includesCommitKey);
}

public CommitChange WithTextChanges(ImmutableArray<TextChange> changes)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

should be "textChanges"

/// The <see cref="CommitRule"/> specifies keys that cannot be used to
/// commit the <see cref="CompletionItem"/>
/// </summary>
ExcludeKeys,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

When is ExcludeKeys necessary?

/// commit the <see cref="CompletionItem"/> if the text typed since the start of completion
/// ends with the <see cref="CommitRule.Match"/> text.
/// </summary>
ExcludeKeysIfMatchEndOfTypedText

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 7, 2016

Contributor

Skeptical if this is necessary. Even for the \\ case in interactive I'd prefer this just be gone.

}

private bool? SpecialException(CompletionItem item1, CompletionItem item2)
{

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

This seems awful. It shouldn't be the case that this layer is even aware of language names. All language specific behavior should always be in a library provided by that language. This really violates that pattern we've been following everywhere else.

/// </summary>
public virtual CompletionListProvider CompletionProvider { get; }
public Glyph? Glyph { get; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

i agree that this should be an immutable set of tags. note: i do worry about what the impact of that might be perfwise if we then have to map 100,000 of those to glyphs. But maybe that can be lazily done?

/// The description of the item broken down into display parts.
/// If the description is empty, then the description can be accessed via <see cref="CompletionService.GetDescriptionAsync(Document, CompletionItem, CancellationToken)"/>.
/// </summary>
public ImmutableArray<SymbolDisplayPart> Description { get; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

Ideally removed. I'd like to knwo the cases where we think this is more appropriate/necessary.

/// </summary>
/// <returns></returns>
public bool ShowsWarningIcon { get; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

Should just be a tag.

/// <summary>
/// True if the item represent an argument name in a named argument list.
/// </summary>
public bool IsArgumentName { get; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

Should not be part of the services API. controller should not know about this. This is a language detail leaking into a language agnostic system.

{
var match1 = GetMatch(item1, _completionService.GetCultureSpecificQuirks(filterText));
var match2 = GetMatch(item2, _completionService.GetCultureSpecificQuirks(filterText));
public bool SupportSnippetCompletionListOnTab { get; }

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

move to "IsTriggerCharacter".

/// The <see cref="FilterRule"/> specifies keys that cannot be used to
/// filter the <see cref="CompletionItem"/>.
/// </summary>
ExcludeKeys,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

What do we need Exclude for?

/// if the text typed so far is a subset of the display text.
/// ??? Shouldn't this mean only the next character in the display text?
/// </summary>
FilterIfDisplayTextStartsWithTextTyped,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

Can we make this the default behavior.

/// <summary>
/// Pick based on the item being an argument name
/// </summary>
ArgumentName,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

I would prefer if the last two are not in this list. They shoudl be done within the service itself, not at the controller level.

/// <summary>
/// Pick based on strength of punctuation in the item matching the pattern
/// </summary>
Punctuation,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

Can you give an example of this?


public static string EncodeSymbol(ISymbol symbol)
{
return SymbolId.CreateId(symbol);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

I feel like this should be SymbolKey, just with well defined tostring/fromstring methods on it.

internal static class WellKnownItemKinds
{
//public static readonly string Snippet = "Snippet";
public static readonly string Keyword = "Keyword";

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Mar 8, 2016

Contributor

use nameof

@mattwar

This comment has been minimized.

Copy link
Contributor

mattwar commented Mar 24, 2016

@dotnet-bot test eta

@mattwar mattwar changed the title Remove providers from ICompletionService Make completion service public Mar 24, 2016

@mattwar

This comment has been minimized.

Copy link
Contributor

mattwar commented Mar 25, 2016

Okay @DustinCampbell @Pilchie @rchande @CyrusNajmabadi

Here's the real proposed change with all the bits made public.

;slsle Outdated
@@ -0,0 +1,68 @@
diff --git a/src/EditorFeatures/Core/Implementation/IntelliSense/Completion/Controller_Commit.cs b/src/EditorFeatures/Core/Implementation/IntelliSense/Completion/Controller_Commit.cs

This comment has been minimized.

@rchande

rchande Apr 1, 2016

Member

What is this file?

This comment has been minimized.

@mattwar

mattwar Apr 1, 2016

Contributor

I have not idea. I did not create it.

@rchande

This comment has been minimized.

Copy link
Member

rchande commented Apr 1, 2016

@mattwar: I looked through this and nothing seemed obviously wrong to me. I did want to build it locally and try things out. However you're missing some changes in master (as you can see from Jenkins) and I encountered a ton of merge commits trying to sync.

@mattwar

This comment has been minimized.

Copy link
Contributor

mattwar commented May 4, 2016

retest prtest/win/vsi/p0 please
retest prtest/win/vsi/p2 please

@mattwar mattwar merged commit 1a3b616 into dotnet:master May 4, 2016

8 of 9 checks passed

roslyn_ignore_this_test_eta2 Build finished.
Details
prtest/lin/dbg/unit32 Build finished.
Details
prtest/win/dbg/eta Build finished.
Details
prtest/win/dbg/unit32 Build finished.
Details
prtest/win/dbg/unit64 Build finished.
Details
prtest/win/vsi/p0 Build finished.
Details
prtest/win/vsi/p1 Build finished.
Details
prtest/win/vsi/p2 Build finished.
Details
prtest/win/vsi/p3 Build finished.
Details
@npolyak

This comment has been minimized.

Copy link

npolyak commented May 17, 2016

When will the changes for this Issue make it into a NuGet release (which release is going to be associated with these changes?)

@Pilchie

This comment has been minimized.

Copy link
Member

Pilchie commented May 17, 2016

You can get them on our 1.3 myget feed already, and they should show up on NuGet when our 1.3 release comes out (with VS 2015 Update 3).

@DustinCampbell

This comment has been minimized.

Copy link
Member

DustinCampbell commented May 17, 2016

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 18, 2016

Thanks

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 19, 2016

How do I get a reference to the completion service?

I am getting the language services from a C# project or from a workspace containing it:
HostLanguageServices csServices = MyWorkspace.Services.GetLanguageServices("C#");

but then csServices.GetService() returns null.

THanks
Nick

@Pilchie

This comment has been minimized.

Copy link
Member

Pilchie commented May 19, 2016

What sort of Workpace are you in? Is it possible that the CSharpFeatures assembly is not part of the MEF composition used to populate the workspace and language services?

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 19, 2016

After I created my own MefHostServices with the composition container that loaded CSharpFeatures and created the Workspace based on that, I started getting the completion service, but should not there be a better way to do it?

Here is my code:

       var assemblies = new[]
        {
            Assembly.Load("Microsoft.CodeAnalysis"),
            Assembly.Load("Microsoft.CodeAnalysis.CSharp"),
            Assembly.Load("Microsoft.CodeAnalysis.Features"),
            Assembly.Load("Microsoft.CodeAnalysis.CSharp.Features")
        };
        _compositionContext = new ContainerConfiguration()
            .WithAssemblies(MefHostServices.DefaultAssemblies.Concat(assemblies))
            .CreateContainer();
        _host = MefHostServices.Create(_compositionContext);
        TheWorkspace = new AdhocWorkspace(_host, "Host");

Is there a better way to make sure the CompletionServices exist?

@Pilchie

This comment has been minimized.

Copy link
Member

Pilchie commented May 19, 2016

I don't know of a better way to do it. @mattwar Should we consider adding adding these two MefHostServices.DefaultAssemblies, or providing other built-ins?

@mattwar

This comment has been minimized.

Copy link
Contributor

mattwar commented May 19, 2016

@Pilchie that sort of makes sense, seeing as we are making these APIs public now.

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 23, 2016

How can I get the type of a CompletionItem now (whether it is a property or a method or an event)?

Before you had a Glyph property on the CompletionItem - but it is no longer there.

Thanks

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 23, 2016

I think i got it - it is now in Tags. Still I think an enumeration would be better.

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 23, 2016

Let me clarify myself - from my point of view - it would be better to rename the Glyph enumeration to CompletionType, make it public and create a public CompletionType property within CompletionItem class. That way the ViewModel does not have any hints of View constracts (like Glyph) and value conversion can achieve transformation between the CompletionType and the corresponding image.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 23, 2016

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 24, 2016

Hey Cyrus,
it is simply much clearer and more type safe, which is better for learning the code and using it.
The tags are strings that potentially can contain anything. CompletionType can be a predefined enumeration that can only contain certain values.
Imagine a person learning the code - and I was such person very recently - He can go to the definition of the CompletionType and learn what it is without running the application. I had to run the application and in the beginning I simply did not realize that the Tags array had to be opened to get the type of the completions. Also even now, I do not know, perhaps sometimes it can contain smth completely different. Finally your own developers might not understand what it is for and use it for something else, resulting in an error.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 24, 2016

The tags are strings that potentially can contain anything.

Yes, that's a virtue :)

CompletionType can be a predefined enumeration that can only contain certain values.

That's a problem. Now we need to know the set of things that will go in completion. As this is an extensible API that we would like any language to be able to plug into, that's not ok.

Also even now, I do not know, perhaps sometimes it can contain smth completely different.

Yes. It may. Any code processing it will have to handle this case.

Finally your own developers might not understand what it is for and use it for something else, resulting in an error.

It can be used for anything else. The point is that's its a mechanism to allow people to use it how they want.

We're moving away from being prescriptive here. That's because it's really hard to be both prescriptive, while still being generally usable by any language. Instead, we allow providers to be descriptive. They loosely state what they are, and consumers can loosely process what they understand.

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 25, 2016

I do not want to spend much time arguing, but the strong typing is very important for the ease of usage and in no way restrictive (when you have a new completion type you can always modify the enumeration).

@DustinCampbell

This comment has been minimized.

Copy link
Member

DustinCampbell commented May 25, 2016

@npolyak, modifying an enumeration is actually an API breaking change. It can break existing code that uses the enumeration.

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 25, 2016

Using the same logic, one can drop all the strong types because modifying them is an API breaking change.

@DustinCampbell

This comment has been minimized.

Copy link
Member

DustinCampbell commented May 25, 2016

No, that's not true at all. Adding members to enumerations can specifically break existing code, for example a switch case that tests every value and throws in the default case. That'll fail at runtime.

That does not extend to, say, classes, where members can be added in a non-breaking way.

@npolyak

This comment has been minimized.

Copy link

npolyak commented May 25, 2016

Yes, if e.g you remove a class field and there is a code which uses that field, it can definitely break. The key is - the word 'can' - both enum and class change can break existing API but do not have to.

A shift towards less type safe approach brings more problems from my point of view than benefits. Anyways I rest my case.

@DustinCampbell

This comment has been minimized.

Copy link
Member

DustinCampbell commented May 25, 2016

Appreciate the feedback.

@aelij

This comment has been minimized.

Copy link
Contributor

aelij commented Jun 1, 2016

I was also somewhat irked to find Glyph missing from the public API, but this makes total sense. Thanks for the explanation.

Wouldn't it be better for Tags to be a dictionary, though? It would make the ImageMonikers class (and similar implementations) much nicer. For example, you could have Tags.TryGetValue(TagCategories.Accessibility, out accessibility)

@mattwar mattwar deleted the mattwar:FeatureAPIs branch Aug 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment