Scripting revamp #659

Merged
merged 11 commits into from Nov 24, 2016

Projects

None yet

4 participants

@filipw
Member
filipw commented Nov 4, 2016 edited

This PR is an attempt to revamp/normalize the C# scripting support in Omnisharp.

Here is what is done:

  • removed all dependencies on scriptcs - based on Roslyn directly. Omnisharp makes no assumptions about "scripting dialect" any more. It tries to be as close to "standard" as possible. Also renamed things like ScriptCsProjectContext to ScriptProjectContext etc etc
  • scripting intellisense is now compatible with InteractiveScriptGlobals - same as used by csi.exe. This gives us same set of global properties as there, i.e. Args.
  • scripting is (for now) supported only as .NET Core scripting update: you can specify net46 target in project.json or use no project.json at all to force "net46 scripting mode". If your project.json contains a target for netcoreapp1.0, then .NET Core language services will be used
  • dependencies for scripts are no longer defined by the old packages.config, instead they are pulled from project.json
  • no implicit using statements or assembly references are made - except for System namespace, mscorlib and System.Runtime (if needed).
  • suppress a couple of code check warnings which are not applicable to scripting code - CS8099 & CS1701

This PR is in line with my proposal here OmniSharp/omnisharp-vscode#23 (comment)

This is a breaking change of course for scripting story in Omnisharp, but I believe it was never really officially supported but more of an experimental stuff. I believe trying to normalize to "pure" Roslyn scripting is a step in a right direction here, especially as we can easily align with runners like csi.exe then or dotnet-script

@filipw
Member
filipw commented Nov 4, 2016

This PR also - from the scripting perspective - removes Omnisharp's dependency on full desktop framework

@david-driscoll
Member

I am in favor of this change! Will see if I have time to look at it before summit... if not we'll sit down at summit and talk about it :)

- IOmnisharpEnvironment Env { get; }
- ScriptCsContext Context { get; }
- ILogger Logger { get; }
+ private CSharpParseOptions CsxParseOptions { get; } = new CSharpParseOptions(LanguageVersion.CSharp6, DocumentationMode.Parse, SourceCodeKind.Script);
@DustinCampbell
DustinCampbell Nov 10, 2016 Contributor

Use LanguageVersion.Default to ensure that the latest version of the language is used?

@DustinCampbell
Contributor

Thanks for making this change @filipw! I definitely agree it's a step in the right direction.

- var localScriptServices = baseBuilder.ScriptName(csxPath).Build();
- var processResult = localScriptServices.FilePreProcessor.ProcessFile(csxPath);
+
+ var fileParser = new FileParser(Context.RootPath);
@DustinCampbell
DustinCampbell Nov 10, 2016 Contributor

Any reason not to just parse the file with Roslyn?

@filipw
filipw Nov 10, 2016 Member

to be honest I would love to, and I tried but couldn't find the right APIs. In fact - unless I am mistaken - the necessary APIs (this?) are not available publicly. What we need here is to walk the dependency tree of #load directives and gather all the files and their using statements and so on so that we can construct ProjectInfo correctly. It seems the only way to publicly get there with Roslyn scripting APIs at the moment is to manually create a script compilation which I wanted to avoid

@DustinCampbell
DustinCampbell Nov 10, 2016 Contributor

This can be done syntactically, correct? If so, SyntaxTree.ParseText(...) is what you're looking for.

@filipw
filipw Nov 10, 2016 Member

yeah. The only "hurdle" is if I encounter a #load foo.csx directive, I need to follow it and pick up the foo.csx and that one might have it's own #loads and so on.

Scripting APIs would use this to resolve that whole dependency tree so maybe that would work, to parse the entry file and then follow it's #loads using ScriptSourceResolver recursively.

@filipw
filipw Nov 10, 2016 Member

but that would be better than manual parsing, definitely

@DustinCampbell
DustinCampbell Nov 10, 2016 Contributor

I guess I'm not seeing that as much of a hurdle. You could still have the file parser but use the SyntaxTree to find the using's, #load's, and #r's. You could keep the logic where it continues processing #load's. The real issue is that the simple text "parsing" isn't going to do all that good of a job. For example, it looks like it would return a weird malformed using if it countered a using static.

@filipw
filipw Nov 10, 2016 Member

