add ProgressToken constructor

[ZacLN]

fixes #651 ?

[jipolanco]

It seems to work. But now that I think about it, isn't this type piracy? Since ProgressToken is just an alias for Union{Int, String}...

[ZacLN]

Yeah possibly, but who really owns a Union?

[jipolanco]

I'd say that, since we don't own any of the types in the Union, then we don't own the Union.

If we have the right to define a constructor for the Union, then any other module has the right to do the same, which is (as I understand it) what the concept of type piracy tries to avoid.

More concretely, this implementation could break if someone else decides to define their own constructor for the same Union.

[davidanthoff]

Yeah, this seems kind of risky....

[ZacLN]

Probably right - I'll fix and add a check in StaticLint

[gdkrmr]

why not simply do progresstoken(x) = x instead of ProgressToken(x) = x, not super pretty, but avoids Type piracy. You could also make a struct:

struct ProgressToken
    token::Union{String, Int}
end

[matsievskiysv]

Sorry for jumping in, but ProgressToken is used as a constructor only once in src/protocol/initialize.jl and it's not needed there. This will work just fine:

-    haskey(dict, "workDoneToken") ? ProgressToken(dict["workDoneToken"]) : missing)
+    haskey(dict, "workDoneToken") ? dict["workDoneToken"] : missing)

Also, there's a struct ProgressParams which was probably designed to hold this value but isn't used anywhere.

[ZacLN]

Sorry, I've not had time to have a proper look at this yet.

I'm pretty sure ProgressToken could be used during the construction of any struct defined with the @dict_readable macro (these represent possible incoming messages as part of the protocol).

[matsievskiysv]

I'm pretty sure ProgressToken could be used during the construction of any struct defined with the @dict_readable macro (these represent possible incoming messages as part of the protocol).

I've grepped project and found only this instance.
I also compiled LanguageServer with the patch above and it worked like a charm.

[ZacLN]

The macro mentioned above creates, for each struct it is used on, functions that convert a Dict to that struct where keys of the Dict match the fields of the struct. The datatype specified for each field is called is called on the matching Dict value, e.g.:

macroexpand(LanguageServer, :(
@dict_readable struct T 
    d::ProgressToken 
end))
-->
struct T
    d::ProgressToken
end
function T(dict::Dict)
    T(ProgressToken(dict["d"]))
end

So it's not only explicit calls to ProgressTokens constructor that need to be adjusted, but the whole body of auto-genned code.

Your fix above will work for now, but as soon as we start getting sent ProgressTokens in other contexts it will break. It's better to fix it broadly.

[gdkrmr]

The macro mentioned above creates, for each struct it is used on, functions that convert a Dict to that struct where keys of the Dict match the fields of the struct. The datatype specified for each field is called is called on the matching Dict value, e.g.:

Then you really should use

struct ProgressToken
    token::Union{String, Int}
end

[davidanthoff]

What is the status with this? Should we first merge #701 and then revisit this?

[ghost]

I just thought I'd mention I managed to get lsp-julia working without a hiccup by dropping in the progtoken branch and then editing InitializeParams as suggested. The progtoken branch did not work by itself. I'm on mac running 1.4.2. Thanks to the guys that solved this, it was driving me crazy.

[ordicker]

Could you share your init.el?

[ghost]

My lsp-mode is installed manually because I was trying different versions when I was trying to get lsp-julia to work but I don't see why an elpa installed lsp-mode wouldn't work. Also, I had to edit lsp-julia.el to use the new LanguageServerInstance() syntax. That was throwing errors for me too.

my init.el

[aviatesk]

@julia-vscode/core bump ? (I got a report on this today in other place) I guess it's okay if we pirate the Union{Int,String} constructor since LanguageServer.jl isn't supposed to interact with user code ?

[non-Jedi]

I think piracy is fine in an application, but I'd really prefer we continue thinking of LanguageServer.jl as a library/package so that people could use parts of it to build their own code analysis tools if desired. That said, it's unlikely to happen, so if the right fix for this is onerously difficult (involves special-casing a macro? I haven't dug in to see what the best fix would be), creating a method for a Union{Int,String} probably won't break things.

[aviatesk]

creating a method for a Union{Int,String} probably won't break things.

na, (::Type{Union{Int,String}})(x) = x is essentially same as Union{Int,String}(x) = x and so it pirates things.

[pfitzseb]

So what's the reason for not making ProgressToken a struct again?

[aviatesk]

Guess we want to treat it as a value ?

pps = ProgressParams(...)
pps.token # want to get the actual value here ?

And overloading getproperty for every parent structs who own ProgressToken will be verbose.

[pfitzseb]

Yeah, not sure how/in what contexts ProgressToken is used and whether defining a bunch of convert methods wouldn't be enough. I can take a more thorough at some point though.

[ZacLN]

If this instance of pirating is deemed unacceptable can we simply replace all fields that are typed by ::ProgressToken with ::Union{String,Int} # ProgressToken? I think the handwritten constructor for InitializeParams is the only instance where the Type is called

[pfitzseb]

Sounds good to me.

[aviatesk]

yeah good to me too