486949 - Integrate Bespin with jslint

Status: RESOLVED INVALID

(Skywriter Graveyard :: Editor, enhancement)

Description

[Chris J]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

Add a jslint setting to Bespin. When turned on, jslint warnings should appear in the code editor for javascript files. 

Reproducible: Always

Comment 1

[Gordon P. Hemsley [:GPHemsley]]

Sounds like a reasonable request, so confirming.

Comment 2

[Chris J]

Attachment: initial patch to add jslint

The basic JSLint functionality now works. To turn on, use "set jslint on". This will stop the old Narcissus syntax highlighter, and replace it with JSLint. Reverse with "set jslint off". 

Design decisions taken:
* Even though jslint is used for syntax errors / warnings, we still need to run the old parser as well so that the code outline and find function code will work. I think this is inevitable without serious JSLint hackery. That's why I haven't created a new engine to replace Narcissus completely, it's a parallel component.

Known issues with this patch:
* There's no way to set the JSLint options (see http://www.jslint.com/), you just get the default ones
* I currently create a new worker every parse, it doesn't work in Gears (only with HMTL5 workers) and I don't use the existing Bespin.Workers class (partly because I don't like the way it's implemented using URL hashes)
* It only works with javascript code so far. JSLint is actually written to parse HTML and CSS as well, we should enable this too.
* After swapping parsers via "set jslint on|off", you need to trigger a document change event for the new parser to kick in

There are some architectural issues to decide here with the current code (before I change anything) of parser.js. For example, it assumes a global "parse" function from Narcissus, but obviously it's not called that in JSLint. Also it assumes only one parser per engine, but JSLint complements Narcissus rather than replacing it. And it uses a rather horrible WorkerFacade class that passes all the code via a URL Hash. I have studiously avoided all this, presumably it can be handled in follow-up patches.

Let me know what you think!

Comment 3

[Dion Almaer]

Chris J,

Fantastic stuff!

A few things:

* Getting the following on Safari 4: 
  TypeError: Result of expression 'errors' [undefined] is not an object.

* Definitely would like for you and Malte to chat to unify the worker stuff. I really want us to have one way to do Web Workers, and Malte is trying very hard to work on having his abstraction work on all browsers. (Will add him to the cc list on this bug to get his review)

* Swapping parsers: You could just fire bespin.publish("editor:document:changed") in the check for set:jslint.

* Would be awesome to have this on HTML/CSS!

Cheers,

Dion

Comment 4

[Malte]

Hey,

it would be great to have JSLint in the project. I have a couple general questions:
- What kind of parser does it use?
- How does it perform on very large files? (Narcissus uses recursive decent, which can get slow)

Regarding WorkerFacade:
- The url hash hack that you describe is no longer in tip
- You could use a very shallow facade object to wrap JSLint. This will get you a cross browser implementation of importScripts so that you dont have to package JSLint with the worker file and we can install it via paver required.
- Creating a worker for each step should be avoided. You might create a new worker before the old finished and really mess up the CPU :)
- You get Gears support for free

Current browser behavior suggests that we really shouldn't have more than 1-2 extra workers for all stuff that is performed in workers. If you start using WorkerFacade (even if only a very shallow facade without any real logic), we can easily integrate the JSLint code into the new concurrency framework.

Regarding the integration of JSLInt in parallel to the narcissus parser:
- You might want to teach the resolver to have multiple impl. per language
- Maybe we can have the parsers publish events like: parser:js:narcissus:error and than subscribe to that to republish the parser:error event. This would avoid the evil hackery to do different stuff based on the value of the jslint setting.

Thanks for the patch. Great Stuff!

Malte

Comment 5

[Gordon P. Hemsley [:GPHemsley]]

This is a mass migration from Mozilla Labs :: Bespin to Bespin :: Editor.

Comment 6

[Dion Almaer]

Hi Chris,

Any thoughts on Malte's questions and the "error" bug that means it doesn't work for me?

Cheers,

Dion

Comment 7

[Chris J]

Attachment: v2 of the jslint patch

Thanks for the great feedback! I've taken it on board. This version should fix most known issues.

I have re-architected parsers.js, moving much of the code inside each individual parser and enabling multiple parsers to be run against each document. 

I also now use the WorkerFacade stuff as suggested. Since I'm running Linux I haven't had a chance to test Safari or Gears yet but it hopefully should work.

There are only two remaining tasks:
* Figure out how to set JSLint options. Any advice gratefully received - does the Bespin command line allow for anything other than Boolean settings?
* Enable JSLint for html and css. Should be simple, depending on how good JSLint it, but you never know.

Malte - JSLint uses Top Down Operator Precedence - see http://javascript.crockford.com/tdop/tdop.html where the man himself explains performance characteristics.

Let me know what you think.

Comment 8

[Chris J]

Attachment: quick fix - latest version

Oops! the old version had a few lines of legacy code that I should have stripped out. This version is better.

Comment 9

[Malte]

