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 syntax checker for CHICKEN Scheme #987

Merged
merged 1 commit into from Jul 24, 2016

Conversation

@manuel-uberti
Copy link
Contributor

commented Jun 6, 2016

This PR adds a syntax checker for CHICKEN Scheme.

It uses csc to check for syntax errors, and report them accordingly via Flycheck. I followed the example of the Racket checker, meaning Geiser must be running with CHICKEN Scheme implementation for this syntax checker to work.

This is my first (and I hope not last!) contribution to Flycheck, so please do feel free to point out mistakes or any improvements I need to fix.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2016

@manuel-uberti Thank you very much :)

Please document this syntax checker in doc/languages.rst and—if possible—add an integration test to test/flycheck-test.el.

flycheck.el Outdated
"A CHICKEN Scheme syntax checker using CHICKEN compiler `csc'.

See URL `http://call-cc.org/'."
:command ("csc" "-A" "-local" source-inplace)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jun 6, 2016

Contributor

Do we need source-inplace here? If so please add a comment that explains why we're using source-inplace instead of source here.

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jun 6, 2016

Author Contributor

No, you're right, source is enough. Just tested it. Thanks.

flycheck.el Outdated
(warning line-start "Warning: " (zero-or-more not-newline) ":\n "
"(" (file-name) ":" line ") " (message) line-end)
(error line-start "Error: " (zero-or-more not-newline) ":\n "
"(" (file-name) ":" line ") " (message) line-end))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jun 6, 2016

Contributor

In all of these patterns please don't hard-code the expected indentation after the line break, but use (one-or-more (any space)) instead. That's more tolerant of whitespace changes in the output.

flycheck.el Outdated
@@ -6068,6 +6069,27 @@ See URL `http://www.foodcritic.io'."
;; http://matschaffer.github.io/knife-solo/#label-Init+command
(locate-dominating-file parent-dir "cookbooks")))))

(flycheck-define-checker chicken-scheme
"A CHICKEN Scheme syntax checker using CHICKEN compiler `csc'.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jun 6, 2016

Contributor

Missing "the" after "using".

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2016

Please amend the comment to fix the typo on the commit message :)

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2016

Great, I'll fix everything you pointed out. 👍

@lunaryorn lunaryorn changed the title Add syntach checker for CHICKEN Scheme Add syntax checker for CHICKEN Scheme Jun 6, 2016

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2016

I updated with more commits. Only problem I have is make specs test. My test fails with:

