~gpcf/advtrains-devel

2 2

Possible bug in advtrains/occupation.lua line 162 (function o.reverse_lookup)

Details
Message ID
<2f88bc6d-dec9-4888-a85f-0638003761f6@f4grx.net>
DKIM signature
missing
Download raw message
Hello,

This code refers to an inexistent variable or parameter:

https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L162


This has caused a failure to start my server (5.6.1, from debian 12 
repos) with an existing world that had trains tracks and lines already 
defined.

It did not trigger at the moment I installed the advtrains in this world.


I have commented out the line with the check for train_id (and the 
associated end) so the function has the same structure as 
reverse_lookup_quick:

https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L174

Also note that the comments on both these functions are inconsistent 
with the code.


With that change my world loaded ok and trains seem to behave ok, too.


I am not providing a patch because I am not intimate with this code and 
I dont know what was the intended effect.

Best regards and thank you all for developing advtrains, it's 
excessively good.

Sebastien F4GRX
Details
Message ID
<daed9a98-8e40-45e5-8f6d-fc12a9f368ff@forksworld.de>
In-Reply-To
<2f88bc6d-dec9-4888-a85f-0638003761f6@f4grx.net> (view parent)
DKIM signature
missing
Download raw message
> This code refers to an inexistent variable or parameter:
> 
> https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L162

Thanks for the report. My guess here would be that the function should 
be of the form
	function o.reverse_lookup(ppos, train_id)
to filter out entries associated with a certain train. This would also 
explain why the train_id comparison is there in the first place.

That said, git-grep does not show any usage of the function with the 
second parameter, so it is also possible that someone forgot to remove 
the comparison when refactoring the code there.

> This has caused a failure to start my server (5.6.1, from debian 12 
> repos) with an existing world that had trains tracks and lines already 
> defined.
> 
> It did not trigger at the moment I installed the advtrains in this world.

The expression (t[i] ~= train_id) should not cause an error in that 
context though, as (1) t is not falsy (and it seems like it should 
always be a table if it is not falsy), as checked earlier and (2) 
(in)equality comparisons should not throw errors, especially that 
metamethods are out of the question here considering that train_id 
should be nil. I guess this also explains why nobody noticed it for 
almost six years.
> I have commented out the line with the check for train_id (and the 
> associated end) so the function has the same structure as 
> reverse_lookup_quick:
> 
> https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L174
> 
> Also note that the comments on both these functions are inconsistent 
> with the code.

I'm less sure about how the comments on the functions are inconsistent 
though.

Regardless of that, IMO it would make sense to put in luacheck to make 
sure issues like this get addressed, especially now that support for 
Minetest may be introduced into luacheck soon.[1]

[1] https://github.com/lunarmodules/luacheck/pull/108
Details
Message ID
<fead1ac7-0778-4aa5-a9dc-4268906066d9@f4grx.net>
In-Reply-To
<daed9a98-8e40-45e5-8f6d-fc12a9f368ff@forksworld.de> (view parent)
DKIM signature
missing
Download raw message
Hello,

Le 07/03/2024 à 22:39, Y. Wang a écrit :
>> This code refers to an inexistent variable or parameter:
>>
>> https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L162 
>>
>
> Thanks for the report. My guess here would be that the function should 
> be of the form
>     function o.reverse_lookup(ppos, train_id)
> to filter out entries associated with a certain train. This would also 
> explain why the train_id comparison is there in the first place.

That was my first thought and test, it seemed to work when I did that, 
but silversearcher showed only one usage of this function, without this 
parameter, so I imagined the parameter should not be there and then I 
removed the test.

>
> That said, git-grep does not show any usage of the function with the 
> second parameter, so it is also possible that someone forgot to remove 
> the comparison when refactoring the code there.
>
>> This has caused a failure to start my server (5.6.1, from debian 12 
>> repos) with an existing world that had trains tracks and lines 
>> already defined.
>>
>> It did not trigger at the moment I installed the advtrains in this 
>> world.
>
> The expression (t[i] ~= train_id) should not cause an error in that 
> context though, as (1) t is not falsy (and it seems like it should 
> always be a table if it is not falsy), as checked earlier and (2) 
> (in)equality comparisons should not throw errors, especially that 
> metamethods are out of the question here considering that train_id 
> should be nil. I guess this also explains why nobody noticed it for 
> almost six years.

Yes, this was quite unexpected. I have no idea why this code triggered 
when restarting a backed up world with advtrains tracks and trains, but 
not when activating the mod the first time.

>> I have commented out the line with the check for train_id (and the 
>> associated end) so the function has the same structure as 
>> reverse_lookup_quick:
>>
>> https://git.sr.ht/~gpcf/advtrains/tree/master/item/advtrains/occupation.lua#L174 
>>
>>
>> Also note that the comments on both these functions are inconsistent 
>> with the code.
>
> I'm less sure about how the comments on the functions are inconsistent 
> though.

The comments seem to indicate usage of the train_id variable in function 
reverse_lookup_quick, but the code and function signature do not.

Please note that I'm french and english is my second language, so my 
emails may contain syntax errors or inaccuracies.

>
> Regardless of that, IMO it would make sense to put in luacheck to make 
> sure issues like this get addressed, especially now that support for 
> Minetest may be introduced into luacheck soon.[1]
>
> [1] https://github.com/lunarmodules/luacheck/pull/108

Looks like a great tool, thanks for the info!

Best regards,

Sebastien
Reply to thread Export thread (mbox)