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

Add 'let' to top level definitions; infer breaks using top-level keywords #85

Merged
merged 8 commits into from
Jan 2, 2017

Conversation

lepoetemaudit
Copy link
Contributor

This PR represents a small shift in how we lex/parse statements at the top level.

Previously, all top level statements had to terminate with at least two new lines, i.e. \n\n. This meant encountering situations such as this: #82

To remedy this, and following discussions with @j14159 and @ypaq on IRC, this proposed PR includes the following changes:

  1. All whitespace significance is removed.
  2. After the initial scanning, breaks are inserted into the token list by logically determining top level keywords. This has to be done with some notion of state in this style as 'type' and 'let' may have a different meaning depending on their context. This step can be found here.
  3. We now require 'let' before top level function / value definitions. It's noisier than before, but there isn't any way of otherwise distinguishing these from other top level constructs, so it was this or add more whitespace significance and 'offside' rules such as in Haskell/Elm. On the bright side, more OCaml code just became copy-pastable into Alpaca :)
  4. The previous fix to sanitise groups of newlines is no longer required so it has been removed

I also made it aware of the up-coming 'import' keyword so hopefully integrating that will be smooth.

@lepoetemaudit
Copy link
Contributor Author

Seems to be getting stuck on the common test stage... Seems to be hanging for me locally unless it's just really slow?

@j14159
Copy link
Collaborator

j14159 commented Dec 31, 2016

Travis-CI is killing the ct stage, could be time or memory limits.

@j14159
Copy link
Collaborator

j14159 commented Dec 31, 2016

@lepoetemaudit the number of tests proper is running for ct is set to 1000 at the moment - maybe try cutting that in half? Line 12 of test/alpaca_SUITE.erl.

@lepoetemaudit
Copy link
Contributor Author

@j14159 thanks, I actually tried setting it to 1 (!) locally and same effect, so I can only assume I broke it somehow. I'll investigate :)

@j14159
Copy link
Collaborator

j14159 commented Dec 31, 2016

Oh, maybe test/alpaca_SUITE.erl needs to change how it generates its top-level defs to include let as well? Doesn't look like that change is in here but maybe I missed something.

@lepoetemaudit
Copy link
Contributor Author

Yes, I think that's it - I had a quick stab at adding the let but I'm not very familiar with quickcheck et al. Will figure it out and update in due course.

@lepoetemaudit
Copy link
Contributor Author

Fixed - it turns out removing the sanitization fix I put in a while for tabs in-between newlines caused the scanner to hang in some corner cases. I'm not entirely sure why, though.

@j14159
Copy link
Collaborator

j14159 commented Jan 1, 2017

Thanks for this! I know it's a bit of a slog to walk through fixing all of those tests.

Before I merge, do you think it's worth expanding the definition of WS in the lexer to include \n and just do away with tokenizing breaks entirely? I'm not sure but it might elide the need for the cleanup before inferring breaks. I'm easy either way.

@yurrriq
Copy link
Contributor

yurrriq commented Jan 1, 2017

Maybe we can write some PropEr tests. I found them invaluable when writing LFE's docstring support.

@lepoetemaudit
Copy link
Contributor Author

Amending all the lets was a nifty way of getting familiar with all the tests so it wasn't so bad :)

I think it would be better to fix the scan as you suggest, it's a bit of a smell otherwise. It's possible to cause infinite loops in Lecc with non-greedy matches which is what I think is happening here. I'll see what I can figure out.

@yurriq this whitespace issue was caught by proper, as added by @YPAC - it's very clever stuff which I'm looking forward to getting more familiar with!

@yurrriq
Copy link
Contributor

yurrriq commented Jan 1, 2017

this whitespace issue was caught by proper

Oh, cool! I guess I missed that.

@j14159
Copy link
Collaborator

j14159 commented Jan 2, 2017

Per discussion with @lepoetemaudit in IRC I'll merge this now with discussed scan fixes to come later. Fixes #82

@j14159 j14159 merged commit 8b51f8c into alpaca-lang:master Jan 2, 2017
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.

3 participants