Documentation Syntax checkers documents all syntax checkers
FAILED: Expected (chicken-scheme) to `equal' nil

Which I don't understand. Sorry to be such a newbie. The checker works on the test file I provided.

flycheck.el Outdated
;; In `scheme-mode' we must check the current Scheme implementation
;; being used
(and (boundp 'geiser-impl--implementation)
(eq geiser-impl--implementation 'chicken))))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jun 6, 2016

Contributor

I'm not sure whether I understand this predicate. Do you want to enable this syntax checker only when Geiser Mode is active?

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jun 6, 2016

Author Contributor

Yes, I tried to do something similar to the racket checker.

This comment has been minimized.

Copy link
@fice-t

fice-t Jun 18, 2016

Is there a reason to keep the (not (eq major-mode 'scheme-mode)) form? I'm also not sure why the Racket checker has that form instead of (eq major-mode 'racket-mode).

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jun 19, 2016

Author Contributor

No particular reason. It is my first checker for Flycheck, I started by shamelessly copying the one for Racket.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 1, 2016

Contributor

@fice-t The Racket syntax checker supports two different major modes hence the extra check in the predicate. But it doesn't make sense here.

@manuel-uberti Please remove the major mode check—it's never true since this syntax checker is only ever enabled for Scheme Mode.

Please also update the documentation of the syntax checker to state that Geiser Mode must be active for this syntax checker to work.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 1, 2016

Contributor

@manuel-uberti Do we need to require Geiser Mode? Can't we also use Chicken Scheme when Geiser is not active?

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jul 17, 2016

Author Contributor

Without Geiser Mode, I'm not sure how to determine if the Scheme implementation in use is CHICKEN or not.

flycheck.el Outdated
@@ -6068,6 +6069,33 @@ See URL `http://www.foodcritic.io'."
;; http://matschaffer.github.io/knife-solo/#label-Init+command
(locate-dominating-file parent-dir "cookbooks")))))

(flycheck-define-checker chicken-scheme

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jun 6, 2016

Contributor

I think the syntax checker should probably rather be named "scheme-chicken", given that the language it actually compiles is Scheme, and chicken is just the name of a Scheme compiler.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2016

@manuel-uberti Well, the name of the syntax checker is not the name that you documented in doc/languages.rst, hence the test failure—which has nothing to do with the actual syntax checking, but just tests whether everything is documented properly.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2016

@manuel-uberti And talking of the name, I think I'd prefer if the syntax checker was named scheme-chicken. After all, scheme's the language being checked, and chicken just the name of a scheme compiler.

Please rename the syntax checker and move it into the right place in flycheck-checkers and doc/languages.rst. In the latter it should also go under language “Scheme”, imho.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2016

Sure thing. I'll fix the test as well.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2016

Updated. Now the test works correctly. :)

@@ -902,6 +902,15 @@ to view the docstring of the syntax checker. Likewise, you may use
`flycheck-scalastylerc` is not set or the configuration file not found
this syntax checker will not be applied.

.. supported-language:: Scheme

Flycheck checks CHICKEN Scheme files with `csc`.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 1, 2016

Contributor

You need to backticks on either side to mark csc as literal. With just one backtick it's a cross-reference.


.. syntax-checker:: scheme-chicken

Check syntax with `csc`, the `CHICKEN Scheme <http://call-cc.org/>`_

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 1, 2016

Contributor

Likewise, please two backticks on either side of csc.

@@ -3885,6 +3885,13 @@ Why not:
'(5 9 warning "Redundant braces after class definition"
:checker scala-scalastyle))))

(flycheck-ert-def-checker-test scheme-chicken scheme nil
(skip-unless (funcall (flycheck-checker-get 'chicken-scheme 'predicate)))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 1, 2016

Contributor

This test will always be skipped, because Geiser Mode is never enabled and thus the predicate is never true.

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jul 9, 2016

Author Contributor

Do you suggest I remove the skip-unless check?

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 11, 2016

Contributor

If you did the test would always fail. The point is that the syntax checker won't work if Geiser Mode isn't active, but you never enable it in the tests. You need to install Geiser Mode—add it to the Cask file—and set it up in this test case.

This comment has been minimized.

Copy link
@manuel-uberti

manuel-uberti Jul 17, 2016

Author Contributor

I added Geiser to Cask. How do I enable geiser-mode in the test? make integ is still skipping the test at the moment.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 18, 2016

Contributor

@manuel-uberti You'd temporarily add a hook to scheme-mode-hook that enables Geiser. Take a look at the does-not-check-with-no-byte-compile test for Emacs Lisp to see how to add a hook for just for one test case.

The hook function should then do whatever is required to setup Geiser Mode.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2016

Argh! Where exactly? I didn't change anything besides CHANGES.rst during the rebase.

(lambda ()
(setq-local geiser-scheme-implementation 'chicken)
(geiser-mode))))
(add-hook 'scheme-mode-hook setup-geiser)

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Jul 23, 2016

Member

This lambda isn't indented right :)

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 23, 2016

I think you want this patch:

diff --git a/test/flycheck-test.el b/test/flycheck-test.el
index fa04b59..75d061a 100644
--- a/test/flycheck-test.el
+++ b/test/flycheck-test.el
@@ -3890,9 +3890,9 @@ Why not:

 (flycheck-ert-def-checker-test scheme-chicken scheme nil
   (let ((setup-geiser
-          (lambda ()
-            (setq-local geiser-scheme-implementation 'chicken)
-            (geiser-mode))))
+         (lambda ()
+           (setq-local geiser-scheme-implementation 'chicken)
+           (geiser-mode))))
     (add-hook 'scheme-mode-hook setup-geiser)
     (unwind-protect
         (flycheck-ert-should-syntax-check
@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

Done. :)

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2016

LGTM 😊 thanks

@lunaryorn lunaryorn merged commit 3800bb6 into flycheck:master Jul 24, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

Now I found a problem with this. It seems like the racket checker takes over my scheme-chicken one, meaning it is always recognized has the syntax checker for .scm files when geiser-mode is active, even with geiser-impl--implementation set to 'chicken.

Steps to see the problem:

  • create a test.scm file
  • write ((* 2 2) / 2 ) in it
  • M-x run-geiser and pick chicken
  • M-x flycheck-mode
  • C-c ! v

I get this error message:

progn: Searching for program: No such file or directory, raco

Everything's working fine when I do C-c ! C-c and select scheme-chicken.

I can create another PR to fix the issue, but I need some help to understand how Flycheck can pick the right syntax checker.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

Also, another question. Just discovered that not every message (being it an info, a warning or an error) ends the first line with \n.

Warning: literal in operator position: (2 / 2)

Warning: at toplevel:
  in procedure call to `2', expected a value of type `(procedure (*) *)' but was given a value of type `fixnum'

Is it possible to make that optional?

((info line-start
         "Note: " (zero-or-more not-newline) ":\n"
         (one-or-more (any space)) "(" (file-name) ":" line ") " (message)
         line-end)
   (warning line-start
            "Warning: " (zero-or-more not-newline) ":\n"
            (one-or-more (any space)) "(" (file-name) ":" line ") " (message)
            line-end)
   (error line-start
          "Error: " (zero-or-more not-newline) ":\n"
          (one-or-more (any space)) "(" (file-name) ":" line ") " (message)
          line-end))

Again, I can add the code if needed, I'm glad to help if I can. :)

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@manuel-uberti Can you show the results of C-c ! v in an affected Scheme buffer?

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@manuel-uberti As for the error pattern, I'm not sure what you're asking… I do know not whether the modified patterns work. You'd have to test the modification.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2016

@lunaryorn C-c ! v gives this:
progn: Searching for program: No such file or directory, raco

About the error pattern: I was asking whether there is a way to consider the \n at the end of the line optional.

lunaryorn added a commit that referenced this pull request Jul 25, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@manuel-uberti I've pushed a change to the Racket syntax checker which should fix this problem. The Racket checker tried to call raco unconditionally, now it checks whether raco exists before.

I've also pushed updates to Racket and your syntax checker which should make C-c ! v more helpful for these syntax checkers.

Please try again after the next MELPA rebuild which should be due in about six hours.

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2016

Thank you. However, now I get this when trying C-c ! v:

Debugger entered--Lisp error: (wrong-type-argument symbolp (quote geiser-impl--implementation))
  boundp((quote geiser-impl--implementation))
flycheck-verify-generic-checker(scheme-chicken)
flycheck--verify-princ-checker(scheme-chicken #<buffer test.scm>)
flycheck-verify-setup()
funcall-interactively(flycheck-verify-setup)
call-interactively(flycheck-verify-setup nil nil)
command-execute(flycheck-verify-setup)
@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

@manuel-uberti That seems to be a small typo in aed41c0 (@lunaryorn, do you want me to fix it?)

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@cpitclaudel Please go ahead. No need to ask me, you're an owner of the repo just as I am 😊

Would be awesome if you push the fix, I won't get to do it before tomorrow morning CEST.

I'm sorry for my mistake @manuel-uberti 😔

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Pushed :) @manuel-uberti, can you give it a new try? Thanks!

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@cpitclaudel Thanks 🤗

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2016

Thank you guys, it works now.

Any idea on how to make that \n optional? Just so the checker can print messages that come as one-liners. :)

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

I'm sorry but I do no know off the top of my head. I don't know how the output looks like in all possible variations. Getting patterns for complex output right often requires multiple iterations and fixes, and even then things are still fragile—that's why we prefer to parse from JSON or XML.

I'm sorry but I can't give you any better answer than to experiment. Take a look at M-X re-builder which is very helpful to build complex regular expressions and support the rx syntax Flycheck uses for its patterns albeit without any of Flycheck's special rx form.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

@manuel-uberti I've sent you an invitation to our org which gives you push access to Flycheck (not to the master branch, though) and the ability to merge approved pull requests 😊 It's been a pleasure to work with you on this checker! Thanks for the kind blog post! 😊

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

@lunaryorn wow, I feel honoured! As you saw with this PR, work is taking up most of my time so I hope to be helpful anyway. Thanks a lot.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2016

@manuel-uberti Feel free to invest as much or as little time as you'd like, every bit helps 👍

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

@manuel-uberti Indeed, thanks for your contributions :) Nice to have you on board, and no pressure at all ^^ We're happy to get any help, comments, and suggestions.

@dleslie

This comment has been minimized.

Copy link

commented Oct 1, 2017

What version of Chicken does this require? I can't seem to get this to work; for instance, adding () in an empty line on an otherwise-valid source file results in:

Suspicious state from syntax checker scheme-chicken: Flycheck checker scheme-chicken returned non-zero exit code 1, but its output contained no errors: 
Syntax error: illegal atomic form

	()

	Expansion history:

	<syntax>	  (##core#begin ())	<--

Error: shell command terminated with non-zero exit status 17920: '/home/dleslie/local/bin/chicken' '/tmp/flycheck18272P_n/nanovg.setup' -output-file '/tmp/flycheck18272P_n/nanovg.c' -analyze-only -local
@dleslie

This comment has been minimized.

Copy link

commented Oct 1, 2017

I'm using Chicken 4.12.0

@manuel-uberti

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

I'm not doing much CHICKEN Scheme these days, but if I remember correctly I used 4.9.0.1 when writing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.