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

Bug: Incorrect result for multi-assignment #315

Closed
1 of 2 tasks
tooolbox opened this issue Apr 15, 2021 · 8 comments
Closed
1 of 2 tasks

Bug: Incorrect result for multi-assignment #315

tooolbox opened this issue Apr 15, 2021 · 8 comments

Comments

@tooolbox
Copy link

  • GopherLua is a Lua5.1 implementation. You should be familiar with Lua programming language. Have you read Lua 5.1 reference manual carefully?
  • GopherLua is a Lua5.1 implementation. In Lua, to keep it simple, it is more important to remove functionalities rather than to add functionalities unlike other languages . If you are going to introduce some new cool functionalities into the GopherLua code base and the functionalities can be implemented by existing APIs, It should be implemented as a library.

Please answer the following before submitting your issue:

  1. What version of GopherLua are you using? : GopherLua 0.1 Copyright (C) 2015 -2017 Yusuke Inuzuka
  2. What version of Go are you using? : go version go1.15.7
  3. What operating system and processor architecture are you using? : darwin/amd64
  4. What did you do? : Execute glua ./repro.lua where the file contains:
local a = {}
local d = 'e'
local f = 1

f, a.d = f, d

print(f..", "..a.d)
  1. What did you expect to see? : The result of 1, e
  2. What did you see instead? : The result of d, e

Somehow the property-access in the multi-assignment causes f to contain the name of the second variable on the right side instead of the value of the 1st variable on the right side.

As a note, swapping the sequence results in the correct behavior: a.d, f = d, f

Reference: #314

@Simerax
Copy link

Simerax commented Apr 16, 2021

It looks like the Problem is within compileAssignStmtLeft. There is some check if the identifier (in this case f) is the last argument. In case it is its Position (ec.reg) is looked up via context.FindLocalVar. If it's not the last argument there is no lookup.

Changing this so the variable is always looked up works and the tests still pass. However I don't know if there is some edge case which requires the variable not to be looked up.

@tooolbox
Copy link
Author

Is this something you would fix @Simerax or do we need to involve @yuin ?

@Simerax
Copy link

Simerax commented Apr 22, 2021

I can create a pull request yes. I was just hoping yuin would maybe give us some insight on this FindLocalVar thing.

@Simerax
Copy link

Simerax commented Apr 22, 2021

Alright after looking into this a little more - I think there is something broken in the way multi assignment works in gopher-lua.
There is a multi assign test in _lua5.1-tests/attrib.lua which is currently commented out. This test fails before and after my change.

do
  local a,i,j,b
  a = {'a', 'b'}; i=1; j=2; b=a
  i, a[i], a, j, a[j], a[i+j] = j, i, i, b, j, i
  assert(i == 2 and b[1] == 1 and a == 1 and j == b and b[2] == 2 and
         b[3] == 1)
end

I really don't know enough about how lua or gopher-lua works to fix this. I think this is something @yuin should get involved in.

@tooolbox
Copy link
Author

@yuin any thoughts?

@tooolbox
Copy link
Author

tooolbox commented Jan 2, 2023

Hooray!

@Simerax
Copy link

Simerax commented Jan 3, 2023

Alright after looking into this a little more - I think there is something broken in the way multi assignment works in gopher-lua. There is a multi assign test in _lua5.1-tests/attrib.lua which is currently commented out. This test fails before and after my change.

do
  local a,i,j,b
  a = {'a', 'b'}; i=1; j=2; b=a
  i, a[i], a, j, a[j], a[i+j] = j, i, i, b, j, i
  assert(i == 2 and b[1] == 1 and a == 1 and j == b and b[2] == 2 and
         b[3] == 1)
end

I really don't know enough about how lua or gopher-lua works to fix this. I think this is something @yuin should get involved in.

The mentioned test still fails with this change.

Lua 5.1
--- FAIL: TestLua (7.57s)
    script_test.go:79: attrib.lua:334: attempt to index a non-table object(number) with key '3'
        stack traceback:
        	attrib.lua:334: in main chunk
        	[G]: ?
-------------------------

@yuin
Copy link
Owner

yuin commented Jan 3, 2023

This issue has been closed because the original issue was resolved.
Please submit a new issue If you have a problem.
And I KNEW the problem so I've commented out the test code. It is hard to fix because of the parsing strategy differences between CLua and GohperLua.

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