Fixes #2849: use correct filename and code in require()d sources #2856

Merged
merged 4 commits into from Jun 9, 2013

10 participants

@epidemian

This PR should fix #2849.

As CS source files can be require()d on the fly when using the coffee command, the file and input arguments that are passed to compileScript may not match with the actual file being compiled and run (which could potentially throw compilation errors).

This patch modifies the CoffeeScript.compile function (which is in turn used by compileScript and the Node.js require.extensions mechanism used to require() CS files) so in case a compiler error is raised there, the filename and source information will be attached to it and then used to correctly report errors.

This means that if one runs coffee a.coffee and that file, in turn, does require './b' and the b.coffee file has syntax errors, the error messages will now correctly point to b.coffee.

@epidemian epidemian Fixes #2849: now the compilation errors thrown by CoffeeScript.compil…
…e will include the correct filename and source code information
67fd84f
@jashkenas
Owner
  1. Can we do it without a try/catch?
  2. Can we add a test case to ensure it doesn't break in the future?
@epidemian

BTW, i couldn't think of any easy way to test this. Any ideas?

And also, is the CoffeeScript.eval function ever used?

Edit: ups, race condition; asked the same as @jashkenas there.

@epidemian

Can we do it without a try/catch?

Probably. The problem is mostly that the catch at command/compileScript can't know about which file caused the compilation error at that point, as CoffeeScript.run could have compiled more than one file. That catch at least seems necessary to me, as it's the one that reports the error to the user, it makes sense that is part of the CLI part of the compiler.

Now, what i would question is whether it is necessary to have the require.extensions hook mechanism in the coffee-script.coffee file instead of the command.coffee. If it could be moved to command.coffee and make it use compileScript instead of calling CoffeeScript.compile directly, then i think that this PR's try/catch wouldn't be necessary, as the compileScript function would be receiving the correct information. But i'm not sure how that would affect the running process really.

@michaelficarra
Collaborator
@epidemian

@michaelficarra, thanks for the pointer 👍

Unfortunately, i couldn't get child_process.exec to work in a CS test because of its async nature (the test finishes before child_process.exec calls its callback). And mimicking Mocha's behaviour for async tests seemed like an overkill TBH.

But i found another solution. This passes with this PR and explodes on master:

# (in test/error_messages.coffee)
child_process = require 'child_process'

test "#2849: compilation error in a require()d file", ->
  assertErrorFormat '''
    require './test/sample/syntax-error'
  ''',
  """
    #{__dirname}/sample/syntax-error.coffee:1:15: error: unexpected RELATION
    foo in bar or in baz
                  ^^
  """

This needs to change assertErrorFormat to use CoffeeScript.run instead of CoffeeScript.compile. But it also needs the addition of a test/sample/syntax-error.coffee file, which kinda sucks IMO.

Another alternative is to create the file in temporarily in the test itself:

fs = require 'fs'

test "#2849: compilation error in a require()d file", ->
  # Create a temporary file to require()
  ok not fs.existsSync 'test/syntax-error.coffee'
  fs.writeFileSync 'test/syntax-error.coffee', 'foo in bar or in baz'

  try
    assertErrorFormat '''
      require './test/syntax-error'
    ''',
    """
      #{__dirname}/syntax-error.coffee:1:15: error: unexpected RELATION
      foo in bar or in baz
                    ^^
    """
  finally
    fs.unlink 'test/syntax-error.coffee'

I personally would prefer the second option, as it makes the test more self-contained (though it incurs in a bit of noisy file handling). What do you think? Any better alternative?

@michaelficarra
Collaborator

@epidemian: I like generating the files in the tests. I do something similar in my commonjs-everywhere tests:

I use this really awesome node module, scopedfs, which allows me to scope all the filesystem operations to a fixtures directory.

@epidemian

@michaelficarra, oh, those fixtures look so nice!

CS likes being low on dependencies though, so it's probably not worthy to include that module only for this use-case. In case we need to do this sort of thing more often, we could then create a helper function to use as withTempFile 'temp.txt': content, -> ....

I'll push the second solution then. Thanks!

@epidemian epidemian closed this Mar 21, 2013
@epidemian epidemian reopened this Mar 21, 2013
@michaelficarra
Collaborator

@epidemian: We shy away from regular dependencies, but I don't think we have a similar restriction around development-only dependencies.

@epidemian

@michaelficarra, hmm... ok, in that case, including a nice testing library like Mocha (with cool error reports and async capabilities) instead of re-inventing the wheel is probably a more rewarding endeavour than making this particular test prettier 😄

