Remove direct dependency on CommandLineParser #1876

brettfo · 2017-03-28T20:35:49Z

This is based off of @srivatsn's work in #1670.

I wanted to get this out for review, but since #1670 hasn't been merged yet, I'm including those commits just so that stuff will build. I'll re-base from master before merging, but I really just wanted to get some feedback about this, first. As such, you should only be considering the last commit made by me. Refer to Sri's PR for the rest of the work.

In short, this PR removes the dependence on the type CommandLineParser because it has numerous internal components and deriving from that for F# would either require additional InternalsVisibleTo elements in the Roslyn code base (which we don't want) or it would require making numerous types and APIs public, which seems heavy-handed. So the solution I went with simply adds an interface that has one Parse() method that matches CommandLineParser.Parse() exactly so that F# can play along, too.

This work is dependent on my other PR, dotnet/roslyn#18258.

davkean · 2017-03-29T00:25:54Z

...ectSystem.VisualBasic/ProjectSystem/LanguageServices/VisualBasicCommandLineParserProvider.cs

+            get { return new VisualBasicParseCommandLineArguments(); }
+        }
+
+        internal class VisualBasicParseCommandLineArguments : IParseCommandLineArguments


Instead of continuing with the Export property approach, turn this inner class into the actual thing that is exporting.

davkean · 2017-03-29T00:26:31Z

...ectSystem.VisualBasic/ProjectSystem/LanguageServices/VisualBasicCommandLineParserProvider.cs

+
+        internal class VisualBasicParseCommandLineArguments : IParseCommandLineArguments
+        {
+            private VisualBasicCommandLineParser _parser = VisualBasicCommandLineParser.Default;


Why do you even need a field here - why can you just call through Default property in the Parse method?

davkean · 2017-03-29T00:27:44Z

While this removes the dependency on CommandLineParser - it still has a dependency on CommandLineArguments won't this suffer the same problem?

davkean

As above.

brettfo · 2017-03-29T20:41:13Z

I've made some changes as requested by @davkean by factoring out a class that only contains the properties consumed on Microsoft.CodeAnalysis.CommandLineArguments.

There is one oddity, left, however: If I try to export the command line parser classes directly then I get the error:

Attribute 'AppliesToAttribute' comes from a different version of MEF than the export attribute on 'CSharpParseCommandLineArguments'

But I get no error when I use an [ImportingConstructor] and export the property, even though I simply copy/paste the attributes. Any ideas?

Edit:
And the only Roslyn change required to make this work for F# is in dotnet/roslyn#18258. As it stands this will still work for C#/VB.

srivatsn · 2017-03-29T21:57:21Z

I think that warning comes from a buggy analyzer and the warning's been suppressed in other projects.

srivatsn · 2017-03-29T21:58:23Z

...rojectSystem.Managed.UnitTests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests.csproj

@@ -25,6 +25,7 @@
    <ProjectReference Include="..\DeployTestDependencies\DeployTestDependencies.csproj">
      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
    </ProjectReference>
+    <ProjectReference Include="..\Microsoft.VisualStudio.ProjectSystem.CSharp\Microsoft.VisualStudio.ProjectSystem.CSharp.csproj" />


Is this right? Seems like a layer violation - managed.unittests shouldn't know about C#

davkean · 2017-03-29T22:26:02Z

...tudio.ProjectSystem.CSharp/ProjectSystem/LanguageServices/CSharpParseCommandLineArguments.cs

+                CSharpCommandLineParser.Default.Parse(args, baseDirectory, sdkDirectory: null, additionalReferenceDirectories: null));
+        }
+
+        [Export(typeof(IParseCommandLineArguments))]


Apply this to the class, the property is unneeded.

Will do; I'll have to disable the analyzer as per @srivatsn's comment.

davkean · 2017-03-29T22:26:15Z

...tudio.ProjectSystem.CSharp/ProjectSystem/LanguageServices/CSharpParseCommandLineArguments.cs

+
+        public CommandLineArguments Parse(IEnumerable<string> args, string baseDirectory)
+        {
+            return CommandLineArguments.FromCommonCommandLineArguments(


I don't understand why you need this?

I see below why, sync up with me and we can discuss the approach.

davkean · 2017-03-29T22:26:35Z

...tudio.ProjectSystem.CSharp/ProjectSystem/LanguageServices/CSharpParseCommandLineArguments.cs

+
+namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices
+{
+    public class CSharpParseCommandLineArguments : IParseCommandLineArguments


Mark this internal.

davkean · 2017-03-29T22:31:13Z

...ft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/CommandLineArguments.cs

+
+namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices
+{
+    public class CommandLineArguments


You can't do this - your throwing away all options. TBH, I'm not sure how this is compiling - we pass this over to IWorkspaceContext as a CommandLineArguments.

I searched for all references to the Microsoft.CodeAnalysis.CommandLineArguments type and only the four properties reflected in this class were ever used.

I see, we used to pass this thing as SetOptions but now we pass a string. I stand corrected. If you're doing the work here to open this up, why do you still need to do this?

I need to expose the constructors so F# can create these types that are still used after this PR is merged. I originally looked at opening up the entire Microsoft.CodeAnalysis.CommandLineArguments type but that quickly snowballed (Koala-balled? I'm having trouble coming up with an Australian version of this 😄) into exposing way more stuff than we probably ever wanted too.

@brettfo I'm still confuised, if we're opening up CommandLineArgunents why we do need to wrap the type?

We're not opening up CommandLineArguments, because that quickly snowballed into opening up way more internal stuff than we'd like. I'm only opening up the constructors to the structs that this new type contains in its IEnumerable<> properties, and the properties on this class are the only things that were being used, anyways.

davkean

As above.

brettfo · 2017-03-30T18:10:06Z

Updated with review feedback as per @davkean.

This will enable non-Roslyn languages like F# to provide CommandLineOptions.

brettfo · 2017-04-04T21:17:33Z

Ping for review now that I've fixed the latest merge conflicts.

davkean · 2017-04-04T22:02:34Z

Okay, can we change the name of the type CommandLineArguments so that it doesn't conflict with the Code Analysis version and I can sign off.

brettfo · 2017-04-04T22:06:31Z

Given that the class doesn't have much to do with the command line any more I propose BuildOptions (maybe DesignTimeBuildOptions?) and I'll also rename IParseCommandLineArguments appropriately.

davkean · 2017-04-04T22:08:15Z

BuildOptions sounds good.

brettfo requested review from srivatsn and davkean March 28, 2017 20:35

dnfclas added the cla-already-signed label Mar 28, 2017

This was referenced Mar 28, 2017

Add F# support #1670

Merged

add IVT so F# can consume CommandLineParser dotnet/roslyn#18221

Closed

davkean reviewed Mar 29, 2017

View reviewed changes

davkean requested changes Mar 29, 2017

View reviewed changes

srivatsn reviewed Mar 29, 2017

View reviewed changes

davkean reviewed Mar 29, 2017

View reviewed changes

davkean requested changes Mar 29, 2017

View reviewed changes

brettfo added 3 commits April 4, 2017 13:49

remove direct dependency on Roslyn's CommandLineParser

52eb512

This will enable non-Roslyn languages like F# to provide CommandLineOptions.

export the command line parsers directly

cb9be74

retain proper unit test isolation

5d0de0c

rename CommandLineArguments to BuildOptions

5f720bb

davkean approved these changes Apr 4, 2017

View reviewed changes

brettfo merged commit 2e9680b into dotnet:master Apr 4, 2017

brettfo deleted the fsharp-support branch April 4, 2017 22:23

srivatsn mentioned this pull request May 25, 2017

F# language service integration #1853

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove direct dependency on CommandLineParser #1876

Remove direct dependency on CommandLineParser #1876

brettfo commented Mar 28, 2017

davkean Mar 29, 2017

davkean Mar 29, 2017

davkean commented Mar 29, 2017

davkean left a comment

brettfo commented Mar 29, 2017 •

edited

Loading

srivatsn commented Mar 29, 2017

srivatsn Mar 29, 2017

davkean Mar 29, 2017

davkean Mar 29, 2017

brettfo Mar 29, 2017

davkean Mar 29, 2017

davkean Mar 29, 2017

davkean Mar 29, 2017

davkean Mar 29, 2017

brettfo Mar 29, 2017

davkean Mar 30, 2017

brettfo Mar 30, 2017

davkean Mar 31, 2017

brettfo Mar 31, 2017

davkean left a comment

brettfo commented Mar 30, 2017

brettfo commented Apr 4, 2017

davkean commented Apr 4, 2017

brettfo commented Apr 4, 2017

davkean commented Apr 4, 2017

Remove direct dependency on CommandLineParser #1876

Remove direct dependency on CommandLineParser #1876

Conversation

brettfo commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean commented Mar 29, 2017

davkean left a comment

Choose a reason for hiding this comment

brettfo commented Mar 29, 2017 • edited Loading

srivatsn commented Mar 29, 2017

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

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

davkean left a comment

Choose a reason for hiding this comment

brettfo commented Mar 30, 2017

brettfo commented Apr 4, 2017

davkean commented Apr 4, 2017

brettfo commented Apr 4, 2017

davkean commented Apr 4, 2017

brettfo commented Mar 29, 2017 •

edited

Loading