Right I see what you mean now. Of course, let me pluck the naive syntax parsing 👍

@filipw
Member
filipw commented Nov 10, 2016

Thanks for making this change @filipw! I definitely agree it's a step in the right direction.

great! I also had a chat with Tomas yesterday and he suggested that it might be a better idea to treat all CSX files as single project, rather than each of them as a separate 1-file project (which is how it is implemented at the moment). I will try to incorporate this change too.

@DustinCampbell
Contributor

How is this going @filipw?

@filipw
Member
filipw commented Nov 23, 2016

It's coming!

On Nov 23, 2016 5:58 AM, "Dustin Campbell" notifications@github.com wrote:

How is this going @filipw https://github.com/filipw?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#659 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABoZIeceDUScLBLqXA2uRCUKES0XQDZFks5rA8gPgaJpZM4KpkTV
.

@filipw
Member
filipw commented Nov 24, 2016

I've rebased this and introduced better parsing of files (no more naive parser, instead use SyntaxTrees) as suggested. Also improved some of the overall logic.

Tested in the following scenarios:

  • .net core single-file CSX script with nuget references in project.json
  • .net core multi-file CSX script with nuget references in project.json
  • accessing script args using globals from InteractiveScriptGlobals (same global object as in csi.exe)
  • .net desktop CSX script with nuget references in project.json and extra GAC references in project.json
  • .net desktop CSX script with no nuget references (everything self contained via #r)

This is still a simple approach to the problem, but it's definitely an improvement over what's there at the moment. My suggestion would be to go ahead with this merge and I can progressively make this better as we go forward (for example I'd like to include this in the future, though behaviorally things are OK already).

Finally, this solution works best on net46 build of OmniSharp because when we are in netcoreapp1.0 it's impossible to i.e. infer some of the assemblies without running into a lot of problems with object has not been imported. However my understanding is that net46 will be the default mode of distributing OmniSharp

@DustinCampbell

Looking good! I had just a couple of questions.

src/OmniSharp.Script/FileParser.cs
+ var scriptCode = File.ReadAllText(fullPath);
+
+ var syntaxTree = CSharpSyntaxTree.ParseText(scriptCode, CSharpParseOptions.Default.
+ WithPreprocessorSymbols("load", "r").
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Is this line necessary? This would have the effect of implicitly adding the following code at that top of the tree:

#define load
#define r

Is that what you meant?

@filipw
filipw Nov 24, 2016 Member

ah my mistake I thought I needed to use this to make the parser be aware of #load and #r as valid code. will remove

+ var loads = syntaxTree.GetCompilationUnitRoot().GetLoadDirectives().Select(x => x.File.ToString());
+ foreach (var load in loads)
+ {
+ var filePath = load.Replace("\"", string.Empty);
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Does this work on OSX/Linux?

@filipw
filipw Nov 24, 2016 edited Member

yes, this just strips away the quotes, tested on Mac too. when I have i.e. #load "bar.csx" in my CSX and I use GetXXXDirectives() method on the compilation root, the result string comes back with quotes inside i.e."bar.csx" not bar.csx and I want to strip it away to be able to concat the relative path and traverse these resources

@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Oh sorry. I misread the slash in there. :-) I glanced at it and read the slash, not the double-quote. 😄

src/OmniSharp.Script/FileParserResult.cs
+ LoadedScripts = new HashSet<string>();
+ }
+
+ public HashSet<string> Namespaces { get; }
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

FYI that read-only auto-props can have initializers. So you could just have this with no constructor:

public HashSet<string> Namespaces { get; } = new HashSet<string>();
public HashSet<string> References { get; } = new HashSet<string>();
public HashSet<string> LoadedScripts { get; } = new HashSet<string>();

Your choice.

-namespace OmniSharp.ScriptCs
+namespace OmniSharp.Script
{
[Export(typeof(IProjectSystem))]
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Add Shared attribute so that MEF creates this as a singleton.

- var scriptServices = baseScriptServicesBuilder.Build();
+ var runtimeContexts = File.Exists(Path.Combine(Env.Path, "project.json")) ? ProjectContext.CreateContextForEachTarget(Env.Path) : null;
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

If I understand what this is doing, it'll probably need to change in the future for csproj .NET Core. I might not understand though. 😄

@filipw
filipw Nov 24, 2016 Member

yes correct I will want to change it in the future. There is no real official project model for scripting so at the moment the idea is that if you have a project.json it will be used to determine your dependencies and we can load them in for you to provide language services. We will in the future switch to csproj so that you can have <packagereference> there.

In all other cases, you get a no-frills service with mscorlib/system.runtime only and all your needed assemblies must be #r-ed manually by you

@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Sounds good.

+ // if we have no context, then we also have no dependencies
+ // we can assume desktop framework
+ // and add mscorlib
+ if (runtimeContexts == null || !runtimeContexts.Any())
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

if (runtimeContexts?.Any() == false)

- Context.ScriptPacks.UnionWith(scriptPackSession.Contexts.Select(pack => pack.GetType().ToString()));
+ // inject all inherited assemblies
+//#if NET46
@DustinCampbell
DustinCampbell Nov 24, 2016 Contributor

Can this be deleted?

filipw added some commits Nov 2, 2016
@filipw filipw remove scriptcs b886ff5
@filipw filipw folder rename ba285b8
@filipw filipw scripting WIP a5dbed5
@filipw filipw updated file parsing logic e5c92d5
@filipw filipw general clean up 7d4c7e4
@filipw filipw added scriptcs attribution 83ba62f
@filipw filipw better file parsing and overall logic 46e88d7
@filipw filipw some extra scripting reference logic 7df0560
@filipw filipw updated to roslyn 2.0.0-rc 5f22e94
@filipw filipw applied code review feedback
11f9e10
@DustinCampbell

Looks great with your latest changes.

@filipw filipw fixed build
9c45a68
@filipw
Member
filipw commented Nov 24, 2016

@DustinCampbell everything should be OK now, all the review feedback applied - thanks!

@filipw
Member
filipw commented Nov 24, 2016

@DustinCampbell
Contributor

Merging! 😄

@DustinCampbell DustinCampbell merged commit db3478f into OmniSharp:dev Nov 24, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@filipw
Member
filipw commented Nov 24, 2016

yay - thanks 😄

@filipw filipw deleted the filipw:feature/scripting-revamp branch Nov 24, 2016
@gpuido
gpuido commented Jan 8, 2017

Sounds great !

what should I put in project.json to get started with something simple like the project in your gif ?

#659 (comment)

@filipw
Member
filipw commented Jan 8, 2017

what should I put in project.json to get started with something simple like the project in your gif ?

If you use CSI to execute your scripts, the project.json can be empty, just {} (it's only needed to "light up" OmniSharp). You will be responsible for bringing in any assembly references you need using #r directives. This is the regular CSI experience.

If you use a .NET Core script runner (CSI is not .NET Core compatible at the moment) like https://github.com/filipw/dotnet-script, then you can put any package references you need into the project.json too.

@gpuido
gpuido commented Jan 8, 2017

Thanks a lot filipw,

I got some of the frustation out with dotnet-script running .NET Core
but I'm on win10 and I'd like to target dotnet4.6, is this possible ?

CSI is not .NET Core compatible at the moment

So the 'regular CSI experience' is running on regular .net46 ?
because when I hit F5 vscode ask me to select an environment but only offer to select .NET Core.

@DustinCampbell
Contributor

If I'm understanding you correctly, the platforms that the debugger in VS Code supports (which is just CoreCLR) is completely orthogonal to what environment your scripts run in.

@filipw
Member
filipw commented Jan 9, 2017

I got some of the frustation out with dotnet-script running .NET Core
but I'm on win10 and I'd like to target dotnet4.6, is this possible ?

no, at the moment that runner only exists to run .NET Core scripts.
In fact, it was built only to bridge the current gap, because CSI doesn't support .NET Core

So the 'regular CSI experience' is running on regular .net46 ?

correct, CSI is for running scripts as net46 only. It runs on mono (with some hiccups) too.

because when I hit F5 vscode ask me to select an environment but only offer to select .NET Core.

yep, as Dustin said, the F5 debugging in VS Code only works with CoreCLR, so you can use it to work with scripts running with dotnet-script, but not running with CSI.

@gpuido
gpuido commented Jan 9, 2017

Got it now, Thanks guys.

This attempt to normalize the C# scripting support in Omnisharp is much welcomed.

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