+1 (While I haven't been able to test it yet)

Code looks great. It's so cool that in open source you do something where you know that it needs to get refactored once its extended with e.g. a second parser and then somebody comes along and fixes it :)

Do I miss something or should jslint be included in the patch?
Maybe you can add it to the pavement script. We cant do it with narcissus because we are using a patched version that runs in more JS engines than Rhino.

Comment 10

[Chris J]

JSLint should already be on tip, in the jsparse directory. That's because Dion already added v1 of this patch to the tree. This patch applies on top of that.

Not sure how to move jslint to pavement or what the benefits would be, could you help out here? Thanks. Also if you check out JSLint.reports, it probably contains enough to enable code folding, search for functions and 'smart' outlines. I'd like to work on these next - perhaps we can catch up on IRC on whether there's any work already done on these areas.

Comment 11

[Dion Almaer]

Chris J,

Great. I got a failure on applying the patch as is. Would you mind merging to tip and updating the patch? Then I will get it in ASAP.

Thanks for checking it out Malte.

Cheers,

Dion

Comment 12

[Chris J]

Attachment: merge to tip

merge to tip. Same functionality as before.

Comment 13

[Chris J]

I've filed bug 490219 for setting jslint options

Comment 14

[Dion Almaer]

Hi Chris,

Great stuff.

Put this in changeset 7394e40f5e83

If you could give it a quick test to see if it matches your work and holla if there is an issue.

Thanks!

Cheers,

Dion

Comment 15

[Chris J]

Dion, I'm having real problems testing this changeset because the whole parser keeps stopping working. It seems like there are two issues:
* In the editor, the "enter" key always works twice, i.e. two new lines are created. Similarly, "backspace" always deletes two characters.
* The editor:document:onchange event stops firing after a few changes are made, which in turn stops the parser from running. Using Firefox 3.5b4's error console shows a javascript error "Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMCanvasRenderingContext2D.font]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://127.0.0.1:8080/js/dojo/dojo.js :: anonymous :: line 562"  data: no]"

Have you already come up against these issues? They also appear on tip.

Comment 16

[Dion Almaer]

Hi Chris,

Firefox b4 has messed things up a lot. Should have fixes in before the next RC. Would you mind testing on something else? I am using WebKit / jump to b2 and things seem well.

Cheers,

Dion

Comment 17

[Chris J]

Attachment: fix to tip

This fixes a bug in tip for Firefox which incorrectly assigns the parsor engine as an object not an array for some reason. Other than that, this bug is now good to fix.

Comment 18

[Dion Almaer]

Hi Chris,

Which Firefox did you test that on?

I am testing in FF 3.x and it is doing the right thing for me.....

E.g. doing this in Firebug:

var a = "a"; if (a instanceof Array || typeof a == "array") { console.log("whee"); }

var a = []; if (a instanceof Array || typeof a == "array") { console.log("whee"); }

Comment 19

[Malte]

Hey,

I think I fixed both Narcissus and JSLint parsing. It didnt work for me in FF 3.0 with Gears. I now tested with FF 3 and Safari 4.

@Dion I fixed the bug that caused the need for the ugly workaround that needed the check for Array-ness. WorkerFacase was not correctly serializing Arrays of functions. Thus I could remove the stuff.

I also fixed some other small things. The changeset is here:
http://bitbucket.org/malde/bespin-malde/changeset/88e33e0c1722/

JSLint seems to be really strict. It can't (doesnt want to) parse bespin's source.

Bye
Malte

Comment 20

[Dion Almaer]

Awesome. I got your latest pulled and pushed to tip. Thanks Malte!

Comment 21

[Chris J]

Malte, excellent work! I was just about to try and fix worker.js to handle these arrays myself. 

I have run your changeset and jslint works great for me using Firefox 3.1b3.

Comment 22

[Malte]

Great! Maybe you could look into the ability to run Narcissus and JSLint indepedently (As in not always both). I'm not sure that this can be done with our current setup.

I would think that some people would want only JSLint (because of Speed) or only Narcissus (because it can parse a greater range of JS).

Comment 23

[Chris J]

Malte, good advice I'll work on it. But can you give me an example of some code that JSLint doesn't work on. I'm wondering if it's something to do with the WorkerFacade again.

Comment 24

[Malte]

Sorry, I wasn't meaning that that JSLint doesn't work but rather that it is very strict and most of bespin's source doesn't pass it's standards.

Comment 25

[Chris J]

Attachment: jslint errors vs warnings

This patch, which applies to tip, distinguishes between JSLint errors and warnings. Errors actually stop the parser, warnings are just jslint syntax tips.

Warnings give a golden lineMarker, Errors give a red lineMarker.

All syntax checking is now done via jslint, not Narcissus. It can be set using the "syntaxcheck" setting, which now has the possible values "all, "error", "warning", "off" which means you can turn error checking on independently to warning checking. By default, "syntaxcheck" is set to "off" i.e. neither are reported.

Comment 26

[Chris J]

Attachment: final patch

This patch completes the jslint javascript parsing implementation, by:
* Turning off the old Narcissus parser and replacing with jslint, thus improving parse performance by a factor of up to ten.
* Brings Narcisuss back if codecomplete is turned on (since code complete is the only bit left that relies on certain hacked bits of Narcissus)
* Updating jslint to the new version as modified by Doug Crockford for us, and adding code to pavement.py to automatically re-download from jslint.com on "paver required"
* General parser simplification including replacing the old gotofunction code

Comment 27

[Malte]

Hey Chris,

This sounds great! Can you describe what is currently missing to move codecomplete to narcissus, too? If you got the configurable outline working it should be doable to get codecomplete working as well.

Comment 28

[Kevin Dangoor]

ACETRANSITION

The Skywriter project has merged with Ajax.org's Ace project (the full server part of which is their Cloud9 IDE project). Background on the change is here:

http://mozillalabs.com/skywriter/2011/01/18/mozilla-skywriter-has-been-merged-into-ace/

The bugs in the Skywriter product are not necessarily relevant for Ace and quite a bit of code has changed. For that reason, I'm closing all of these bugs. Problems that you have with Ace should be filed in the Ace issue tracker at GitHub:

https://github.com/ajaxorg/ace/issues