I think the main issue with this PR is the added try/catch magic though. I think that, ideally, the compilation errors should know about the filename and the source code from the moment they are thrown. We should be able to make that, as the errors are thrown in the lexing/parsing/compiling phases, thus having an options object nearby... but i think it would be quite a lot of work to propagate that information to the points where the errors are thrown. So the solution of adding that information to the errors when they have bubbled up the stack trace doesn't seems too bad to me; the problem is that it's not clear where we should add that information, so in this PR it's done in two places: at CoffeeScript.compile (for compilation errors that can occur in required()d files) and at command/compileScript (for all other errors... lexing, parsing, etc).

@epidemian

Adding a link to #2937 here, as that bug is probably related to this issue.

I'd like to get this issue addressed, as right now if one uses the coffee command to run a script that happens to require a Coffee file with a syntax error, the reported error will be wrong (the file name and the marked line of code will not match the file where the error was thrown) or, worse, will actually break when trying to generate that error message.

I tried giving this thing another spin yesterday, but couldn't figure out a clean way to fix it. The fix in this PR, as @jashkenas rightly pointed out, is quite an ugly hack: a try/catch that appears out of nowhere and a new bunch on mutable state on a bubbling SyntaxError. Not pretty. And adds a layer of complexity that may introduce other bugs.

Here's what's causing the problem in the current master (maybe someone can devise a better solution... @michaelficarra, maybe? 😃):

  • The lexer, parser and AST nodes don't include the file name and source code information (which is necessary to generate the complete error messages) when throwing SyntaxErrors, they just use helpers/throwSyntaxError.
  • When running coffee some-script.coffee, the command/compileScript function gets called with the correct file name and source code to compile/run. Because of this, there's where the SyntaxErrors get caught and the error message that is reported to the user is generated with the added file name and source code.
  • But not every file that gets compiled goes through command/compileScript. When a Coffee file gets require()d on a running script, CoffeeScript.compile is called directly.
  • So if a SyntaxError is thrown when compiling these require()d files, it'll bubble through the CoffeeScript.compile call (losing the file name and source code information) and the generic catch at command/compileScript will catch it, and wrongly assume that the corresponding file name and and source code are the ones of the original script that was run with the coffee command.

When i first implemented the prettified error messages i didn't know about this require.extensions thing, so puting a generic catch at the command/compileScript level (and providing a helpers/prettyErrorMessage for being able to generate the error messages if CoffeeScript.tokens/nodes/compiled was used directly) seemed like a reasonable idea.

What other options do we have?

  1. We could put the generic catch at the CoffeeScript.compile level, which always gets called with the correct file name and source code, generate the correct pretty error message there and assign it to the caught SyntaxErrors as an error.prettyMessage property or function (or even overriding their toString method) and then re-throw them. But that would be a bit inconsistent with other functions like CoffeeScript.tokens/nodes, which are at the same level than compile (and can be used independently) and would not be throwing prettified errors (maybe we could wrap all these "compilation related" functions with a generic one that catches the SyntaxErrors and prettifies them).

  2. Another option could be propagating the file name and source file information to the points where SyntaxErrors get originally thrown at the lexer/parser/nodes, so the errors would always have the necessary information to be prettified and no intermediate try/catch shenanigans would be required. I'm personally fond of this idea, but implementing it is not as easy as it sounds, as it means doing some contortions for passing the options object around (adding a global, or file-level, options object in some places could make things easier, but we would be exchanging one ugly hack for another in that case 😛).

So well, that's basically it. I've run out of ideas for this and would like to have some help to figure out the best way to get this fixed.

Cheers. And sorry for the mega-post!

@vendethiel
Collaborator

isn't that related to #2968 ?

@epidemian

@Nami-Doc, i think it isn't, though i'm not sure what stack traces does that patch fix (the ones expected from runtime failures when running coffee some-script.coffee or the ones from the syntax errors raised by the compiler).

@chajath

#2968 only adds SourceMaps support for required coffee scripts. As far as I can see, only stack trace references it (I could be wrong)

@contra

Is this anywhere near being resolved? This makes CS unusable - we can't debug errors. What was the last working version so we can roll back?

@epidemian

Is this anywhere near being resolved? This makes CS unusable - we can't debug errors. What was the last working version so we can roll back?

Sorry for not having been pushing to get this fixed. It's been a while since i last touched this, but i remember having trouble identifying where to add the error information in order to cover this case of .coffee files being loaded and compiled dynamically with require (something that i didn't consider when first implementing the location-aware error messages; as i didn't know that feature even existed).

Just to be clear about this, you're having this problem when running a .coffee file that, in turn, require()s another .coffee file (with a syntax error), right? If that's the case, i'd venture to guess that one of the reasons this issue has not been given the attention it deserves could be that Coffee developers might be used to compiling their code with coffee --watch instead of running and compiling .coffee files on-the-fly (of course i'm not sure about this... just extrapolating what i do hehe).

@contra

@epidemian - Did this issue come with source maps being added? Yeah we use coffee stuff.coffee and for our modules we compile on the fly as well

@simianhacker

I've ran into this bug several times this week and it's frustrating to say the least. Which makes me question "What gains am I really getting from using CoffeeScript?" Yes it has super sugary syntax and it feels like I'm able to write code faster. All those benefits are wasted when I end up having to try and figure out which file I forgot the comma in (or some other minor typo) because I'm not doing the whole compile/watch dance with a src directory (most of the stuff I'm working on these days is server side, so it's a reasonable expectation to just run coffee index.coffee and have the errors show up in the right place).

@michaelficarra
Collaborator

@ccowan: Try using CoffeeScriptRedux until we can get this issue fixed.

@jashkenas
Owner

@epidemian -- Does this PR address dynamic requires of CoffeeScript files ... or is more work needed in a different direction to make that work?

And if it does address it -- can the PR be updated for a clean merge against master?

@epidemian

@epidemian -- Does this PR address dynamic requires of CoffeeScript files ... or is more work needed in a different direction to make that work?

It should, but we'd need to check if that's still true after the changes on module.extensions (just out of curiosity, require()ing .coffee files dynamically is a feature that will stay, right?).

And if it does address it -- can the PR be updated for a clean merge against master?

Yeah. Maybe later today, but most surely tomorrow i'll be looking at this then.

In the meantime, @jashkenas or @michaelficarra, could you take a look at what is done in this PR and maybe consider the two alternatives i mentioned earlier in this thread and provide some feedback? I'm not really confident that this PR is the best approach and won't introduce new unexpected behaviours.

@jashkenas
Owner

(just out of curiosity, require()ing .coffee files dynamically is a feature that will stay, right?)

Mr. Schlueter seems to have decided to leave it (albeit unfixed) in Node, so yes, it stays.

could you take a look at what is done in this PR and maybe consider the two alternatives ...

Option 1 sounds pretty sane to me. I think that ideally we continue to throw errors as normal, and in a perfect world you have a single catch somewhere high up, or close to the parse-this-file level, that looks at them and corrects the location information before re-throwing.

epidemian added some commits Jun 9, 2013
@epidemian epidemian Merge branch 'master' into issue2849
Conflicts:
	lib/coffee-script/coffee-script.js
	src/coffee-script.coffee
8e90aae
@epidemian epidemian Move a try/catch from compile to loadFile
This try/catch should only be necessary for dynamically loaded files. Also added a lengthier explanation of why this try/catch is needed.
3c880bf
@epidemian

@jashkenas Merged master branch; let me know if a rebase is preferred. Used merge mostly in case someone has checked out my branch (as this issue is quite old now).

BTW, i had a problem with one test failing in the current master. The "javascript modules can be imported" test from importing.coffee failed for the .coffee.md case. I'm on Node 10.10. Don't know if it's a problem on my environment or it's an already known one.

@jashkenas
Owner

Tests should be passing -- but this looks like a pretty lovely way to get this done.

Edit -- to confirm, after merging, tests are still passing for me.

@jashkenas jashkenas merged commit 13187b0 into jashkenas:master Jun 9, 2013
@davidchambers davidchambers commented on the diff Jun 9, 2013
src/helpers.coffee
return error.stack or "#{error}" unless error.location
+ # Prefer original source file information stored in the error if present.
+ filename = error.filename or filename
+ code = error.code or code
@davidchambers
davidchambers added a line comment Jun 9, 2013

Out of interest, is ? preferred here (in place of or)? It more accurately conveys our intent: we're interested in whether error.code has a meaningful value, rather than whether the value is logical true.

There are rare occasions where this distinction is significant. Imagine, for example, we have an object, model, which may not yet exist on the server. We could write the following code:

model.save() unless model.id

What if, though, our ids are sequential integers starting at zero. This would fail in one case. Instead, we should write:

model.save() unless model.id?
@epidemian
epidemian added a line comment Jun 9, 2013

Hmm... good observation. I'm quite used to using conditionals this way (mostly because i program in Ruby on a daily basis), but i'm not sure it's a very solid practice. In fact, i'm pretty sure that if i were to design a language, i wouldn't allow non-boolean expressions in conditionals; it just adds special corner-cases to the language (like what values are "falsy" and whether that behaviour can be overriden, etc).

In this case, the statements could be rewritten as code = error.code if error.code? and they'd be safer. I don't think we're gonna have problems with those because falsy code (i.e. empty strings) should not cause compilation errors, but i wouldn't oppose the change either 😉

@jashkenas
Owner
jashkenas added a line comment Jun 9, 2013

In general, this style (using or and and) is preferred over ? when the values aren't going to be accidentally falsy. If they might be, like with your 0 example, then that's what ? is for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willwhitney

To be clear, this is still a major issue. Can we please get a new version cut to npm?

@jnovack

I still have code that throws an indentation error when compiled with -l (for no apparent reason to me :/) with v1.7.1 that does not throw without.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment