lunarmodules / ldoc

LDoc is a LuaDoc-compatible documentation generator which can also process C extension source. Markdown may be optionally used to render comments, as well as integrated readme documentation and pretty-printed example files.
https://lunarmodules.github.io/ldoc/
Other
787 stars 173 forks source link

The ldoc 1.4.5 parsing is broken [regression] #248

Closed Elv13 closed 4 years ago

Elv13 commented 8 years ago

---------------------------------------------------------------------------
--- Something
--
-- @author someone
-- @copyright 1999 The universe is about to bug corporation
-- @release 1
-- @module something
---------------------------------------------------------------------------

--- My text.
-- @field foobar

-- foobar < the error is at this line
ldoc -d /tmp/bob /tmp/bob/broken.lua

/tmp/bob/broken.lua:13: definition cannot be parsed - not 'name=value' or 'return value'

output written to /tmp/bob

See: https://travis-ci.org/awesomeWM/awesome/jobs/154053410

I can reproduce on both the Travis CI and my own computer.

stevedonovan commented 8 years ago

'feild' should be 'field'?

What the error is saying is that LDoc cannot understand the line following the comment - and what follows is a comment. What happens if you use 'field'?

Elv13 commented 8 years ago

Sorry, that was a typo when I wrote the github issue. It doesn't work with field. Actually, it doesn't even work without it.

Here is a simplified example

---------------------------------------------------------------------------
--- Something
--
-- @module something
---------------------------------------------------------------------------

--- My text.

-- foobar < the error is at this line

Same error

stevedonovan commented 8 years ago

OK, but the error still makes sense. You are documenting an item, and LDoc needs to read the code afterwards to understand that item. It expects code (like function ... or A = ... or return ...) and instead it finds a comment. In the old version it would not complain and give you bad results anyway.

It will only look ahead if it doesn't have enough information.

Elv13 commented 8 years ago

No, the old version worked fine with this. Here is the element that causes the error in 1.4.5 but work in older versions: http://new.awesomewm.org/apidoc/libraries/mouse.html#awful.mouse.snap.default_distance

Also, why would this be "broken". If module A has an attribute (field), it doesn't need to be defined globally. It could be initialized by some kind of constructor elsewhere in the code. So it would force us to define it globally to nil until that constructor is executed and that doesn't make much sense.

This @field is used to define a module attribute, not a table element.

stevedonovan commented 8 years ago

Yes, I can reproduce - definitely a problem. Should have a fix soon.

Elv13 commented 8 years ago

Thanks. Maybe you should consider reverting the luarocks package (if you can, I never made one and don't know if it's possible) to prevent further continuous integration systems breakage. To me, this seems like something quite major. I am sure we are not the only one to define functions outside of the code. Thinking about it, it is also necessary in case of __index metamethods where some elements are defined "deeper" in the metaobject than the module code. Think of lua objects wrapping some userdata. This is quite common, a large percentage of the libraries do that.

mpeterv commented 8 years ago

Bisecting shows that the issue is introduced in be85db6.

mpeterv commented 8 years ago

Another regression is that if the empty line between @field comment and foobar comment is removed, there is no warnings but the field is not in the docs. That's fixed by resetting is_local flag to false after it is erroneously set to the error message when calling lang:item_follows() results in a parser error. However, this doesn't fix the first regression, ldoc still thinks that after a ldoc comment and space there must be some Lua code and not another comment.

stevedonovan commented 8 years ago

This is the minimal necessary fix - don't complain about parse errors if we have an item that just contains field or param. Plus not resetting is_local - thanks Peter, well spotted. Passes all tests I have but we really need more...

stevedonovan commented 8 years ago

Can do a quick 1.4.6 release if this is satisfactory

stevedonovan commented 8 years ago

Right - I've pushed 1.4.6 to luarocks - please update and hopefully this should be sorted.

Moral of story is: never can have enough tests...

mpeterv commented 8 years ago

After the assignment order fix the warning is still there, although the field is now present in the docs.

Elv13 commented 8 years ago

Thanks for the fix. However, I confirm @mpeterv comment, the issue is still there. Our build still fails for document parsing "errors". We consider doc errors/warnings as fatal errors, we also have extra linting scripts that pre-parse the ldoc tags to add extra constraints on the formatting and build indexes (retro-fed into the documentation in HTML). Below that, we also "extract" the @usage examples to unit test them and auto-generate output images (see). Finally, we have macros to insert repetitive interfaces documentation into the files. So our documentation integration pipeline is a bit more complex than "usual" ldoc projects.

Sorry for insisting on this details and thanks for releasing 1.4.6 so quickly. However, do you consider this a real warning or is this an oversaw from the previous fix?

https://travis-ci.org/awesomeWM/awesome/jobs/154053412

stevedonovan commented 8 years ago

You are quite right to bug me, since it is my dog that is misbehaving ;)

