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

Teal unable to require base64 #865

Open
JR-Mitchell opened this issue Dec 2, 2024 · 7 comments
Open

Teal unable to require base64 #865

JR-Mitchell opened this issue Dec 2, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@JR-Mitchell
Copy link
Contributor

JR-Mitchell commented Dec 2, 2024

What?

If trying to tl run a .tl file that requires the base64 lua package, the command fails, with the error:

/usr/bin/lua5.4: /usr/local/share/lua/5.4/tl.lua:13477: /usr/local/share/lua/5.4/base64.lua:29:2: syntax error hint: construct starting here is not aligned with its 'end' at /usr/local/share/lua/5.4/base64.lua:65:1:
stack traceback:
        [C]: in function 'error'
        /usr/local/share/lua/5.4/tl.lua:13477: in local 'a_loader'
        /usr/local/share/lua/5.4/luarocks/loader.lua:104: in function </usr/local/share/lua/5.4/luarocks/loader.lua:101>
        (...tail calls...)
        [C]: in function 'require'
        main.tl:1: in main chunk
        (...tail calls...)
        /usr/local/lib/luarocks/rocks-5.4/tl/0.24.1-1/bin/tl:977: in main chunk
        [C]: in ?

This worked on a previous version of teal (0.15.3-1), but since updating to 0.24.1, does not work.
It seems like teal is trying to validate the syntax on the base64.lua file, but (unless I'm misunderstanding, which is very possible), it should only check the types and syntax on the base64.d.tl file, and should just load base64.lua like a standard lua file.

Minimal example

-- main.tl
require("base64")

-- base64.d.tl
local record base64 end
return base64
luarocks install base64
tl run main.tl
@JR-Mitchell JR-Mitchell changed the title Teal unable to run base64 Teal unable to require base64 Dec 2, 2024
@hishamhm hishamhm added the bug Something isn't working label Dec 6, 2024
@JR-Mitchell
Copy link
Contributor Author

JR-Mitchell commented Dec 7, 2024

I've looked into this a little bit more - the first thing to note is that I now realise my understanding

should just load base64.lua like a standard lua file

is incorrect - teal does still parse it's lua dependencies, it just parses less strictly.

It looks like the lines that are causing the run command to fail are:

extract = load[[return function( v, from, width )
	return ( v >> from ) & ((1 << width) - 1)
end]]()

in particular, if I manually swap these lines out for

loaded = load[[return function( v, from, width )
	return ( v >> from ) & ((1 << width) - 1)
end]]
extract = loaded()

then the command no longer fails, which suggests a failure to parse is being caused by the return value of load immediately being called as a function.

The issue is also resolved if the loaded string is brought onto a single line, like

extract = load[[return function( v, from, width ) return ( v >> from ) & ((1 << width) - 1) end]]()

Or if explicit parentheses are added to the load call:

extract = load([[return function( v, from, width )
	return ( v >> from ) & ((1 << width) - 1)
end]])()

If I find any more time I might see if I can dive deeper into this.

@JR-Mitchell
Copy link
Contributor Author

New minimal example:

-- main.tl
load[[return function()
end]]()
tl run main.tl

Errors:

main.tl:2:7: syntax error
main.tl:3:1: syntax error, expected ')'

@JR-Mitchell
Copy link
Contributor Author

JR-Mitchell commented Dec 7, 2024

It looks like this is caused by lines 3151 to 3154:

if tkop.y > prev_tk.y then
   table.insert(ps.tokens, i, { y = prev_tk.y, x = prev_tk.x + #prev_tk.tk, tk = ";", kind = ";" })
   break
end

added to resolve #740

Because the previous token is a multiline string literal, I presume its token y is the y at the start of the literal.
Therefore, once it's parsed, the parser reaches (, and because it is on a lower line to the previous statement, it determines that it's a start of a new statement, and separates it from the previous statement by ;.

@JR-Mitchell
Copy link
Contributor Author

JR-Mitchell commented Dec 7, 2024

It's worth noting that this horrifying valid lua syntax:

local function print_data_and_callback(data)
    print(data)
    return function(record)
        print(record["a"])
    end
end

print_data_and_callback[[hello
world]]

{a="nice"}

executes successfully in lua and prints

hello
world
nice

This runs too with tl run, unless the table at the end is encapsulated in parentheses, due to the lines highlighted above

@JR-Mitchell
Copy link
Contributor Author

JR-Mitchell commented Dec 7, 2024

I know that tl will already add parentheses to any call with a string literal that doesn't have them (e.g it'll change print[[hello world]] to print([[hello world]])).

I wonder if there's some possible solution in here where we move this earlier in the process, so that by the time it's checking if the previous token is on the same line, it's already added a token for the closing parenthesis, which will be on the same line.

This of course doesn't solve the issue if a lua dependency is written like

load[[return function()
end]]

()

or even

load[[return function()
end]]
()

but frankly, I would be surprised if any developer actively chose to write their function calls like this

@JR-Mitchell
Copy link
Contributor Author

The only alternative approach I can see that's fully compatible with all lua is that this insertion of an extra semicolon is turned off when parsing .lua files.

This would however mean that if someone is partway through adding typing to their .lua files, they won't be able to start lines with (name as Type); though (as mentioned in #743 (comment)) .lua files partially annotated this way are not "proper" lua files - perhaps an approach here would be to instead have some third type such as .lax.tl, which is clearly not a .lua file, but could allow this syntax, as opposed to a .lua file, which would never have type annotations and therefore wouldn't need to handle this syntax case anyway.

@JR-Mitchell
Copy link
Contributor Author

JR-Mitchell commented Dec 10, 2024

@hishamhm do you have any suggestions here?
I've been looking at adding a config flag that makes it so that the extra semicolon isn't added in when parsing a .lua file, but because there's not currently any passing down of the env or any CheckOptions into the initial parse step, it would require propagating this as an extra argument in almost every function in the parse step, which seems very inelegant for fixing such a small edge case.
Or I could also follow the pattern of TypeChecker and move all of the parsing into a record, so that this property can be stored as a flag on a new instance inheriting this record and accessed via self - but again, that's a massive refactor for basically one flag.
Can you think of any better way of fixing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants