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

Clean output #60

Merged
merged 24 commits into from
Apr 10, 2018
Merged

Clean output #60

merged 24 commits into from
Apr 10, 2018

Conversation

bakpakin
Copy link
Owner

@bakpakin bakpakin commented Apr 6, 2018

This is an attempt to begin addressing a number of issues, including #53 and source mapping issues.
It currently fixes the do local _ = ... end bug from #53, adds more granularity to source mapping (adds byte offsets as well as line information) by embedding mapping information in comments at the ends of lines, and removes empty lines from compilation output. I would like to also like to address mangling here,
removing excessive parens in the output, and restructuring the if form to not generate such deeply nested if elses, and uses the native lua elseif.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 6, 2018

The current output is much nicer now. To see the new output without sourcemaps, try ./fennel --compile --sourcemap false generate.fnl. If you remove the --sourcemap option, you can see sourcemap info is appended to each, line, with source line, source byte offset start, and source byte offset end. I have also added the dash to underscore conversion for nicer output.

fennel.lua Outdated
-- Compile sideffects for a chunk
local function keepSideEffects(exprs, chunk, start, ast)
start = start or 1
for j = start, #exprs do
local se = exprs[j]
if se.type == 'expression' then
-- Avoid the rouge 'nil' expression (nil is usually a literal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITYM rogue 'nil' expression.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

fennel.lua Outdated
return ('_%02x'):format(c:byte())
end)
end
symb = symb:gsub('-', '_')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this cause dashes to always be converted to underscores? Copying my concerns from #53 (comment):

...that seems really confusing to me, considering that you can use underscores literally in Fennel already. (let [a-b 1 a_b 2] a-b) should return 1 IMO. Additionally, that approach doesn't do anything for CamelCase, which in my (limited) experience is more common than snake_case.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue should only matter for globals or externally defined variables. Locals (which are all know internally to the compiler) are each mangled to a unique Lua identifier. The function stringMangle takes care of the collision logic. We can safely modify the symMangle function as we like. They certainly could be better named, however.

