Enable passing of arguments to the polyglot engine

[smarr]

This change allows to pass an array of strings to the polyglot engine and retrieve it in the environment object.

The goal is to provide a straight-forward way to pass implementation-level arguments, as the typically come from the command line to the PolyglotEngine and the languages.

At the moment, we need to jump through many many hoops.
To realize the same effect, in a language-specific way, it seems we need to do roughly the following:

1. we need some wrapper class to be able to get to the value later on in the language:

   public static class CmdArgWrapper<T> implements TruffleObject {
     private final T obj;
     public CmdArgWrapper(final T obj) {
       this.obj = obj;
     }
   
     public T getObject() {
       return obj;
     }
   
     @Override
     public ForeignAccess getForeignAccess() {
       throw new UnsupportedOperationException();
     }
   }

2. we need to pass the arguments to the engine. In the case of SOM, I need these arguments when creating the language context, because the context contains the object system, which depends on the source to be used. And, might also have instruments that need to be configured from the very start.

   String[] args = new String[] {"--kernel", "Kernel.som", "--instrument", "dyn-metrics"};
   Builder builder = PolyglotEngine.newBuilder();
   builder.globalSymbol("CMD_ARGS", new CmdArgWrapper<String[]>(args));
   PolyglotEngine vm = builder.build();

   I believe, the whole notion of global TruffleObjects does not apply here because these arguments are strictly at the implementation level and not language-level objects. But even if there are arguments that become visible to the language, it is the languages job to figure that out and do the necessary wrapping.

3. in the TruffleLanguage.createContext(...) method, I need to do this:

