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

fixed DeclarationNameCompletionProvider when used via Nuget package #36237

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

filipw
Copy link
Contributor

@filipw filipw commented Jun 7, 2019

Microsoft.CodeAnalysis.CSharp.Features doesn't declare Humanizer as a dependency - instead it uses PrivateAssets="all".
PrivateAssets is intended for build-time dependencies only, so the result is that the produced Nuget package doesn't contain the Humanizer.dll nor does it list it as a dependency.

This is a regression that started with Roslyn 2.11.x-beta and continues into Roslyn 3.x, because it used to work.

The consequence of the missing Humanizer is that the DeclarationNameCompletionProvider silently breaks when used via the Nuget package. It ends up throwing an exception about missing Humanizer reference but that exception is swallowed.

This actually broke declaration name completion in OmniSharp and shipped into the latest version of C# extension for VS Code. The current workaround is to reference Humanizer directly in the host application, but obviously the Nuget package should be fixed too.

For the record - repro steps:

          // reference Microsoft.CodeAnalysis.CSharp.Features Nuget

          var host = MefHostServices.Create(MefHostServices.DefaultAssemblies);
          var workspace = new AdhocWorkspace(host);

            var code = @"using System;

            public class MyClass
            {
                public static void MyMethod(int value)
                {
                    MyClass 
                }
            }";

            var projectInfo = ProjectInfo.Create(ProjectId.CreateNewId(), VersionStamp.Create(), "MyProject", "MyProject", LanguageNames.CSharp).
                WithMetadataReferences(new[] { MetadataReference.CreateFromFile(typeof(object).Assembly.Location) });
            var project = workspace.AddProject(projectInfo);
            var document = workspace.AddDocument(project.Id, "MyFile.cs", SourceText.From(code));

            var completionService = CompletionService.GetService(document);
            var results = await completionService.GetCompletionsAsync(document, code.LastIndexOf("MyClass ") + 8);

            // results is null (exception gets swallowed), instead of a completion list with entries like "my", "myClass", "My", "MyClass"

@filipw filipw requested a review from a team as a code owner June 7, 2019 07:43
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Humanizer is an optional dependency of the package. It can be provided by the consumer to enable support for DeclarationNameCompletionProvider, or omitted by the consumer if that support is not needed.

@filipw
Copy link
Contributor Author

filipw commented Jun 7, 2019

Humanizer is an optional dependency of the package. It can be provided by the consumer to enable support for DeclarationNameCompletionProvider, or omitted by the consumer if that support is not needed.

I think it it is quite weird, to say the least, that a package is published in a state in which there are features and types that throw silent exceptions and don't work until you install a 3rd party package? IMHO Roslyn deliverables should be such that you can just install them from nuget and run.

If a dependency on Humanizer is a blocker, perhaps DeclarationNameCompletionProvider ought to be extracted into its own optional package rather than shipping in a broken state?

That said, I hope the change to PrivateAssets="compile" is OK with you 😀

@sharwell sharwell added the Need Design Review The end user experience design needs to be reviewed and approved. label Jun 7, 2019
@filipw filipw mentioned this pull request Jun 19, 2019
@filipw
Copy link
Contributor Author

filipw commented Dec 22, 2019

is there any chance of this getting merged? with the current setup the feature is silently broken..

@sharwell sharwell merged commit 6d27790 into dotnet:master Jan 6, 2020
@sharwell sharwell added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed Need Design Review The end user experience design needs to be reviewed and approved. labels Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants