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

Support of types with dots in their name #5

Closed
adriweb opened this issue Jul 15, 2020 · 13 comments
Closed

Support of types with dots in their name #5

adriweb opened this issue Jul 15, 2020 · 13 comments

Comments

@adriweb
Copy link

adriweb commented Jul 15, 2020

I have a rather large codebase using EmmyLua annotations, and some types have dots (.) in them.
It seems IntelliJ-Luanalysis doesn't support that.

This is quite useful when adding annotations to third-party libs for instance.

Example with a Penlight function:

--- Return a list of all file names within an array which match a pattern.
--- Raise dir and mask must be strings
--- @param filenames table|pl.List An array containing file names.
--- @param pattern string A shell pattern.
--- @return pl.List List of matching file names.
function dir.filter(filenames, pattern) end

This fails with the first @param type since the dots isn't recognized as part of the type name. Same with the return type.

@Benjamin-Dobell
Copy link
Owner

I've been using double underscore as a namespace token for my own purposes. Did this work with EmmyLua?

@adriweb
Copy link
Author

adriweb commented Jul 15, 2020

Well it definitely worked at some point, because the annotations were working just fine, so I guess there was a regression some time? Not sure I could tell you when though :(

@Benjamin-Dobell
Copy link
Owner

Benjamin-Dobell commented Jul 15, 2020

I'd rather reserve ., : and [] tokens for returning the type of fields on another type. In the case of penlight, as long as the pl type contains a List field, then it'll do what you want. However, you won't be able to do:

---@class pl.List

@adriweb
Copy link
Author

adriweb commented Jul 15, 2020 via email

@Benjamin-Dobell
Copy link
Owner

Benjamin-Dobell commented Jul 15, 2020

The downside is just:

reserve ., : and [] tokens for returning the type of fields on another type

It makes much more sense that ., :, [] have the similar semantics in EmmyDoc as they do at runtime i.e. they're used as indexers, which is very useful functionality which is currently missing. It saves you repeating verbose types everywhere, say you've got:

---@class SomeClass
---@field thing number | string | thread | { [1]: "this" | "that", [2]: number }

When you want to refer to the type of SomeClass.thing you ought to be able to do just that i.e.

---@type SomeClass.thing
local aThing = someClass.thing

as opposed to:

---@type number | string | thread | { [1]: "this" | "that", [2]: number }
local aThing = someClass.thing

This neatly addresses #6

However, if your penlight types are accurate, you won't actually need to change anything. Because penlight does have pl type, and on it there is a field called List. So the following will work just fine:

---@type pl.List
local list = pl.List({1, 2, 3, 4})

@adriweb
Copy link
Author

adriweb commented Jul 15, 2020

Right, that's fine maybe for the penlight example, but I do have lots of @Class (and associated @type/param etc in various projects using the lib) declarations that are using it like this:

Let's say I have a Message.lua file just for annotations (so, dummy methods inside) for a DBus lib (I just took some parts for the example):

...

--------------------------------------------------------------------------------
---@class CompanyName.Bus.Message.Method
local Method = {}

---@param call CompanyName.Bus.Message.Method.Call
---@return CompanyName.Bus.Message.Method.Return
function Method.Return(call) end

--- @return CompanyName.Bus.Message.Method.Call
function Method.Call() end

--------------------------------------------------------------------------------
---@class CompanyName.Bus.Message.Signal
local Signal = {}

---@param member string The signal name
function Signal:setMember(member) end

---@param interface string The interface name
function Signal:setInterface(interface) end

---@return CompanyName.Bus.Object.Iterator
function Signal:begin() end

...

return {
  Method = Method,
  Signal = Signal,
  ...
}

For instance CompanyName doesn't actually represent anything, neither in annotations nor at runtime. It's just considered part of the class "name" (and what's used in the require).

Do I have no other way than changing all my annotations?

Looking at the code, I see it's tag_class has just an ID, corresponding to a regex with no possible dots, so I have no idea how it was working this whole time, must have been a bug :(

@Benjamin-Dobell
Copy link
Owner

Benjamin-Dobell commented Jul 15, 2020

Looking at the code, I see it's tag_class has just an ID, corresponding to a regex with no possible dots, so I have no idea how it was working this whole time, must have been a bug :(

Yeah, to my knowledge it was never their intention to support periods in identifiers. I assume that it was working by accident. However, you're absolutely more than welcome to fork and make that change. You'll want to update:

https://github.com/Benjamin-Dobell/IntelliJ-Luanalysis/blob/master/src/main/java/com/tang/intellij/lua/doc.flex#L60
https://github.com/Benjamin-Dobell/IntelliJ-Luanalysis/blob/master/src/main/java/com/tang/intellij/lua/doc.bnf#L37

(Looks like I changed the former file in 6562d4b#diff-905bb6d2efe1bd61d74c550117c8f132L59-R59)

I won't lie, GrammarKit is new to me, so why it was working in the past with periods in just one of those two locations is beyond me.

Anyway, once you've made those two changes, use GrammarKit (https://github.com/JetBrains/Grammar-Kit) to regenerate, quite literally right-click the file in IntelliJ and it's in the context-menu. Then just build again and you're good to go.

As mentioned, I'm unfortunately not interested in making this change myself. I absolutely understand why you want namespaces, having global types and no way of organising them is a recipe for disaster. I'd just rather solve that issue with #3, which needs further brainstorming. Maybe the solution could involve the use of periods to traverse consumer namespaces. However, as for allowing periods in names/identifiers themselves; I don't think I've ever seen a programming language that allows that. Period is usually a token reserved for indexing, which is how I'd like to use the token.

@adriweb
Copy link
Author

adriweb commented Jul 15, 2020

Yeah no worries, I understand your point of view.
It's just too bad that some colleagues and I ended up using this thinking it was a feature and not a bug.

Thanks for the details on how to regenerate it. Not sure I'll do that, but who knows.

@adriweb adriweb closed this as completed Jul 15, 2020
@adriweb
Copy link
Author

adriweb commented Jul 15, 2020

Heh, the VSCode EmmyLua plugin also supports dots:
image

@Benjamin-Dobell
Copy link
Owner

Yeah, impressively, the VSCode plugin actually uses the same code as the Java plugin! It's my intention to get a Luanalysis VSCode build up and running too.

@adriweb
Copy link
Author

adriweb commented Jul 15, 2020

That's great :)

Just as confusing, ID is the same regex, so no idea why it works...

@adriweb
Copy link
Author

adriweb commented Aug 7, 2020

I stumbled into more examples of annotations in other languages allowing periods in class names. For instance https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/server-response-time.js#L49-L54

Is there really no hope of ever getting this to work like in EmmyLua? :) (even though it was never intended, but still :P)

@baharclerode
Copy link

Instead of trying to retroactively reserve ., can . continue to be allowed as part of a class name and instead just reserve a different character for literal indexing (# comes to mind, which is used in java) ?

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

No branches or pull requests

3 participants