protected VM createContext(final Env env) {
    Object argsObj = env.importSymbol("CMD_ARGS");
    String[] args = null;
    if (argsObj instanceof CmdArgWrapper<?>) {
      @SuppressWarnings("unchecked")
      CmdArgWrapper<String[]> argsWrapper = (CmdArgWrapper<String[]>) argsObj;
      args = argsWrapper.getObject();
    }
    assert args != null;
  // ...

All this is non-trivial for a very basic use case that any practical language would want to support: take some parameters to control its initialization process/startup.

With the provided patch, all this shrinks to:

String[] args = new String[] {"--kernel", "Kernel.som", "--instrument", "dyn-metrics"};
Builder builder = PolyglotEngine.newBuilder();
builder.setArguments(args);
PolyglotEngine vm = builder.build();

protected VM createContext(final Env env) {
    String[] args = env.getArguments();
// ...

I hope this is compelling enough for inclusion. I know the API should be minimal, but this is a super basic use case, and while there might be static-state solutions that are shorter than this, I believe we should provide an API that allows users to do 'the right thing' easily.

I am not entirely sure about my patch thought. The whole code based involved here is extremely complex and rather hard to comprehend. I mean, it adds about 40 lines of code for something very very trivial: another field + getter. I did not expect that much change for this simple feature.

[eregon]

Is Env per-language?
If so, we just need a way to add the arguments to the proper Env I think.

[smarr]

The env object references a specific language yes. But the arguments API I added does not take that into account currently. I'll draft something that allows one to specify the language the arguments should belong to, as well.

[smarr]

Ehm, I don't see how that could be working. To be able to construct a global object, I need to have the language initialized, no?

So, I would propose:

Builder builder = PolyglotEngine.newBuilder();
builder.setArguments(Language1.class, args1);
builder.setArguments(Language2.class, args2);

And then, the following would provide me with the correct argsN:

protected VM createContext(final Env env) {
    String[] args = env.getArguments();

I need the arguments in createContext(.), and I don't want to hack around it.

[smarr]

This is now implemented in the last commit.

[jtulach]

Alas, I don't think people should reference the implementation class of a language. They certainly don't have to do so now. Probably the language should be identified by a MIME type.

[smarr]

Ok, used the MIME type. What's the reason for this? Potential class loading issues or something? Intuitively, I would assume that this kind of interaction still happens in tightly bound places, like a main method or something similar. You envision a more dynamic and modular use case?

[woess]

Isn't the actual problem here that you can't get global symbol values back out without wrapper?

[smarr]

I think there are two separate concepts here:

1. Such arguments/parameters as I propose to introduce are on the implementation level, this is data a language runtime might need for initialization.
2. Global Symbols/Values are language-level objects that are used by an application. As such, I agree with the current design. Actually, I even consider the whole notion of unboxing on those value problematic (for primitives there is a use cases yes). Generally, this is something a specific language knows how to handle and deal with. This is for applications.

So, I would argue we need this second way for implementation-level purpose only to pass data to a language implementation. Unifying these two ways does not seem the right solution for me, because this weakens the nice semantic properties we got for interop.

[woess]

@smarr: Good point. I was thinking that perhaps one might want to pass something else than a String[] which is already possible with a global symbol; but I agree it's not the right tool.

[smarr]

hm, yeah, I was only briefly thinking of other things than strings. Perhaps it should be just Object instead of String[]? My current use case is really typical command line arguments. But, other languages also need access to environment variables from the host system. So, perhaps this should be more general.

[jtulach]

Right, this is the kind of discussion that lead me to believe there shouldn't be any generic setArguments. The languages may have different needs and it should be up to them to decide what data and in what format they want.

I've created #11 to demonstrate how (simple) it feel to access array of parameters in current API.

[smarr]

Ok, well, so there is at least one lesson we should learn from this. The current API is absolutely not discoverable, and the implementation is way to involved to get any idea of intentions. I am really suffering over here :(

In your version #11, making the jump from TruffleObject to JavaInterop.asJavaObject(.) is just something that is not discoverable. Not sure how much documentation is going to do about that.

[jtulach]

I have a bug in codesnippets that I will fix on Monday. Then we will see...

Right. The trick isn't easy to discover and that's the reason it took me so long and I had to test it first.

[smarr]

Taking Andreas' comment into account, I generalized the String[] to an Object. Benefits of this vs. the Java Interop based approach:

• does not require reflection
• is a separate mechanism for implementation-level data
• might be easer to find for new users

[jtulach]

Object == the JVM style of typing things. I don't like it.

I've closed the #10 request, because I realized that in this case we don't want to wrap Java types into TruffleObject. Thus we need different method. Rather than setArguments, I'd however prefer:

Builder Builder.config(String mimeType, String key, Object value);
Map<String,Object> Env.getConfig();

I think this would work for your usecase as well and gives languages a chance to expose just names, not real objects.

[smarr]

@jtulach I added the change to have the API structure like this:

Builder Builder.config(String mimeType, String key, Object value);
Map<String,Object> Env.getConfig(); // can be called from createContext(), value is not wrapped

At the other languages (@woess, @ansalond, @eregon, @chrisseaton, @lukasstadler, @chumer), does this look useful/sufficient to you?

[jtulach]

We should improve our workflow. I've just added implementation of my request too: jtulach@32d4240

[jtulach]

Isn't this broken? I am getting exceptions from here. I suggest to use my version of the test - it uses just public methods.

[smarr]

ok, will change the test. Do you have any strong opinions on the Map<> vs. List<> difference between my and your implementation?

[smarr]

Hm, ok, since I started the pull request, I assumed it to be my responsibility to fix the issues.

[jtulach]

I don't think there is any use for two levels of the map. Just merge values for all mimetypes of a language together into one map.

[smarr]

For documentation: some discussion on the relationship between MIME types and config options: jtulach@32d4240#commitcomment-15653479

[smarr]

Ok, the latest change merges the per-mime-type options to per-language options from the view of the languages.

Anything else left to do before a merge can be done? (at some point, I'll rebase the change set again on master)

[jtulach]

If you could use the codesnippet tag to enrich Javadoc of the two new methods with real sample, it would be great. Then I am ready to merge.

[smarr]

Hm, perhaps this is me being lazy, but code examples for getter/setter methods? I mean, the javadoc references the two elements that belong together, which is very helpful. Don't know but code example feel too much here.

[jtulach]

OK. I'll do it myself and merge.

[jtulach]

Here is the state I am OK with: jtulach@f4756bf - can you pull the changeset and push it into your repository so we can reuse this pull request or shall I start a new one?