Those errors look harmless (as long as correct documentation is generated) but they remain a problem since you want a clean build with no bad status returned.

But I know the pattern that triggers the error and we'll fix it soon.

Elv13 commented 8 years ago

Ping.

I am getting more and more reports about this as more people install newer ldoc. I can't automagically fix earlier versions of AwesomeWM already on the users computers. Is it possible to have a new release that can process AwesomeWM without issues?

I am not complaining or angry or anything, but this is less than good for our software if people can't install it without uninstalling ldoc. Maybe for new releases, we will make this check for debug builds only, but for now I can't change the past. The only way to resolve this is to change the future, and that involve you ;). Sorry about being annoying.

stevedonovan commented 8 years ago

No, you're not being annoying at all! Just keeping the issue to the forefront. I'll check out the repo and fix this one properly.

blueyed commented 8 years ago

@stevedonovan That's very much appreciated! Feel free to ask in #awesome on Freenode if you run into problems while trying to reproduce this.

stevedonovan commented 8 years ago

I think we're sorted guys - I tested on master branch, no more spurious errors. Check on your side and I'll push a new rockspec

blueyed commented 8 years ago

@stevedonovan I am still getting an error:

reading configuration from config.ld
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/luaa.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/luaa.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/objects/client.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/mouse.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/objects/screen.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/objects/tag.c
found master module …/a.example.com-x86_64-pc-linux-gnu-6.2.1/lib/naughty/core.lua
format: using built-in markdown
lua: …/Vcs/ldoc/ldoc/tools.lua:112: table index is nil
stack traceback:
    …/Vcs/ldoc/ldoc/tools.lua:112: in method 'add'
    …/Vcs/ldoc/ldoc/doc.lua:397: in method 'finish'
    …/Vcs/ldoc/ldoc.lua:534: in upvalue 'add_special_project_entity'
    …/Vcs/ldoc/ldoc.lua:613: in local 'operation'
    …/Vcs/ldoc/ldoc/tools.lua:529: in function 'ldoc.tools.process_file_list'
    …/Vcs/ldoc/ldoc.lua:612: in main chunk
    [C]: in ?
make[3]: *** [CMakeFiles/ldoc.dir/build.make:184: CMakeFiles/ldoc] Error 1

awesome c9f085f (master), ldoc 3f57e43 (master).

blueyed commented 8 years ago

Seems to be related to some local / untracked file, works fine in a clean checkout! Thanks!

blueyed commented 8 years ago

Wait!

The clean build used ldoc 1.4.3..

blueyed commented 8 years ago

Still an issue in a clean checkout/build, using make ldoc.

Uncommenting the print (https://github.com/stevedonovan/LDoc/blob/3f57e431a28c55dde9cb398e5b991301f15f1439/ldoc/tools.lua#L111) I see:

nil Signals section
lua: …/Vcs/ldoc/ldoc/tools.lua:112: table index is nil

Looks like what was reported at https://github.com/awesomeWM/awesome/issues/1127#issue-180344282 already, and is probably related to an unclean build dir?!

docs/config.ld
43:new_type("signal", "Signals", false, "Arguments")

awesomerc.lua
499:-- {{{ Signals

.a.example.com-x86_64-pc-linux-gnu-6.2.1/docs/config.ld
43:new_type("signal", "Signals", false, "Arguments")

.a.example.com-x86_64-pc-linux-gnu-6.2.1/docs/05-awesomerc.md
598:## Signals

.a.example.com-x86_64-pc-linux-gnu-6.2.1/awesomerc.lua
476:-- {{{ Signals
blueyed commented 7 years ago

This is fixed by 8de6c79 now. Let's close this as duplicate of #251 then - or the other way around.

@stevedonovan I see that you do not really like the fix as-is, but please consider releasing it for now. Thanks!

blueyed commented 7 years ago

@stevedonovan Thanks for creating a new release (1.4.6)! But it seems to identifies itself as version 1.4.5 (and therefore gets blacklisted in Awesome's build; https://github.com/awesomeWM/awesome/pull/1207).

So you might want to create another release, and also tag it in Git / GitHub.

psychon commented 7 years ago

1.4.6 is about as old 1.4.5: https://luarocks.org/modules/steved/ldoc I guess most people who said that "broken with 1.4.5" actually meant "broken with 1.4.6, but I didn't notice that I have 1.4.6 installed" (there is also no tag for it here in git).

stevedonovan commented 7 years ago

Mea culpa! Yes 1.4.6 rockspec bundles LDoc that still labels itself as 1.4.5 (no new tag either).

Easy to fix this, but the question is: is LDoc master fine with Awesome docs now?

blueyed commented 7 years ago

is LDoc master fine with Awesome docs now?

Yes.

stevedonovan commented 7 years ago

Excellent! ldoc-1.4.6-2 rockspec has just been uploaded. The only material change (apart from tagging) has been bumping the actual version to 1.4.6