Minor quote quality improvements after static analysis #10905

mytskine · 2023-09-15T07:34:03Z

I searched for a (maintained) static code analyzer for Lua and found selene. I configured and applied it to koreader's source code. Here are fixes for some of the errors and warnings listed by selene. I hope that helps a little. If not, just drop the request.

The first commit clearly fixes a bug. The following commits are more innocuous.

I did not include a commit where I had doubts: In koreader, some calls to io.open() use a mode of "re" or "we". AFAIK, this "e" is undocumented in Lua's io. It's not in POSIX fopen() either. So I was wondering if it's a bug, or if I missed something.

This change is

frontend/ui/widget/focusmanager.lua

Frenzie · 2023-09-15T09:22:43Z

frontend/apps/reader/modules/readersearch.lua

@@ -287,12 +287,12 @@ function ReaderSearch:onShowSearchDialog(text, direction, regex, case_insensitiv
                self.wait_button.movable:setMovedOffset(self.search_dialog.movable:getMovedOffset())
                UIManager:show(self.wait_button)
                UIManager:tickAfterNext(function()
-                    do_search(func, pattern, param, regex, case_insensitive)()
+                    do_search(func, pattern, param)()


This smells like the wrong solution to an accidental regression (depending a bit on what param signifies, maybe it's all fully functional).

Ought to be safe, everything is in the same function, so these are closure'ed earlier, looks like.

Haven't looked at the history, though.

Possibly clearer to just keep the larger signature, and populate it instead, though?

I suppose it's more of a comment on the commit message than on the change per se. As written it sounds a lot like "apply suggestion from dumb linter that doesn't understand context," which I find unhelpful at best. The question is if it's unnecessary, not if the number of arguments is mismatched. Simply removing some arguments to shut up a linter smells like papering over a deeper problem.

You might say it's similar in that I'd still have to check if it had said something like "unnecessary," but then it's double checking a human, not a dumb linter.

Possibly clearer to just keep the larger signature, and populate it instead, though?

Also that.

Looks indeed like these 2 args are not used.
Pinging @zwim from #7883 - to check if this is alright and that it doesn't hide some other problem (a bit head scratching these closures & callbacks...)

It really seems that in #290, #295, #345 and #352 those last two parameters can be deleted.
They are used, but not as function-parameters but by the jit-compiler.

(Untested, as I have no time now.)

My understanding of the code was that do_search() was now wrapped in another container where the context (regex, ci) was defined. So the old parameters of do_search() were no longer needed. Of course, this is not visible in the patch, and the source code of the search is quite long, so I could have misunderstood.

Frenzie · 2023-09-15T09:30:27Z

frontend/apps/filemanager/lib/md.lua

-    local min = 1/0
-    local finish = 1/0


I suspect this may be a recommendation for Lua that's completely irrelevant to LuaJIT. (Not an objection, but not support either.)

Actually I just noticed which file this is in. Don't touch that file. ;-)

Or at least not without bothering upstream about it https://github.com/bakpakin/luamd

## Why this is bad `n / 0` equals `math.huge` when n is positive, and `-math.huge` when n is negative. Use these values directly instead, as using the `/ 0` way is confusing to read and non-idiomatic.

But sorry, / 0 confusing to read? I feel a sudden urge to replace all math.huge with 1 / 0 since it's very much not, lol

As suggested in my opening comment, I also think it's mostly cosmetic.

In any case, either put this file or that selene contraption on the ignore list.

Frenzie · 2023-09-15T09:46:28Z

Thanks for the PR! I'm not quite convinced by the commits other than the first (give or take) and the last but it does probably warrant more investigation.

NiLuJe · 2023-09-15T14:33:08Z

It's not in POSIX fopen() either.

It's Linux's O_CLOEXEC, and as LuaJIT's io.open is a very thin wrapper around fopen÷, it just works ;).

(And it's fairly critical for some stuff on Kobo, for instance).

fopen(3)

     e (since glibc 2.7)
          Open the file with the O_CLOEXEC flag.  See open(2) for more information.  This flag is ignored for fdopen().

Frenzie · 2023-09-15T14:41:49Z

or if I missed something.

Oh yeah, forgot to say, but it's not Lua, it's LuaJIT. It generally doesn't matter but we use some special LuaJIT features and occasionally also things that aren't unique to LuaJIT per se but that are actually from 5.2 or 5.3.

NiLuJe · 2023-09-15T14:49:01Z

(And that linters or LSP may have trouble groking, for that matter ;)).

poire-z · 2023-09-15T15:29:49Z

frontend/apps/reader/modules/readerhighlight.lua

-            self:onHoldRelease(nil, self:_createHighlightGesture("hold_release"))
+            self:onHoldRelease()


Looks like there is indeed nothing to receive these arguments.
But as this is from #8877 for highlights with Non-Touch devices and the need to create fake events, and there may be some magic/hijacks, pinging its author @comphilip if this makes sense or if it's just leftover from copy/paste or else.

Or more probably, this is just building and passing arguments as we do to most onTouchGestureHandler, passing (arg, ges).
It just happens that our own ReadHighlight:onHoldRelease() in that file does not need them, so it was declared without them.
I think the proper way would be to just declare /define ReadHighlight:onHoldRelease(arg, ges) - as most other :onSomething in readerhighlight.lua - even if the implementation does not use them.

Indeed, I'd also argue it's better than the alternative. But your suggestion introduces an unused local variable; something linters always complain in such event handler type situations to my annoyance. :-)

Recent luacheck versions will grok if if prefixed by an underscore, IIRC (e.g., func(_foo, _bar)).

@poire-z You are right. It is creating fake events following touch call protocol (arg, ges). Lua function handler can declare ignore arguments. But changes to self:onHoldRelease() is not a good idea.

But changes to self:onHoldRelease() is not a good idea

The call in question or the implementation?

(i.e., is that just a NAK on that commit?)

I mean the call: removing arguments break event handler function protocol. It may cause unexpected issues in future:

If there is a chance that some one will override onHoldRelease and he needs arguments, then the application will crash due to null pointer exception.

The event protocol is solid: there are always two arguments passed to event handler. Widget author will be surprised that it is not always true. It will make event handler function need more code to handle such unexpected behavior.

Follow the protocol make life easy.

poire-z · 2023-09-15T15:44:57Z

Here are fixes for some of the errors and warnings listed by selene

Are there others ? :)
I'm surprise there are so little - we must be incredibly good :) (or luacheck is good enough to notify us of most problems.)

pazos · 2023-09-16T00:08:37Z

Here are fixes for some of the errors and warnings listed by selene

Are there others ? :) I'm surprise there are so little - we must be incredibly good :) (or luacheck is good enough to notify us of most problems.)

a few of them

Some categories, like ambiguity or different-requires, are ok to fix.
Addressing missing-parameter, redundant-parameter and unbalanced-assignments would be madness :D

Both luacheck and lua-lsp fail to catch performance issues like the excesive usage of string concatenation.

I think lsp for lua is great but their diagnostics are not so useful (above luacheck).
After all it is lua and any variable could be Any? all the time.

mytskine · 2023-09-17T08:12:42Z

@NiLuJe Thanks for explaining the "e" mode in glibc's open().

@poire-z I filtered out the errors and warnings of selene that I didn't think useful enough (see my config below). It still complained about 3 goto uses and a few unbalanced assignments (e.g. local s, e, m, d = 1, hist_nb).

If someone wants to reproduce the checks, I've attached my config (remove the ".txt" extensions).
luakoreader.yml.txt
selene.toml.txt
I applied the stable release to koreader:

cargo install selene
~/.cargo/bin/selene frontend

NiLuJe · 2023-09-17T14:47:34Z

goto

Supported by LuaJIT

Frenzie · 2023-09-17T15:14:49Z

They have an issue about adding support (for goto that is, not LuaJIT). Also it seems you can compile it various ways with support for the Roblox Lua derivative Luau or some 5.2 features.

a few unbalanced assignments (e.g. local s, e, m, d = 1, hist_nb).

That's debatable. While I'd also prefer to write , nil, nil at the end or to spread it out over multiple lines for clarity, if whatever that's referring to isn't intended for scope control I'd be quite surprised.

Edit: I think that's a pattern @hius07 likes. :-)

05cd59e

zwim · 2023-09-17T20:23:34Z

goto

Supported by LuaJIT

Yeah, very very seldom goto makes sense.
And 3 times is quite OK fmpov

NiLuJe · 2023-09-17T21:33:45Z

