Language Server API

[farmaazon]

When I was discussion with @Frizi the issue #6715, we discovered several points about Language Server API to be discussed broader @4e6 @hubertp :

What widgets needs to know

The widgets need to know how what method is called in the node, and also what arguments are already applied. Currently, It uses the information from method_pointer field in expression updates. It works for most cases, but has some issues:

1. Information about the self argument only may be deduced from method_pointer. We cannot handle situation where more arguments were applied, for example in this graph:
   
   The func1 is taking two arguments, so in the node on the left-bottom we want to see a single widget.
2. Moreover, the fact that MethodPointer differs depending on the call syntax does not fit its definition in the protocol. See the section below.
3. MethodPointer is not always reported. To be more specific, it's only reported when the method name appears in the node, or all arguments are applied.

Therefore, we need some sort of call_info in expression updates, which for given expression it would specify what method it is (taking method_call field from expression update) and what arguments are already applied. @Frizi please correct me if I'm missing something.

MethodPointer and .type suffix.

The MethodPointer, according to its documentation in https://github.com/enso-org/enso/blob/102ce7ae5691f9aaceb43226600515af73d78424/docs/language-server/protocol-language-server.md:

Points to a method definition.

Its field is called definedOnType, but as @Frizi mentioned in his comment it's value depends actually on how the method is called instead how it is defined. I think this should be fixed.

But now an important question emerges: shall static method of local.Unnamed.Foo be reported as defined in local.Unnamed.Foo or in local.Unnamed.Foo.type?

The latter is very tempting, but then:

1. The Suggestion Database should also report statics as defined on .type, for consistency
2. Similarly, the search/completion should understand the self_type argument with .type suffix as asking for the appropriate statics methods. Currently, it does not return them (it only returns methods of Any).
3. Given point 1, the is_static field could be removed as not needed anymore. It would only make IDE live a bit harder, as we would need to handle modules in a special way (as modules, in turn, does not have .type suffix, what obviously make sense from the language perspective).

@4e6 @hubertp what do you think about it?

This problem is rather a low priority, as so far the issues caused by it are in obscure scenarios. Also, whatever decision will be made, we need to make sure that the problems in the previous sections will be addressed first.

Solidifying API

From all the above, I think one important side note must be taken.

Given the approaching release, I think we are at the last chance of making significant and breaking changes in the Language Server. After the design partner or beta release, we should add new things sparingly and possibly only backward-compatible. Therefore, I think we should at some point think about the API and fix all inconsistencies and perhaps rename things: for example, the suggestion database is no longer about suggestions only.

I will try to look through it and post more propositions here.

[4e6]

I looked into this issue today. @farmaazon thank you for the detailed description, it was very helpful in the investigation.

I agree that the issue is that the Suggestions database does not add .type suffix to the types, and maintains the is_static field instead. This creates inconsistencies in runtime vs. suggestions types.

I created task #6800 for myself which should fix that.

[Frizi]

@4e6 I don't think that this solves the problem. It is possible to call non-static method directly on a type (e.g. Text.trim "a"), which creates a situation where the reported method pointer doesn't match any entry in the suggestion database. See #6715 (comment) for more details.

[4e6]

Good point. I need to check what is happening in the engine. I guess, the fix for the Text.trim "a" case would be

• to make that every non-static method can be invoked with a static definedOnType (with .type suffix)
• to make the suggestions database return both static and non-static entries for the provided self_type that ends with .type suffix

I need to experiment with that a bit

[farmaazon]

Regarding "reported method pointer" the solution is as I said: it should not depend on call syntax. See my original comment.

Returning static and non-static entries is a needless duplication imo.

[4e6]

We had a discussion today during the engine daily sync and decided that the .type suffix is an internal implementation detail and should be hidden from the user.
And instead of propagating .type into the suggestions database, we should eliminate it from the method pointer info provided by the runtime.

[farmaazon]

I have one question: if I create node Table, what type will be reported? Before it was Standard.Table.Table.type (or something similar), what make it distinct from instance of Table.

[4e6]

Clearing the .type suffix from the method pointer fixed the dynamic dropdowns. I.e. the encoding argument of Text.from_bytes reported in the original issue #6715 now has the dropdown.

Although it caused one issue with Text.trim. When the trim method is called in the "static" notation, IDE thinks that the dropdown is the first argument. I assume this case has some special handling in IDE, and just needs to be updated as well.

[JaroslavTulach]

runtime knows operator8 has what pre-applied.

Of course it knows! I was asked to debug #6903 last Thursday and (while I was lost for almost whole day) this is what I believe in right now. TL;DR: The ProgramExecutionSupport.scala shall do following when composing sendExpressionUpdate:

• when the value.getType is Standard.Builtins.Main.Function
• accompany the information with a MethodPointer to the function definition
• use (value.value : runtime.Function).getSchema() to obtain hasPreApplied array and send it to the IDE somehow

The IDE shall be modified to use this "function schema" information associated with a value. Does that sound OKeyish, @Frizi?

[JaroslavTulach]

extend the expression info to not only contain method pointer, but also a list of arguments that are expected at the call site.

When is the expression info delivered? Only when the call site is executed? Then that's too late! In my opinion you need this information when you obtain operator8 value, without making any calls on that value.

For Text.trim, that would be ['self', 'where', 'what'], and for '-a-'.trim it would be['where', 'what'].

That's correct.

FunctionSchema has this kind of information and I believe the only thing we need is to deliver this information (about a value) to the IDE.

... use the argument indices (e.g. [0, 1, 2]) to ... important to keep that consistent with the argument lists provided within the suggestions.

OK.

[Frizi]

When is the expression info delivered?

It is delivered immediately after the IDE sends a file update to the language server. But initially it is not a very useful update:

{
  "expressionId": "fbb2a02d-6345-472c-8fa0-f5877a29bdd9",
  "type": null,
  "methodPointer": null,
  "profilingInfo": [],
  "fromCache": true,
  "payload": {
    "type": "Pending",
    "message": null,
    "progress": null
  }
},

Only after the expression is evaluated, we actually receive methodPointer as a non-null value:

{
  "expressionId": "fbb2a02d-6345-472c-8fa0-f5877a29bdd9",
  "type": "Standard.Base.Function.Function",
  "methodPointer": {
    "module": "Standard.Base.Data.Text.Extensions",
    "definedOnType": "Standard.Base.Data.Text.Text.type",
    "name": "trim"
  },
  "profilingInfo": [
    {
      "ExecutionTime": {
        "nanoTime": 288300
      }
    }
  ],
  "fromCache": false,
  "payload": {
    "type": "Value",
    "warnings": null
  }
},

Only when the call site is executed? Then that's too late! In my opinion you need this information when you obtain operator8 value, without making any calls on that value.

I absolutely agree. If you are able to perform static analysis and fill methodPointer (or a function schema for that matter) before actual execution, the IDE should be ready to accept it. The argument placeholders and other widgets would show up quicker. I think that this is also necessary to provide any completion for method calls within lambda expressions, as currently those don't receive any meaningful updates at all. Being able to receive something without actual exeuction would allow us to present widgets there.

Note that his behavior is also a cause of some issues during graph reevaluation, where new Pending updates are setting methodPointers to null, causing widgets and arguments to disappear for a moment. Fixing those expression updates would prevent IDE from doing those unfortunate "back and forth" view changes in response to that.

[JaroslavTulach]

I absolutely agree. If you are able to perform static analysis and fill methodPointer (or a function schema for that matter) before actual execution,

Yes, we can get all the info about applied and unapplied arguments when we obtain the operator8 functional value from the execution previous statements.

the IDE should be ready to accept it. The argument placeholders and other widgets would show up quicker.

If this "mental experiment" yields a performance improvement, then it is going to be an amazing success!

[JaroslavTulach]

I reported #6957 to plan and track the progress.

[JaroslavTulach]

Given the approaching release, ... we should add new things sparingly and possibly only backward-compatible.

That's always good idea. However...

last chance of making significant ... changes in the Language Server

Why? Is the engine and IDE distributed separately?

By downloading an .appimage, .dmg or .exe one downloads both the IDE as well as LS+engine. In such setup there is no need to invest in some protocol version negotiation as the protocol version is given by the version of LS+engine and IDE.

The cloud+browser setup has similar behavior. The cloud shall associate a particular combo of IDE and LS+engine with the project and always make sure the browser loads the right IDE for the engine running in the docker container.

The only problematic case is cloud+desktop...

new things ... only backward-compatible

...we cannot easily give the user the right IDE for the project in the cloud. We can just warn the user that there is a mismatch... We will need some form of semantic versioning to define what is and what isn't a mismatch. Being compatible will of course help.

[Frizi]

Why? Is the engine and IDE distributed separately?

No, but you may have a project that was created with older engine version. That version is stored within the project metadata. If we want to be able to open that project with updated IDE, it must support communication with older engine.

We could implement a form of semanting versioning for engine, such that the IDE is able to tell if it is compatible with given engine distribution (the IDE maintains the "major" version range of engines it supports, the engine bumps major whenever it introduces a hard breaking change). But that still means that we need to be careful about such breaking changes, as we don't want to make the updates unnecessarily hard for the end users.

The cloud+browser setup has similar behavior. The cloud shall associate a particular combo of IDE and LS+engine with the project and always make sure the browser loads the right IDE for the engine running in the docker container.

As far as I'm aware, the dashboard itself is a part of the IDE distribution. I don't think it has the capability to reload to any arbitrary IDE distribution after you select the project. Though it is a neat idea that could be considered.

[farmaazon]

// EDIT lost race with @Frizi, so some of the below are doubled with his post.

By downloading an .appimage, .dmg or .exe one downloads both the IDE as well as LS+engine. In such setup there is no need to invest in some protocol version negotiation as the protocol version is given by the version of LS+engine and IDE.

But every project has engine version specified (to avoid a problem of "upgrading IDE has broken my project"). So opening old project still downloads old engine and run old language server. Now we display "Unsupported version..." warning, but we want to be better than that.

The cloud+browser setup has similar behavior. The cloud shall associate a particular combo of IDE and LS+engine with the project and always make sure the browser loads the right IDE for the engine running in the docker container.

That's true. This is the only case, however.

...we cannot easily give the user the right IDE for the project in the cloud. We can just warn the user that there is a mismatch... We will need some form of semantic versioning to define what is and what isn't a mismatch. Being compatible will of course help.

So ok, maybe it's not the last chance. But still it should be avoided as long as possible.

@mwu-tow on this occasion I want to complain about our current versioning, which allows breaking changes only on the first version after New Year.

[JaroslavTulach]

opening old project still downloads old engine and run old language server.
...
dashboard itself is a part of the IDE distribution. I don't think it has the capability to reload to any arbitrary IDE distribution after you select the project. Though it is a neat idea that could be considered.

To play devil's advocate: Can't we also download suitable IDE for that LS+engine version? That would give us the "Internet Explorer compatibility" (there used to be a menu with choices to view a project as IE 7, 8, 9, 10, 11...).

semantic versioning for engine, such that the IDE is able to tell if it is compatible with given engine distribution ... we need to be careful about such breaking changes, as we don't want to make the updates unnecessarily hard for the end users.

+1 - NetBeans project had always been firm when it came to compatibility. I certainly prefer "compatible" approach to releases. However that is not going to happen by itself. It will take a time to make the process working smoothly. 1st question that puzzles my mind:

How are we going to test that IDE works with old LS+engine version? Certainly not manually - the amount of IDE vs. LS+engine combinations would easily grow out of hand. Probably we need (at least some) automated test suite...

... complain about our current versioning, which allows breaking changes only on the first version after New Year.

There is a difference between marketing and engineering versions. @mwu-tow date-based version is just an end user label and has nothing to do with compatibility. As the article suggests, we need at least to version the language server protocol and make sure the versioning makes sense from semantic perspective.

[farmaazon]

Actually I was was wrong for "cloud + desktop IDE" case: #6899 (comment).

To play devil's advocate: Can't we also download suitable IDE for that LS+engine version? That would give us the "Internet Explorer compatibility" (there used to be a menu with choices to view a project as IE 7, 8, 9, 10, 11...).

Yes, we could, and we even have to according to @wdanilo's comment #6899 (comment)

However that is not going to happen by itself.

I do agree, I just wanted to make simple steps first, ensuring for some time we try to avoid "hard-remove fields" (making them optional) and don't change their semantic significantly for some time, until the process settled.

Anyway, given what was written in the comments linked above, I think it's not needed. But I'm a bit confused. As I remember, two years ago or so, the ability to open old projects in a newer IDE was a requirement.

[wdanilo]

Ok, so the important thing here is this – if we COULD somehow guarantee that we will be progressing with IDE development and make it work with older Engine versions, then yes, that would be ideal. I just have seen so many problems with incompatible IDE/Engine versions that I stopped believing it is possible in near future. However, if it is possible and we can agree that "starting with IDE version X we will make sure that IDE works with old versions of engine up to Y" and we will maintain this invariant, then yes, our live would be so much easier both on local and cloud distributions, as we would be able to use always the newest IDE version even with older projects and older engines. Is it possible to establish such an invariant? If yes, let's do it and it will solve al ot of issues in this discussion.