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

fix: Return original t in find_module if search_for_token fails #396

Merged
merged 2 commits into from
May 5, 2023

Conversation

lionkor
Copy link
Contributor

@lionkor lionkor commented May 5, 2023

Fixes #258.

The if block starting at line 400 in ldoc/parse.lua is the problem.
It seems to look for a module() call, and then:

  • if it finds one, uses that for the module name
  • if it doesn't, infers the name from the filename etc.

This means that in all modern Lua code it will infer the name, obviously.
The issue is that in line 402, it calls lang:find_module(tok, t, v) (that is Lua:find_module from lang.lua), which will (for anything that doesn't use module()) return nil, t, v where t, v is from self:search_for_token(tok, 'iden', 'module', t, v).

I know this is a bit to read, but here I believe the issue lies:

Lang:search_for_token(...) will return t as nil, here's why:

(from search_for_token)

   -- `t` may be nil here, which means it returns `false, nil, v`
   -- if `t` is nil
   return t ~= nil,t,v

And find_module says:

   res,t,v = self:search_for_token(tok,'iden','module',t,v)
   -- we know res may be `false` and `t` may be nil at the same time
   if not res then return nil,t,v end

We go back to parse.lua, line 402:

            module_found,t,v = lang:find_module(tok,t,v)

at this point module_found would be nil, and t would be nil also.

Now, a few lines down:

            if not t then
               F:warning('contains no items','warning',1)
               break;
            end -- run out of file!

So this is the issue - t may VERY well be nil.

Alternatively, we could:

  • Remove the entire if statement that causes this warning, but Chestertons Fence speaks against this - I can't quite figure out exactly why this warning and the subsequent break need to be there, so I will not remove it.
  • Remove the break only, and hope nothing relies on this - again, not sure that's the right move.

t arbitrarily being set to nil seems wrong here, so that's what I fixed, and it fixes the issue with the following reproducible sample:

--- my function
-- takes nothing and returns nothing
-- @return nothing
function DoNothing()
end

--
Thing = {
}

And also seems to work fine for code using module().

…ixes lunarmodules#258)

If `search_for_token` does not find a `module` token, t is nil, and t is
returned instead of the original t.

This is an issue, because it causes a branch in `parse.lua` to
erroneously be executed, which exits the entire parsing process with the
warning `contains no items`, even if items are left (because the
original t was scrubbed).
@lionkor
Copy link
Contributor Author

lionkor commented May 5, 2023

Long story short - we preserve t across a function call that will set it to nil if it fails, because we expect that it may fail.

ldoc/lang.lua Outdated Show resolved Hide resolved
It's unclear whether `search_for_token` would end up clobbering `v` if
it returns false or a false-y value (failure), as it definitely does
clobber `t`. For this reason (as suggested by @alerque), a temporary
`sv` is used.

I picked the `s*` prefix naming since the function generating them
starts with s.
@alerque alerque merged commit dc612f5 into lunarmodules:master May 5, 2023
@lionkor lionkor deleted the fix-contains-no-items branch May 5, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Getting "<luafile>: contains no items"
2 participants