I'm possibly the odd duck in being a huge fan of gotos (mostly for error and cleanup handling), but I'll readily agree that they shouldn't be over abused ;).

In the specific case of Lua, it's also sometimes the only way to (somewhat cleanly) handle constructs that would better be handled with a continue keyword, because Lua lacks such a keyword.

Frenzie · 2023-09-18T06:02:11Z

I wouldn't know for sure without checking but I think we only use goto as continue.

And in any case the popular try/except stuff is just goto with a different name.

François Gannaz added 5 commits September 15, 2023 08:54

table == [] will always be false

bc688f1

function with 3 args was called with 5 args

0550b0e

A function with no args was called with 2 args

6f36665

Prefer math.huge to 1/0

a9f520e

Remove useless string.format()

bc88c95

Frenzie reviewed Sep 15, 2023

View reviewed changes

frontend/ui/widget/focusmanager.lua Show resolved Hide resolved

Frenzie reviewed Sep 15, 2023

View reviewed changes

poire-z reviewed Sep 15, 2023

View reviewed changes

Frenzie added 2 commits October 12, 2023 14:49

ignore md.lua third-party lib

b55d3e9

Update readerhighlight.lua: leave that one alone

423e066

Frenzie merged commit 1108302 into koreader:master Oct 12, 2023
1 of 3 checks passed

Frenzie added this to the 2023.10 milestone Oct 12, 2023

Frenzie changed the title ~~Fix lua after static analysis~~ Minor quote quality improvements after static analysis Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor quote quality improvements after static analysis #10905

Minor quote quality improvements after static analysis #10905

mytskine commented Sep 15, 2023 •

edited by Frenzie

Frenzie Sep 15, 2023 •

edited

NiLuJe Sep 15, 2023 •

edited

NiLuJe Sep 15, 2023

Frenzie Sep 15, 2023

Frenzie Sep 15, 2023

poire-z Sep 15, 2023

zwim Sep 16, 2023

mytskine Sep 17, 2023

zwim Sep 17, 2023

Frenzie Sep 15, 2023

Frenzie Sep 15, 2023

Frenzie Sep 15, 2023 •

edited

mytskine Sep 17, 2023

Frenzie Sep 17, 2023

Frenzie commented Sep 15, 2023

NiLuJe commented Sep 15, 2023 •

edited

Frenzie commented Sep 15, 2023

NiLuJe commented Sep 15, 2023

poire-z Sep 15, 2023

poire-z Sep 15, 2023

Frenzie Sep 15, 2023

NiLuJe Sep 16, 2023

comphilip Sep 16, 2023

NiLuJe Sep 16, 2023

comphilip Sep 17, 2023

poire-z commented Sep 15, 2023

pazos commented Sep 16, 2023

mytskine commented Sep 17, 2023

NiLuJe commented Sep 17, 2023

Frenzie commented Sep 17, 2023 •

edited

zwim commented Sep 17, 2023

NiLuJe commented Sep 17, 2023 •

edited

Frenzie commented Sep 18, 2023

		self:onHoldRelease(nil, self:_createHighlightGesture("hold_release"))
		self:onHoldRelease()

Minor quote quality improvements after static analysis #10905

Minor quote quality improvements after static analysis #10905

Conversation

mytskine commented Sep 15, 2023 • edited by Frenzie

Frenzie Sep 15, 2023 • edited

Choose a reason for hiding this comment

NiLuJe Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Sep 15, 2023

NiLuJe commented Sep 15, 2023 • edited

Frenzie commented Sep 15, 2023

NiLuJe commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poire-z commented Sep 15, 2023

pazos commented Sep 16, 2023

mytskine commented Sep 17, 2023

NiLuJe commented Sep 17, 2023

Frenzie commented Sep 17, 2023 • edited

zwim commented Sep 17, 2023

NiLuJe commented Sep 17, 2023 • edited

Frenzie commented Sep 18, 2023

mytskine commented Sep 15, 2023 •

edited by Frenzie

Frenzie Sep 15, 2023 •

edited

NiLuJe Sep 15, 2023 •

edited

Frenzie Sep 15, 2023 •

edited

NiLuJe commented Sep 15, 2023 •

edited

Frenzie commented Sep 17, 2023 •

edited

NiLuJe commented Sep 17, 2023 •

edited