fennel.lua Outdated
symb = '_' .. symb
end
return symb:gsub('[^%w_]', function(c)
local function escaper(c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason to create this function within symMangle rather than in the scope above it?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason, the definition can be hoisted. I will do so.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved it back to how it was before as it was clearer.

@technomancy
Copy link
Collaborator

Great to see this coming together. I like the elseif changes; that
makes a big difference in readability. In fact, at this point really
my only remaining readability concern is the _1_, etc locals created
for every function.

Out of curiosity, what do you imagine the source maps being used for?
Is there a way you can feed them to a Lua implementation so that it
will use that data for stack traces? It seems like this removes the
ability for the user to get meaningful correspondence in their stack
traces, so the idea of removing that before an alternative method
is ready feels a bit concerning.

I saw that you got rid of macro but kept special. Is special
redundant if folks can do stuff like (eval-compiler (tset _SPECIALS :myspecial (fn [...] ...)))? I could see an argument being made for not wanting to commit to exposing _SPECIALS.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 7, 2018

We actually can feed sourcemaps to Lua in a custom error handler via xpcall. We could then use the debug library (debug.traceback) to get the Lua stack and remap each line in the stack trace to the fennel source. This should allow almost perfect Fennel stack traces. I am also confident that this method will be more robust under complex macro transformations than trying to keep a 1-to-1 correspondence with source code and output.

Since this PR is includes lots of potentially breaking changes, I may not merge it for a bit until people weigh in on various things. Perhaps the custom error handler should be added before a merge.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 7, 2018

And no, special is not special :). I think we can also get rid of luaexpr and luastatment as well. The compiler environment is sufficiently powerful already it seems.

@bb010g
Copy link
Contributor

bb010g commented Apr 7, 2018

I currently require luastatement to get out of an IIFE in a hot loop. Providing these escape hatches is probably a good thing long-term, even if they're almost always not necessary.

As for getting rid of the _1_ locals, I want to try my hand at getting rid of those in a PR after this is merged.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 7, 2018

I've removed the special special form, but left in the luaexpr and luastatement specials for now. As @technomancy pointed out, eval-compiler combined with tset _SPECIALS does much the same thing.

@technomancy
Copy link
Collaborator

technomancy commented Apr 8, 2018

Well, I don't love the idea of requiring a special error handler before line numbers make sense, but I'll hold my judgement till I see how it works in practice.

Edit: that's my only hesitation; everything else looks great.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 8, 2018

These are some rather large commits as they change the mangling a bit, and add more meta data on bindings (basically, extending the current var tracking). Also a lot of work on updating destructure to be more strict about what it accepts for bindings.

For example, global, local, var, and let will now not accept multisyms. You must use set if you want to update a multisym.

There is now a difference between the local name mangling, which is internal to the compiler, and the global name mangling, which is intended to be used as an interface to Fennel if needed. A fennel.mangle and fennel.unmangle function are made available in the API.

This perhaps should have gone in a separate PR, but it depended on the changes from this branch.

@bakpakin
Copy link
Owner Author

bakpakin commented Apr 10, 2018

This latest commit adds the fennel.traceback function that allows proper error handling. I ended up doing something similar to what leafo did with moonscript, and just optionally store the source mapping in memory. This removes the ugly comments at the end of lines, but means there is no source mapping for fennel that has been compiled to Lua ahead of time (I suppose maps could be stored in .map file like JS and loaded into memory manually). It works by default in the repl and the fennel script, and usage when using fennel from Lua is something like

local ok, res = xpcall(function ()
  return fennel.dofile('mycode.fnl')
end, fennel.traceback)
if not ok then
  -- handle the stacktrace in res
  print(res)
else
  -- handle the result in res
  print('OK! ' .. tostring(res))
end

The resulting stacktraces look much like Lua stacktraces, but the file and line information for fennel code should be correct, including code run via fennel.eval.

Example error producing fennel:

;; this will error because x will be :woof
(local bork (fn [x] (+ x 1)))

(local bark (fn [x]
             (bork x)
             nil))

(local dog (fn [x]
             (bark x)
             :cat))

(dog :woof)

{}

Then run with fennel command, fennel.dofile, in the repl, or fennel.eval.

./fennel error.fnl

Should print

fennel error: attempt to perform arithmetic on local 'x' (a string value)
stack traceback:
  error.fnl:2: in function 'bork'
  error.fnl:5: in function 'bark'
  error.fnl:9: in function 'dog'
  error.fnl:12: in main chunk
  (tail call)
  (tail call)
  (tail call)
  [C]: in function 'xpcall'
  ./fennel:21: in function 'dosafe'
  ./fennel:75: in main chunk
  [C]: in ?

@bakpakin bakpakin merged commit 3984875 into master Apr 10, 2018
@technomancy
Copy link
Collaborator

Interesting approach with the traceback; seems to work pretty well!

It looks like there's currently a limitation that the traceback must be generated by the same process as the one that initially compiled the code, but this doesn't necessarily need to be the case moving forward as source maps could be saved and propagated to runtime?

Also, doesn't this mean that including the source map comments in the compiler output isn't actually helpful for AOT compilation as they will never actually get used?

@bakpakin
Copy link
Owner Author

Also, doesn't this mean that including the source map comments in the compiler output isn't actually helpful for AOT compilation as they will never actually get used?

Yeah, I have removed the end-of-line comments for now. In the future, it could be possible to save source maps from compiled code inline in Lua comments, or in a separate file. I'm liking the latter right now. Some function like fennel.loadmap(luafile, mapfile) could then load the sourcemap into memory.

@technomancy technomancy deleted the clean-output branch November 29, 2018 06:22
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

Successfully merging this pull request may close these issues.

4 participants