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

Custom globals type support #1372

Open
CyberSinh opened this issue Dec 29, 2018 · 24 comments
Open

Custom globals type support #1372

CyberSinh opened this issue Dec 29, 2018 · 24 comments

Comments

@CyberSinh
Copy link

CyberSinh commented Dec 29, 2018

Hi,

Is there any preprocessor directives defined by OmniSharp ?

Here is the concept:

#if OMNISHARP
// the code would be read only by OmniSharp in VS Code
// this section would be used to provide class definition in CSX script files, 
// to allow OmniSharp to provide intellisense and other code editing assistance

// this section would be ignored by Roslyn at runtime as real classes and references 
// would be passed at CSharpScript.Create() with the ScriptOptions parameter, 
// or as 'globals' object at Script.RunAsync()
#endif

If not, is there an another way to detect if it is OmniSharp reading the code?

Thank you.

@filipw
Copy link
Member

filipw commented Jan 2, 2019

can you provide some concrete example of what you are trying to achieve? It looks a bit hacky at first glance, I don't think you should be using "fake" classes to replace a globals object. Instead, we are also going to add support for custom globals objects, this is tracked here #1171

I suspect what you can find helpful, is being able to define an RSP file to import namespaces/references (see #1112 and #1024).

Anyway, if you could please describe your use case, ideally with some example, we should be able to work out some good solution - thanks.

@CyberSinh
Copy link
Author

This is exactly what I was looking to do: import references to assemblies and provide the global type to omnisharp.

To reference assemblies, using the RSP file is indeed the cleanest solution. Thanks for the tip.

But I understand that the RSP file does not yet allow to pass the globals objects to omnisharp. Is there any workaround?

I would like to allow users to use Visual Studio Code to develop C# scripts to manipulate objects that will be provided by my app when it will run these scripts. And of course, without knowledge of the type of my globals object, omnisharp reports me error and code completion is not functional.

Thanks again.

@filipw
Copy link
Member

filipw commented Jan 2, 2019

Correct, globals types are not yet part of RSP files - it's actually not supported by Roslyn at the moment (here is the issue tracking that: dotnet/roslyn#23421)
At the moment there is really no workaround for OmniSharp to use a custom globals type.

However, as mentioned in #1171 it will be supported in OmniSharp as we'll add the support of using a custom globals type as a separate setting in omnisharp.json. The old OmniSharp was net451 application which made it a bit problematic, but since we moved to net461 it is now compatible with netstandard2.0 which makes it much more flexible for this globals type scenario (as the globals assembly must be loaded in process). I can bump the priority of this feature if this helps, it's been on my todo for a while 🙂

@CyberSinh
Copy link
Author

Yes, thank you very much for bumping up the priority of this feature. This feature is a must have for using omnisharp in scripting scenario. Do you know when this feature might be available? Is there any public roadmap for omnisharp?

In the meantime, as workaround, I'm thinking of adding the following (ugly) lines in my scripts:

// Uncomment the following line to get better coding experience with VS Code
// DO NOT forget to comment it before running the script into host
//IEnumerable<MyFile> files = new List<MyFile>(); // list of MyFile passed by the host as globals object at runtime

bool hasFiles = files.Any(); // no omnisharp error in VS Code if you uncomment previous line

Do not hesitate to tell me if you see any better workaround. Thanks again.

@CyberSinh
Copy link
Author

@filipw, is there roadmap for omnisharp somewhere? Thanks.

@filipw
Copy link
Member

filipw commented Jan 6, 2019

There is no explicit roadmap, but I think (hope) we can have it PR-ed this month. I will look closer next week to see how much work is needed.

@CyberSinh
Copy link
Author

Hi @filipw, do you have any news to share about this issue? Thanks.

@filipw
Copy link
Member

filipw commented Mar 13, 2019

I have a WIP branch here https://github.com/filipw/omnisharp-roslyn/tree/feature/customhost-scripting
It can be tested already if you are willing to clone the branch and build it yourself, otherwise I can provide you an omnisharp build to try it out with.

The usage is (via omnisharp.json:

{
    "script": {
        "GlobalsOptions": {
            "GlobalsTypeName": "ClassLibrary1.Foo",  // fully qualified name
            "GlobalsAssemblyPath": "C:/Users/filipw/source/repos/ClassLibrary1/ClassLibrary1/bin/Debug/netstandard2.0/ClassLibrary1.dll",  // assembly in which your globals type is located, must be net461 compatible / netstandard2.0
            "AdditionalAssemblyPaths": [ // any additional assembly dependencies your globals type needs, can be omitted if no dependencies are needed
                "C:/Users/filipw/source/repos/ClassLibrary1/ClassLibrary1/bin/Debug/netstandard2.0/ClassLibrary2.dll"
            ]
        }
    }
}

@CyberSinh
Copy link
Author

Thanks. Can we omit GlobalsAssemblyPath if the assembly is already defined via the RSP file?

@filipw
Copy link
Member

filipw commented Mar 13, 2019

Unfortunately this is currently a limitation of Roslyn.

Normally OmniSharp doesn't load your assemblies in-proc (and sometimes it wouldn't even be able to - for example .NET Core assemblies), but that's fine because this is not needed to provide language services, which are handled via MetadataReferences instead. However, globals type is a special case. Roslyn doesn't allow setting globals type via RSP and the only way a globals type can be set is using a Type reference on the Project API, meaning it must be loaded into the host process (meaning, OmniSharp in this case). This is also the reason for this limitation: // assembly in which your globals type is located, must be net461 compatible / netstandard2.0. And we don't know which of your RSP references may need to be loaded in proc (since it has a globals type) and which should stay as MetadataReference (since it's a compilation dependency only).

Hope it makes sense.

FWIW I have opened a request to allow globals type to be set via RSP file long time ago 😀dotnet/roslyn#23421 And that would make this experience better.

@CyberSinh
Copy link
Author

CyberSinh commented Mar 13, 2019

OK. Thanks.
Can we use generics in GlobalsTypeName (eg.: List<Foo>)?

@filipw
Copy link
Member

filipw commented Mar 13, 2019

yes you could use generics like this -

"GlobalsTypeName": "ClassLibrary1.Foo`1[ClassLibrary1.Baz]"

This is the standard syntax for expressing generics in .NET. However, it gets really complicated, really quickly, doesn't work across assemblies and doesn't really work with system types like List<Foo>. In that case you'd need to make your own class Bar : List<Foo> and use Bar as globals type.

The alternative approach would be to ditch this entire approach and provide an OmniSharp plugin way of doing this (which is something I am starting to lean towards now).
So this would look (hypotethically) like this:

public interface IScriptGlobalsProvider
{
     Type GetGlobalsType(ScriptContext context);  // context gives access to some metadata about scripts
}

Then you'd need to implement this provider and load it into OmniSharp as a plugin. This would give a lot more flexibility with types and assemblies to be used (however, everything would still need to be net461/netstandard2.0 to be loadable in proc by OmniSharp).

Would that approach work better for you?

@filipw filipw changed the title Preprocessor directives for OmniSharp Custom globals type support Mar 13, 2019
@CyberSinh
Copy link
Author

I think the best way in my case is to wrap my real variable in a global non generic class, like:

public sealed class GlobalVariables // GlobalVariables MUST be public
{
   public readonly IEnumerable<AudioFile> files = AudioFileCollection.Instance.CheckedFiles;
}

Do you agree?

@filipw
Copy link
Member

filipw commented Mar 14, 2019

yep that would work for you in this case.
However, I am now thinking that the approach with "plugin" that I described above, would be more flexible, more stable and easier to support for us going forward, so I think this feature would probably be better implemented that way, rather than the way I have on my PoC branch.

That would be contingent on us merging #1360 first, and publishing OmniSharp nuget packages (they are currently not published), as these are pre-requisites for defining the plugin story.

@CyberSinh
Copy link
Author

I can understand the interest of providing a more powerful and flexible solution. But it seems essential to me to provide a solution that allows you to load globals object without having to deploy compiled code as a plugin to OmniSharp.
I would like to be able to open my git scripts repository (https://github.com/luminescence-software/scripts) in any non-customized instance of Visual Studio Code and be able to code without having to deal with variables related error messages. So, I would prefer a solution that relies only on text files that could be embedded in the scripts git repository itself.

@CyberSinh
Copy link
Author

Hi @filipw,
Do you think that your your branch https://github.com/filipw/omnisharp-roslyn/tree/feature/customhost-scripting will be merged into master soon?
Thanks.

@CyberSinh
Copy link
Author

Hi @filipw,
Do you have any news to share about this issue?
Thanks.

@germanftorres
Copy link

Hi everyone!

Is this still something the team is considering? As @CyberSinh was pointing, this feature would enable a lot of cool uses of C# scripts as the base for application configuration DSLs with a VS like coding experience for a broader audience, not necessarely programmers.

In this context, with C# syntax highlight and by running and vaildating the script before saving to the database, the DSL users can now get along more or less. But that doesn't compare to the kind of experience in terms of api discoverability and error feedback that OmniSharp would give to these "programmers by accident". This would also align pretty good with OmniSharp's vision of C# in every platform, editor, context...

Thanks for reviewing this feature again and for checking if the aforementioned limitations and stopers are still applying.

Germán

@filipw
Copy link
Member

filipw commented Nov 6, 2020

thanks, unfortunately this is on hold because of the current limitations of Roslyn (requiring the host object to be loaded into process vs being just a MetadataReference). We could support it but it would be limited, because OmniSharp at the moment could only load netstandard2.0/net472 assemblies. This would also be quite fragile e.g. when the host object type has depnednecies that conflict with OmniSharp in-process dependencies.

I believe VS doesn't support intellisense for custom host objects for the same reason.

@germanftorres
Copy link

germanftorres commented Nov 6, 2020

Thanks @filipw ,

It's a pity that this is somehow blocked.

The route I'm heading now is loading a "globals.csx" script where the ambient types are defined again just for the sake of being accesible to OmniSharp. At runtime the idea is to strip the `#load´ directive and provide the real environment to the script runner though the globals object.

// Load some globals script for OmniSharp to show intellisense
// This leads to code duplication: types are defined in the hosting app
// as well as in the OmniSharp script workspace
#load "globals.csx"

// Adjust low level options according to the SearchContext
ConfigureFlightSearch = (context, flightSearch) => {
    return flightSearch with {
        NumberOfRecommendations = 200,
        FareTypes = context.IsDomesticSearch? "Public": "PublicAndPrivate"
    };
};

The problem is code duplication as I need to define the types in both environments, the hosting application and the OmniSharp hosting solution. I have read something about source code generators in Roslyn. Do you think they would help to autogenerate the "globals.csx" from a reference to the types in the hosting application?

Thanks!!

Germán

@haacked
Copy link

haacked commented Aug 24, 2021

@filipw, the #load "globals.csx" approach that @germanftorres does is a nice workaround for the lack of a Globals Type support. It's not perfect, but since I can create an instance of my global type, Omnisharp doesn't have to create it in-proc.

However, it still requires that we have #load "globals.csx" at the beginning of the script. Is there a way to do that outside of the script file? Might be nice to have an option I can place in omnisharp.json.

{
  "script": {
     "enabled": true,
     "defaultTargetFramework": "net461",
     "enableScriptNuGetReferences": false,
    "includes": ["globals.csx"]
  }
}

The idea would be that every script in the includes array would be loaded at the beginning as if included by #load scriptname.

If there's no way to accomplish this today and you think this is a good idea, I'm happy to log an issue.

@filipw
Copy link
Member

filipw commented Aug 26, 2021

Yes that makes sense. It would be simplest if Roslyn supported completion for globals type via MetadataReferences, same as for everything else. We investigated allowing loading custom type into process, but that was very fragile and had so many edge cases that the time investment was just not worth it for a half-baked feature.

One possibility going forward, might be to allow custom script hosts to be loaded as plugins. Omnisharp has the capability of loading Omnisharp plugins via the startup args or from the config file - for example Razor support is the plugin that happens to be loaded by the C# extension by default, and it is not part of the core Omnisharp build.

Plugins are not very well documented (in fact, it's not documented at all) and they do not support scripting at the moment but that could be the way for allowing custom hosts to have their own globals in there. It would still require to load the globals type into process, but it would be more explicit - you opt into the plugin that you need to install.

All things considered, the workaround with a custom script file is a decent one, and I don't see any problems with allowing includes specified via the omnisharp.json file, and this should be easy enough to add.

@haacked
Copy link

haacked commented Aug 26, 2021

@filipw thanks! I'll log an issue.

@izanhzh
Copy link

izanhzh commented Oct 13, 2023

hi, is there any good news on this topic

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

No branches or pull requests

5 participants