14 ms·

How I fixed a bug in Atom

551 points by Garbage 11y ago

sotojuan 11y ago
I'll admit and thought that by "fixed" he meant made it not start slow... but this was very well written and interesting. Great job Mr Galbraith!
snarkyturtle 11y ago
You'd think both Atom contributors and the author would make use of coffeescript's multi-line Regex syntax so they could mark it up with comments...
clay_to_n 11y ago
I liked this article a lot. A programmer found a bug in something he uses every day, and learned enough to fix it. I think articles like this are very useful to beginner/intermediate developers. Everyone in software says "write open source code", "make pull requests to code you use" etc, but there's a very big gap between knowing how to program and knowing how to track down, fix, and submit a PR for a bug in a program (and language!) you've never worked on before. This is a good little tutorial on ways to attack a bug in a program. I especially like that he starts with two print statements - there's no wizardry here, just a programmer digging into a bug.
- sanderjd 11y ago
  Absolutely. I think articles like this are the best way to demystify some of the stuff that seems a bit magical in working software. Things that look complex are rarely the product of genius, but rather of a long string of practical and sometimes painstaking, but fundamental logical problem-solving.
- atom-morgan 11y ago
  I agree. I was recently working with an npm package that wasn't behaving as expected. After some debugging on my end, I was convinced I was using the library as intended. So I started digging through the source code, wrote a few console.log()s, and found the issue. The check for a property in the code didn't match the property name in the README. A simple issue to fix but one that seems much more attainable if you know how someone worked their way to that point. A few years ago and I doubt I would have taken the time to dig. Maybe I should write up a blog post like this one..
andreapaiola 11y ago
Good job
deleted 11y ago
[deleted]
jeffjose 11y ago
Ofcourse it was regex. Regular Expressions: Now You Have Two Problems Good job!
lucaspiller 11y ago
I haven't worked on Atom or Electron apps (although it's on my todo list), but does the Chromium debugger not work inside them?
- Klathmon 11y ago
  Yes it does, and that was my biggest annoyance with this article. The chrome debugger with breakpoints, profilers, and a whole slew of other goodies is great for this sort of debugging. Yeah, sometimes i need to avoid it because turning it on can actually slow the code down significantly, but the profiler would have been perfect to see where the time was spent here with just a few clicks.
  - nacs 11y ago
    > that was my biggest annoyance with this article Perhaps a core contributor or someone who regularly works on the code-base would have approached it with the full suite of debugging tools available as you mention. However, I get the impression that the author of this article is not one of those and simply wanted to crack open his editor to fix this one problem he saw (and happened to learn more about Atom, Coffeescript and regexps along the way). Being upset over a great open-source contribution because they used "non-ideal" methods to arrive at a solution is just silly.
    - Klathmon 11y ago
      I probably could have worded it differently, but I'm not upset about it, it just makes me sad that people might not know that these tools exist! I often use the same kind of thing for debugging (console.log('got here')) because it's sometimes too much work to leave the code to just get an understanding of if something is hit or not. When i started reading the article i was really hoping he would use the profiling tools. I just feel sad that they are being overlooked a lot of the time, and this is a textbook perfect use case for it!
- skybrian 11y ago
  Yes, on a Mac you can type Command-Option-I. On other platforms it should be similar.
wereHamster 11y ago
Could be avoided if the editor had a full AST of the code instead of using regular expressions to try to make sense of it.
- EdiX 11y ago
  The syntax will be broken 99% of the time and making a parser that recovers gracefully under any circumstance is very hard. You could make an editor that only allows valid programs but that opens up a lot of UI problems, it was tried many times and it never took off in practice.
  - Kristine1975 11y ago
    Yet Eclipse and IntelliJ IDEA (and most likely a lot of other IDE's) successfully use AST's for their code editor. The syntax will only be broken locally, so it's not that hard to recover.
    - scotty79 11y ago
      I tried Eclipse many times over last decade. Always used for few weeks before code highlighting and completion drove me insane due to multiple crashes, hangups and slowdowns. Trouble with elaborate 'correct' solutions starts when you need another thing. I don't think support in Eclipse for .cjsx or PureScript or whatever is coming soon. I won't wait 10 years till they get it 'right'. I'll use Atom to get the 95% right in few months.
    - EdiX 11y ago
      I said it's very hard not that it's impossible. If you think it's easy write a parser with recovery for every language Atom supports.
  - barrkel 11y ago
    It doesn't need a full AST. It needs a tokenizer and a nesting level or potentially a stack of different classes of token nesting. This parser would not be difficult to write; the only "recovery" is in choosing how to react to mismatched token pairs. The simplified problem means heuristics could potentially be used.
ilaksh 11y ago
"How I fixed a bug in Atom that affected almost no one, and then spent quite a lot of time writing an article about it, then wrote a title that attempted to give me more credit than I deserved" .. which is his main pass time if you see his other posts, rather than spending his effort on fixing bugs that actually affect a lot of people. I am sorry but I don't really appreciate it and have trouble getting over the misrepresentation in the title of the blog posts.
- reustle 11y ago
  Wow, talk about "haters gonna hate". This dude went into great detail about debugging the issue, which is very interesting. Your negativity isn't really welcome here.
  - ilaksh 11y ago
    Its interesting, but you missed my point.
- jk563 11y ago
  Sure the title is a bit sensationalist, but there was a legitimate issue to fix and he did fix it. Plus, it is a good write up that explains the problem, solution, and the steps taken well. It would be a shame to see others put off from fixing bugs, and posting commentary on the solution, in open source software for fear of being put down.
  - ilaksh 11y ago
    Others should be put off from taking more credit than due in the title of their posts and from taking on bugs that aren't important just for that purpose.
    - oneeyedpigeon 11y ago
      No, nobody should be put off taking on any bug. I hugely appreciate the fact that different people are up for different challenges and, therefore, with enough people, bugs should be fixable. Your ethos appears to be 'some bugs are just boring and not important enough; never bother fixing them'.
diegorbaquero 11y ago
Wow mate, nice debugging down to the core.
cyphar 11y ago
First of all, this is why people should stop adding stupid features to regular expressions. A sane regular expression implementation has no pathological cases. DFA generation can be done in O(n^2) from memory (in the absolute worst case O(n) is average), and matching can't be worse than O(m) or similar (n is the size of the regex and m the size of the string). When you add features like back references and recursive matches, you bring the worst case complexity to O(a^n), which is exponential. And why? So you could write some dirty hack rather than writing a simple recursive descent parser to create an AST of the code (which takes O(n)). What did you gain by ruining your regular expression implementation and writing shitty code? EDIT: Matching might be O(m), but I'm wondering about maximally linked graphs with many epsilon edges. The conversion from DFA to NFA would require O(n^2) in that case. Implementing it as an NFA evaluation might be faster than full conversion, but then you run the risk of having O(n^2) dominating in the matching. EDIT: The author is somewhat correct on what "catastrophic back referencing" is. While you could argue that it is due to bad implementations of the greedy matching, it's a more endemic problem of how you have to implement a regular expression engine that supports back references. If you have to support back references, then you will always have a class of pathological cases which cause exponential time complexity. I had a useful graph about this on my toy regular expression engine (which performs much better than Python's implementation even though it's written in Python and not C): https://github.com/cyphar/redone https://github.com/cyphar/redone.
- JetSetWilly 11y ago
  I don't always want to spend insane amounts of time writing full AST parsers. Sometimes it is a lot easier to write a simple, hacky, throwaway regex. It's a bit much to claim that backtracking regular expressions, a very useful tool sometimes, should never ever be used and everybody should waste loads of time writing careful code even in situations where it isn't required. The problem is not the tool. The problem is using the tool for the wrong job - although who knows, maybe if they didn't just chuck out a quick hacky implementation, then atom simply wouldn't have the feature at all - in which case it is a trade-off between having the feature at all and an evidently tiny corner case bug that was easily fixed by someone who isn't even a regular developer of the project. So I don't see the reason to be outraged or judgemental here.
boulos 11y ago
I got to the bottom and couldn't believe that the solution chosen was to modify the regexp instead of use a for loop. He did such a great job explaining that the code is really just trying to count the number of parentheses or braces to see if they're imbalanced, that it felt the next step was "so I wrote a really simple set of loops that is fast enough on short strings and way less crazy otherwise".
- viraptor 11y ago
  Same here. Also, I don't use Atom, but looking at the expression I'm pretty sure it fails to account for strings with parentheses. The way the matching is done, it looks like it will happily count: func("some call :)") as extra closing paren. (regex101 agrees)
  - mjmasn 11y ago
    Just tried this, you are correct... http://imgur.com/J9tP7lk http://imgur.com/J9tP7lk (note the incorrect bracket highlighting)
    - Grue3 11y ago
      Jesus, how did nobody catch this before? The least a code editor should do is to match parentheses correctly.
      - toomanybeersies 11y ago
        Apparently that's easier said than done.
        zamalek 11y ago
        ... with regex.
        mbrock 11y ago
deleted 11y ago
[deleted]
foxhedgehog 11y ago
getting a 500, which tracks with my experience of atom
jakub_g 11y ago
It seems there's a need of a regex linter (eslint plugin?) that could warn on pathologically complex regexes. Is building this kind of plugin feasible? Having said that, probably few people would explicitly opt in to using such a plugin unless it's bundled by default to some linter.
- andreasklinger 11y ago
  technically yes. but i wouldnt be surprised if deciding on "best practices" is hard given that regex are very often used to get something "just to work".
- oneeyedpigeon 11y ago
  Well, regex101 [1], linked in the article, detects it, so it's definitely possible. Whether that code is open, or it's easy to replicate, is another matter. [1] https://regex101.com/ https://regex101.com/
kitwalker12 11y ago
you sir deserve this https://xkcd.com/208/ https://xkcd.com/208/
roddux 11y ago
Nitpick: Why is the link to 'Atom' in the article just a link to a Google search for 'atom editor'?
- oneeyedpigeon 11y ago
  Maybe Google is offering paid search-referral now! ;-)
- davidvgalbraith 11y ago
  Haha, don't know how that got in there. Fixed it to point to atom.io, thanks for pointing it out!
spinningarrow 11y ago
> This defines a named capture group, <m>. I thought JavaScript regexes don't support named capturing groups. Is Atom using some library or custom functionality for that? EDIT: Or is it a CoffeeScript addition? In their table of contents I only see regex blocks mentioned though (http://coffeescript.org/#regexes http://coffeescript.org/#regexes).
- fuzionmonkey 11y ago
  https://github.com/atom/node-oniguruma https://github.com/atom/node-oniguruma
  - mwcampbell 11y ago
    Would switching to standard JS regular expressions substantially improve Atom's performance, even though it would mean losing some advanced regex features? I remember hearing that V8 JIT-compiles regular expressions to native code.
josteink 11y ago
Completely unrelated to the issue at hand... I had no idea Atom was written in CoffeeScript. I thought it was written in Javascript, which made me positive at the thought of hacking into it. But this code? Definitely giving me a headache, and my interest went down to zero. There seems to be a meme and unchallenged claim in hacker circles that CoffeeScript is somehow more "readable" and easier to understand. Allow me to disagree, and I suspect there's quite a few like me. This code is harder to read. I think I understand what the code does, but I can no longer be certain about the language semantics. That introduces needless uncertainty. And it just broke all my pre-configured and pre-setup JS-tooling. Can't use any of that for this codebase. Just great. So what's up with people writing applications and projects in NodeJS, a prime JS-environment which supports "all" modern Ecmascript-features, classes included, and then decide to go use a non-standard language for their app? And for what gains? How did Coffeescript make this code more readable or easier to debug? It didn't. And now you need to debug code compiled from the actual code you wrote. How does that do anything except make everything harder? TLDR: CoffeeScript seems like a bad choice for just about everything and I can't see why any big project with a desire for contributors would even consider using it.
- Lazare 11y ago
  First, good news: The Atom teams is (slowly) moving away from Coffeescript towards modern JS, and I think they'd agree (unofficially) that the choice proved to be a mistake. Beyond that... 1. The JS world moves stupidly fast, and Coffeescript is a relic of a now-vanished age. It was born, it evolved, and it died. Back in those long ago days of, um, 5 year years ago, there was no ES6, and Coffeescript looked a lot more attractive. So much so, in fact, that ES6 stole a bunch of Coffeescripts better features. 2. If you're familiar with Coffeescript, it's terse and expressive and very readable. If you're not, it looks like gibberish. But that's true of any language. The proper critique of Coffeescript should be "hey, not a lot of potential collaborators know it, so it'll see unreadable to them", not "hey, Coffeescript is generally unreadable". JS is pretty confusing and unreadable if you don't know it too. Mind you, Atom was released 2 years ago, when Coffeescript was already starting to look dated. And it was an open source project looking for contributors so...yeah. Bad, bad choice. :)
sergiotapia 11y ago
I wonder if this is why Atom feels so snappy now. It's my main editor as of a few weeks ago because the speed is now good. It used to choke on a 1000 line rails controller, but now it edits that file just perfectly. Congratulations to the author, great commit! Small payload, enormous benefits.
deleted 11y ago
[deleted]
Tyr42 11y ago
Grrr, that's not a Regular expression at all. I think you'd be better off looping over the string and counting the number of ( and )s in a loop, guaranteed linear time. I wish people would stop abusing regular expressions.
- GFK_of_xmaspast 11y ago
  A regex is far more useful than a regular expression, and I'm ok with the terms getting conflated.
wmonk 11y ago
While this was a good read, should it not be titled: # How I fixed the 'atom/language-go' package
- random_rr 11y ago
  No, he fixed a function of Atom, the original title is valid.
  - wmonk 11y ago
    The regex he fixed was committed into the package atom/language-go, not into atom core. It was only an issue when writing go code.
    - odbol_ 11y ago
      Dude, you don't need to be so semantic. The headline had "atom" in the title, it had to do with Atom the editor, what more do you want? Do you think every headline should be excruciatingly specific and exact? Might as well just put the whole article in the headline.
    - random_rr 11y ago
      I fixed my car the other day by changing a tire - well, technically, I fixed a problem in the car/going-on-the-highway package. But no one on earth would talk or write like that.
ousta 11y ago
Performance wise I never understood why ATOM is even used. it is lackluster compared to a notepad++ and there is a delay in every action: loading the software, clicking on a tab, on a menu, on an option. it seems to me like a very wrong idea to push the web into softwares
- Klathmon 11y ago
  When was the last time you used Atom? It's definitely not the fastest opening editor, and it does have performance issues, but they tend to be more rare than common. The 2 that bite me are: * the update/install screens in the settings tend to be a bit slow * opening "large" files that have 1000+ characters per line will either hang or crash the browser depending on the size. If it's a "normal" looking source file, it runs like a dream with files up to 5gb+ (the largest i've used it for), but if it's something like a minified javascript file, a few kb is enough to hang the editor. Outside of those 2 issues, i don't see any lag or stuttering in my normal use, and i've got about 70 plugins on top of the defaults. But to answer your question, it's the customizability and the massive number of plugins that draws me. Plus it's fucking beautiful, and i know this isn't the most popular opinion, but if i'm going to stare at this thing for 6+ hours a day, i want it to look good!
  - kungtotte 11y ago
    It's by far the slowest compared to Sublime Text 3 and Visual Studio Code. VSCode and Atom obviously suffer from being Electron based compared to ST3, so the comparison there isn't exactly fair, but VSCode is noticeably faster to load and snappier to work with even loaded up with third party plugins. I have a similar amount of plugins for each of the three (10-15) too, so no real difference there. And honestly I don't see a marked difference in visuals between those three. After installing my zenburn colour scheme they're virtually identical with similar if not identical UI features. I don't expect the Electron based editors to match ST3 for performance (at least not yet), but honestly it's kind of embarrassing how slow Atom is compared to VSCode, particularly when it comes to things like checking for package updates and just starting up. This is all based on using all three editors within the last three months (I've been swapping around trying to find what I like).
zenocon 11y ago
Here's another one that needs to get fixed with Atom. Try writing this in the editor with syntax set to Go: expected func someFunc() { aSlice := []string{}{ } } actual func someFunc() { aSlice := []string{}{ } } The end bracket on the slice's initializer never indents correctly when you type it and hit <enter>. It always defaults to the first character of the next line. It seems insertNewLine somehow is not able to grok the idea of more than one set of matching brackets. Edit: issue filed https://github.com/atom/bracket-matcher/issues/209 https://github.com/atom/bracket-matcher/issues/209
- sergiotapia 11y ago
  Install the go-plus package and it'll `go fmt` on save everytime, saving you this headache. Works seamlessly.
  - zenocon 11y ago
    Thanks for the tip -- yea, I have go-plus installed. It is helpful, but still isn't ideal to have to save the file in the middle of trying to initialize a slice.
jeffbr13 11y ago
Isn't arbitrarily-long nesting or matching any sort of palindrome, where you count up and down, the classic case of something you should never do with a regex, because they're finite automata? People don't build parsers because of masochism, but because regular expressions are provably insufficient to capture things like nesting. You need to go at least one level up on the Chomsky hierarchy[1] to pushdown automata for that. More importantly, shouldn't SOMEBODY working on a text editor know and recognise this sort of thing? I'm all for the hacker mentality of shipping, reducing developer time instead of machine time, using what you know, but this is the sort of hacky fix that just pushes the problem further down the line. Great write-up though, very enjoyable read!
- mediumdeviation 11y ago
  You forgot your [1] URL
- jdmichal 11y ago
  I found this on the ground. Did you drop it? [1] https://en.wikipedia.org/wiki/Chomsky_hierarchy#The_hierarchy https://en.wikipedia.org/wiki/Chomsky_hierarchy#The_hierarch...
- revelation 11y ago
  I guess NFA is the classic regex, but most implementations are vastly more powerful through the introduction of backtracking and back references. In any case, a regex is hilariously unsuitable for the purpose here. It's obtuse, it has terrible corner case performance (as noticed here), it probably took vastly longer to write than a simple loop and apparently it isn't even correct.
- cyphar 11y ago
  > Isn't arbitrarily-long nesting or matching any sort of palindrome, where you count up and down, the classic case of something you should never do with a regex, because they're finite automata? Yes, but features like backtracking and backreferences (or recursive referencing) make regex implementations non-regular regular expressions. That comes with a whole heap of issues (pathologically exponential complexities in any circumstance where backtracking is inolved).
fenollp 11y ago
> Having very little to go on, I began the search by searching the whole codebase for the word “newline”. Is this really how people troubleshoot JS issues? In 2016?
- GFK_of_xmaspast 11y ago
  I don't know javascript beyond the most basic aspects, what would you have suggested here.
  - Merad 11y ago
    Start debugger, cause 'freeze' to happen, pause debugger, look at call stack. I touch JavaScript as little as possible, but that's what I would do in any decent language.
bproctor 11y ago
I really really want to love this editor, it's beautiful, but it burns me every time I use it and I end up going back to sublime text. Slow, buggy, hangs, 100% cpu usage, huge amounts of memory usage...
pmilot 11y ago
I'm not familiar with the way syntax highlighting and code indentation logic in text editors is implemented, but using regular expressions to try and parse a CFG like the Go programming language seems like a phenomenally bad idea. Maybe everyone does it and everybody is happy with it working only in 90% of cases, but it just seems like the wrong approach to me.
kcbanner 11y ago
I am still unsure why anyone uses Atom when it consumes so many resources. The emacs instance I have had open for some time now is using 92Mb. Atom instances have been reported in the hundreds of megabytes [1]. Of course, this is because it is actually a web application running in an entire browser, renderer and helper processes included. I understand the need for people to be able to "easily" hack on it (easily in quotes here because really that just means "people who know web languages"), but that goal could still be accomplished in a native application that embedded a JS runtime for plugins. [1]: https://discuss.atom.io/t/high-usage-cpu-and-memory/16165/3 https://discuss.atom.io/t/high-usage-cpu-and-memory/16165/3
- anon4 11y ago
  Because it has a friendly interface, while Vim and Emacs have all these "weird" keybinds and whatnot. These are people switching from e.g. SublimeText to Atom. I am not trying to harp on Vim or Emacs, I myself am a Vim user and use it for C++ code.
  - zeveb 11y ago
    > Because it has a friendly interface, while Vim and Emacs have all these "weird" keybinds and whatnot. vim and emacs are friendly and welcoming to people who already know them; new users in 2016 have some weird expectations due to growing up using insufficiently-powerful UIs, which means that they have quite a learning curve when picking up a powerful UI.
    - dragonwriter 11y ago
      > vim and emacs are friendly and welcoming to people who already know them; new users in 2016 have some weird expectations due to growing up using insufficiently-powerful UIs, which means that they have quite a learning curve when picking up a powerful UI. Having first used both vi and emacs (and other text-mode - and even line-mode -- editors), though only casually then, I disagree; vi and emacs are, like almost anything, friendly and welcoming to people who have become deeply familiar with them, but they simply aren't as accessible as tools that benefit from the advances in the intervening years in making UIs easy to use for people that haven't invested huge amounts of time in familiarity. This is really a different issue than UI power (though if vim and emacs didn't have both powerful UIs and substantially bodies of users with long investments, they wouldn't stick around in the face of their disadvantages in terms of onramp.) Older users just didn't, when they were new, have alternatives with a simpler learning curve. So, there really was no trade off for the power vi and emacs offered (less powerful alternatives of the time were still just as inaccessible), where now there is.
pilif 11y ago
In general, when you are working on a problem and you think "let me use a regex for that" and then you come up with ^\s*[^\s()}]+(?<m>[^()]*$(?:\g<m>|[^()]*)$[^()]*)*[^()]*\)[,]?$ to solve your problem, then you have IMHO come across a problem which you should not be solving using regular expressions. Case in point is counting and balancing parentheses which is very easily done using a single loop over the string in question.
- ngoldbaum 11y ago
  The issue seems to be that Atom uses regular expressions as part of the internal API it uses to separate Atom core from add-on modules.
  - pilif 11y ago
    Maybe the API needs to be changed to allow the module to return a function. After all, JS has first-class functions, so this would make total sense.
    - mohaine 11y ago
      This. With a default implementation that falls back to current behavior.
- jandrese 11y ago
  The worst part is that as soon as he mentioned regular expressions I knew exactly what the problem was. Regexes are powerful and useful but also dangerous. People who don't thoroughly understand them and try to get fancy often run into problems like this. In general you shouldn't be using them to parse a computer language anyway, it is something you should be using a tokenizer/parser for.
  - emodendroket 11y ago
    Well, when the only tool you have is a hammer...
  - 11y ago
alkonaut 11y ago
The lesson is: don't parse with regexes. At all. Even for a first draft of a plugin for editor support for some obscure language should regexes be used for this kind of job. Surprised to see that mistake in code that otherwise looks pretty high quality.
zeveb 11y ago
Yet more proof that anyone who uses regular expressions to attempt to parse a context-free grammar is in a state of sin. Balanced parentheses form a context-free grammar, and thus cannot be parsed by regular expressions. There are extensions to regexps which make them irregular — and hence capable of parsing CFGs — but they often lead to poor performance, as here.
ajmarsh 11y ago
Good work. With enough eyeballs all bugs are shallow.
cliffordfajardo 11y ago
For those who want a quick look at the state of Atom today & it's packages I made a video covering the top frontend and backend packages for Atom. https://www.youtube.com/watch?v=cFAzqvYoHJs https://www.youtube.com/watch?v=cFAzqvYoHJs Or here's a small blog post I made: http://cliffordfajardo.com/2016/atom-editor-review/ http://cliffordfajardo.com/2016/atom-editor-review/
mohsinr 11y ago
What a wonderful article! Yes what a nice trip you had to fix awesome editor (Yeah I am using ATOM and I love to code in it)!
limeyx 11y ago
...and thats why Regexes should usually be avoided :)

551 points by Garbage 11y ago

I'll admit and thought that by "fixed" he meant made it not start slow... but this was very well written and interesting. Great job Mr Galbraith!

snarkyturtle 11y ago

You'd think both Atom contributors and the author would make use of coffeescript's multi-line Regex syntax so they could mark it up with comments...

clay_to_n 11y ago

I liked this article a lot. A programmer found a bug in something he uses every day, and learned enough to fix it. I think articles like this are very useful to beginner/intermediate developers. Everyone in software says "write open source code", "make pull requests to code you use" etc, but there's a very big gap between knowing how to program and knowing how to track down, fix, and submit a PR for a bug in a program (and language!) you've never worked on before. This is a good little tutorial on ways to attack a bug in a program. I especially like that he starts with two print statements - there's no wizardry here, just a programmer digging into a bug.

sanderjd 11y ago
Absolutely. I think articles like this are the best way to demystify some of the stuff that seems a bit magical in working software. Things that look complex are rarely the product of genius, but rather of a long string of practical and sometimes painstaking, but fundamental logical problem-solving.
atom-morgan 11y ago
I agree. I was recently working with an npm package that wasn't behaving as expected. After some debugging on my end, I was convinced I was using the library as intended. So I started digging through the source code, wrote a few console.log()s, and found the issue. The check for a property in the code didn't match the property name in the README. A simple issue to fix but one that seems much more attainable if you know how someone worked their way to that point. A few years ago and I doubt I would have taken the time to dig. Maybe I should write up a blog post like this one..

andreapaiola 11y ago

Good job

deleted 11y ago

[deleted]

jeffjose 11y ago

Ofcourse it was regex. Regular Expressions: Now You Have Two Problems Good job!

lucaspiller 11y ago

I haven't worked on Atom or Electron apps (although it's on my todo list), but does the Chromium debugger not work inside them?

Klathmon 11y ago
Yes it does, and that was my biggest annoyance with this article. The chrome debugger with breakpoints, profilers, and a whole slew of other goodies is great for this sort of debugging. Yeah, sometimes i need to avoid it because turning it on can actually slow the code down significantly, but the profiler would have been perfect to see where the time was spent here with just a few clicks.
- nacs 11y ago
  > that was my biggest annoyance with this article Perhaps a core contributor or someone who regularly works on the code-base would have approached it with the full suite of debugging tools available as you mention. However, I get the impression that the author of this article is not one of those and simply wanted to crack open his editor to fix this one problem he saw (and happened to learn more about Atom, Coffeescript and regexps along the way). Being upset over a great open-source contribution because they used "non-ideal" methods to arrive at a solution is just silly.
  - Klathmon 11y ago
    I probably could have worded it differently, but I'm not upset about it, it just makes me sad that people might not know that these tools exist! I often use the same kind of thing for debugging (console.log('got here')) because it's sometimes too much work to leave the code to just get an understanding of if something is hit or not. When i started reading the article i was really hoping he would use the profiling tools. I just feel sad that they are being overlooked a lot of the time, and this is a textbook perfect use case for it!
skybrian 11y ago
Yes, on a Mac you can type Command-Option-I. On other platforms it should be similar.

wereHamster 11y ago

Could be avoided if the editor had a full AST of the code instead of using regular expressions to try to make sense of it.

EdiX 11y ago
The syntax will be broken 99% of the time and making a parser that recovers gracefully under any circumstance is very hard. You could make an editor that only allows valid programs but that opens up a lot of UI problems, it was tried many times and it never took off in practice.
- Kristine1975 11y ago
  Yet Eclipse and IntelliJ IDEA (and most likely a lot of other IDE's) successfully use AST's for their code editor. The syntax will only be broken locally, so it's not that hard to recover.
  - scotty79 11y ago
    I tried Eclipse many times over last decade. Always used for few weeks before code highlighting and completion drove me insane due to multiple crashes, hangups and slowdowns. Trouble with elaborate 'correct' solutions starts when you need another thing. I don't think support in Eclipse for .cjsx or PureScript or whatever is coming soon. I won't wait 10 years till they get it 'right'. I'll use Atom to get the 95% right in few months.
  - EdiX 11y ago
    I said it's very hard not that it's impossible. If you think it's easy write a parser with recovery for every language Atom supports.
- barrkel 11y ago
  It doesn't need a full AST. It needs a tokenizer and a nesting level or potentially a stack of different classes of token nesting. This parser would not be difficult to write; the only "recovery" is in choosing how to react to mismatched token pairs. The simplified problem means heuristics could potentially be used.

ilaksh 11y ago

"How I fixed a bug in Atom that affected almost no one, and then spent quite a lot of time writing an article about it, then wrote a title that attempted to give me more credit than I deserved" .. which is his main pass time if you see his other posts, rather than spending his effort on fixing bugs that actually affect a lot of people. I am sorry but I don't really appreciate it and have trouble getting over the misrepresentation in the title of the blog posts.

reustle 11y ago
Wow, talk about "haters gonna hate". This dude went into great detail about debugging the issue, which is very interesting. Your negativity isn't really welcome here.
- ilaksh 11y ago
  Its interesting, but you missed my point.
jk563 11y ago
Sure the title is a bit sensationalist, but there was a legitimate issue to fix and he did fix it. Plus, it is a good write up that explains the problem, solution, and the steps taken well. It would be a shame to see others put off from fixing bugs, and posting commentary on the solution, in open source software for fear of being put down.
- ilaksh 11y ago
  Others should be put off from taking more credit than due in the title of their posts and from taking on bugs that aren't important just for that purpose.
  - oneeyedpigeon 11y ago
    No, nobody should be put off taking on any bug. I hugely appreciate the fact that different people are up for different challenges and, therefore, with enough people, bugs should be fixable. Your ethos appears to be 'some bugs are just boring and not important enough; never bother fixing them'.

diegorbaquero 11y ago

Wow mate, nice debugging down to the core.

cyphar 11y ago

First of all, this is why people should stop adding stupid features to regular expressions. A sane regular expression implementation has no pathological cases. DFA generation can be done in O(n^2) from memory (in the absolute worst case O(n) is average), and matching can't be worse than O(m) or similar (n is the size of the regex and m the size of the string). When you add features like back references and recursive matches, you bring the worst case complexity to O(a^n), which is exponential. And why? So you could write some dirty hack rather than writing a simple recursive descent parser to create an AST of the code (which takes O(n)). What did you gain by ruining your regular expression implementation and writing shitty code? EDIT: Matching might be O(m), but I'm wondering about maximally linked graphs with many epsilon edges. The conversion from DFA to NFA would require O(n^2) in that case. Implementing it as an NFA evaluation might be faster than full conversion, but then you run the risk of having O(n^2) dominating in the matching. EDIT: The author is somewhat correct on what "catastrophic back referencing" is. While you could argue that it is due to bad implementations of the greedy matching, it's a more endemic problem of how you have to implement a regular expression engine that supports back references. If you have to support back references, then you will always have a class of pathological cases which cause exponential time complexity. I had a useful graph about this on my toy regular expression engine (which performs much better than Python's implementation even though it's written in Python and not C): https://github.com/cyphar/redone https://github.com/cyphar/redone.

JetSetWilly 11y ago
I don't always want to spend insane amounts of time writing full AST parsers. Sometimes it is a lot easier to write a simple, hacky, throwaway regex. It's a bit much to claim that backtracking regular expressions, a very useful tool sometimes, should never ever be used and everybody should waste loads of time writing careful code even in situations where it isn't required. The problem is not the tool. The problem is using the tool for the wrong job - although who knows, maybe if they didn't just chuck out a quick hacky implementation, then atom simply wouldn't have the feature at all - in which case it is a trade-off between having the feature at all and an evidently tiny corner case bug that was easily fixed by someone who isn't even a regular developer of the project. So I don't see the reason to be outraged or judgemental here.

boulos 11y ago

I got to the bottom and couldn't believe that the solution chosen was to modify the regexp instead of use a for loop. He did such a great job explaining that the code is really just trying to count the number of parentheses or braces to see if they're imbalanced, that it felt the next step was "so I wrote a really simple set of loops that is fast enough on short strings and way less crazy otherwise".

viraptor 11y ago
Same here. Also, I don't use Atom, but looking at the expression I'm pretty sure it fails to account for strings with parentheses. The way the matching is done, it looks like it will happily count: func("some call :)") as extra closing paren. (regex101 agrees)
- mjmasn 11y ago
  Just tried this, you are correct... http://imgur.com/J9tP7lk http://imgur.com/J9tP7lk (note the incorrect bracket highlighting)
  - Grue3 11y ago
    Jesus, how did nobody catch this before? The least a code editor should do is to match parentheses correctly.
    - toomanybeersies 11y ago
      Apparently that's easier said than done.
      - zamalek 11y ago
        ... with regex.
        mbrock 11y ago

deleted 11y ago

[deleted]

foxhedgehog 11y ago

getting a 500, which tracks with my experience of atom

jakub_g 11y ago

It seems there's a need of a regex linter (eslint plugin?) that could warn on pathologically complex regexes. Is building this kind of plugin feasible? Having said that, probably few people would explicitly opt in to using such a plugin unless it's bundled by default to some linter.

andreasklinger 11y ago
technically yes. but i wouldnt be surprised if deciding on "best practices" is hard given that regex are very often used to get something "just to work".
oneeyedpigeon 11y ago
Well, regex101 [1], linked in the article, detects it, so it's definitely possible. Whether that code is open, or it's easy to replicate, is another matter. [1] https://regex101.com/ https://regex101.com/

kitwalker12 11y ago

you sir deserve this https://xkcd.com/208/ https://xkcd.com/208/

roddux 11y ago

Nitpick: Why is the link to 'Atom' in the article just a link to a Google search for 'atom editor'?

oneeyedpigeon 11y ago
Maybe Google is offering paid search-referral now! ;-)
davidvgalbraith 11y ago
Haha, don't know how that got in there. Fixed it to point to atom.io, thanks for pointing it out!

spinningarrow 11y ago

> This defines a named capture group, <m>. I thought JavaScript regexes don't support named capturing groups. Is Atom using some library or custom functionality for that? EDIT: Or is it a CoffeeScript addition? In their table of contents I only see regex blocks mentioned though (http://coffeescript.org/#regexes http://coffeescript.org/#regexes).

fuzionmonkey 11y ago
https://github.com/atom/node-oniguruma https://github.com/atom/node-oniguruma
- mwcampbell 11y ago
  Would switching to standard JS regular expressions substantially improve Atom's performance, even though it would mean losing some advanced regex features? I remember hearing that V8 JIT-compiles regular expressions to native code.

josteink 11y ago

Completely unrelated to the issue at hand... I had no idea Atom was written in CoffeeScript. I thought it was written in Javascript, which made me positive at the thought of hacking into it. But this code? Definitely giving me a headache, and my interest went down to zero. There seems to be a meme and unchallenged claim in hacker circles that CoffeeScript is somehow more "readable" and easier to understand. Allow me to disagree, and I suspect there's quite a few like me. This code is harder to read. I think I understand what the code does, but I can no longer be certain about the language semantics. That introduces needless uncertainty. And it just broke all my pre-configured and pre-setup JS-tooling. Can't use any of that for this codebase. Just great. So what's up with people writing applications and projects in NodeJS, a prime JS-environment which supports "all" modern Ecmascript-features, classes included, and then decide to go use a non-standard language for their app? And for what gains? How did Coffeescript make this code more readable or easier to debug? It didn't. And now you need to debug code compiled from the actual code you wrote. How does that do anything except make everything harder? TLDR: CoffeeScript seems like a bad choice for just about everything and I can't see why any big project with a desire for contributors would even consider using it.

Lazare 11y ago
First, good news: The Atom teams is (slowly) moving away from Coffeescript towards modern JS, and I think they'd agree (unofficially) that the choice proved to be a mistake. Beyond that... 1. The JS world moves stupidly fast, and Coffeescript is a relic of a now-vanished age. It was born, it evolved, and it died. Back in those long ago days of, um, 5 year years ago, there was no ES6, and Coffeescript looked a lot more attractive. So much so, in fact, that ES6 stole a bunch of Coffeescripts better features. 2. If you're familiar with Coffeescript, it's terse and expressive and very readable. If you're not, it looks like gibberish. But that's true of any language. The proper critique of Coffeescript should be "hey, not a lot of potential collaborators know it, so it'll see unreadable to them", not "hey, Coffeescript is generally unreadable". JS is pretty confusing and unreadable if you don't know it too. Mind you, Atom was released 2 years ago, when Coffeescript was already starting to look dated. And it was an open source project looking for contributors so...yeah. Bad, bad choice. :)

sergiotapia 11y ago

I wonder if this is why Atom feels so snappy now. It's my main editor as of a few weeks ago because the speed is now good. It used to choke on a 1000 line rails controller, but now it edits that file just perfectly. Congratulations to the author, great commit! Small payload, enormous benefits.

deleted 11y ago

[deleted]

Tyr42 11y ago

Grrr, that's not a Regular expression at all. I think you'd be better off looping over the string and counting the number of ( and )s in a loop, guaranteed linear time. I wish people would stop abusing regular expressions.

GFK_of_xmaspast 11y ago
A regex is far more useful than a regular expression, and I'm ok with the terms getting conflated.

wmonk 11y ago

While this was a good read, should it not be titled: # How I fixed the 'atom/language-go' package

random_rr 11y ago
No, he fixed a function of Atom, the original title is valid.
- wmonk 11y ago
  The regex he fixed was committed into the package atom/language-go, not into atom core. It was only an issue when writing go code.
  - odbol_ 11y ago
    Dude, you don't need to be so semantic. The headline had "atom" in the title, it had to do with Atom the editor, what more do you want? Do you think every headline should be excruciatingly specific and exact? Might as well just put the whole article in the headline.
  - random_rr 11y ago
    I fixed my car the other day by changing a tire - well, technically, I fixed a problem in the car/going-on-the-highway package. But no one on earth would talk or write like that.

ousta 11y ago

Performance wise I never understood why ATOM is even used. it is lackluster compared to a notepad++ and there is a delay in every action: loading the software, clicking on a tab, on a menu, on an option. it seems to me like a very wrong idea to push the web into softwares

Klathmon 11y ago
When was the last time you used Atom? It's definitely not the fastest opening editor, and it does have performance issues, but they tend to be more rare than common. The 2 that bite me are: * the update/install screens in the settings tend to be a bit slow * opening "large" files that have 1000+ characters per line will either hang or crash the browser depending on the size. If it's a "normal" looking source file, it runs like a dream with files up to 5gb+ (the largest i've used it for), but if it's something like a minified javascript file, a few kb is enough to hang the editor. Outside of those 2 issues, i don't see any lag or stuttering in my normal use, and i've got about 70 plugins on top of the defaults. But to answer your question, it's the customizability and the massive number of plugins that draws me. Plus it's fucking beautiful, and i know this isn't the most popular opinion, but if i'm going to stare at this thing for 6+ hours a day, i want it to look good!
- kungtotte 11y ago
  It's by far the slowest compared to Sublime Text 3 and Visual Studio Code. VSCode and Atom obviously suffer from being Electron based compared to ST3, so the comparison there isn't exactly fair, but VSCode is noticeably faster to load and snappier to work with even loaded up with third party plugins. I have a similar amount of plugins for each of the three (10-15) too, so no real difference there. And honestly I don't see a marked difference in visuals between those three. After installing my zenburn colour scheme they're virtually identical with similar if not identical UI features. I don't expect the Electron based editors to match ST3 for performance (at least not yet), but honestly it's kind of embarrassing how slow Atom is compared to VSCode, particularly when it comes to things like checking for package updates and just starting up. This is all based on using all three editors within the last three months (I've been swapping around trying to find what I like).

zenocon 11y ago

Here's another one that needs to get fixed with Atom. Try writing this in the editor with syntax set to Go: expected func someFunc() { aSlice := []string{}{ } } actual func someFunc() { aSlice := []string{}{ } } The end bracket on the slice's initializer never indents correctly when you type it and hit <enter>. It always defaults to the first character of the next line. It seems insertNewLine somehow is not able to grok the idea of more than one set of matching brackets. Edit: issue filed https://github.com/atom/bracket-matcher/issues/209 https://github.com/atom/bracket-matcher/issues/209

sergiotapia 11y ago
Install the go-plus package and it'll `go fmt` on save everytime, saving you this headache. Works seamlessly.
- zenocon 11y ago
  Thanks for the tip -- yea, I have go-plus installed. It is helpful, but still isn't ideal to have to save the file in the middle of trying to initialize a slice.

jeffbr13 11y ago

Isn't arbitrarily-long nesting or matching any sort of palindrome, where you count up and down, the classic case of something you should never do with a regex, because they're finite automata? People don't build parsers because of masochism, but because regular expressions are provably insufficient to capture things like nesting. You need to go at least one level up on the Chomsky hierarchy[1] to pushdown automata for that. More importantly, shouldn't SOMEBODY working on a text editor know and recognise this sort of thing? I'm all for the hacker mentality of shipping, reducing developer time instead of machine time, using what you know, but this is the sort of hacky fix that just pushes the problem further down the line. Great write-up though, very enjoyable read!

mediumdeviation 11y ago
You forgot your [1] URL
jdmichal 11y ago
I found this on the ground. Did you drop it? [1] https://en.wikipedia.org/wiki/Chomsky_hierarchy#The_hierarchy https://en.wikipedia.org/wiki/Chomsky_hierarchy#The_hierarch...
revelation 11y ago
I guess NFA is the classic regex, but most implementations are vastly more powerful through the introduction of backtracking and back references. In any case, a regex is hilariously unsuitable for the purpose here. It's obtuse, it has terrible corner case performance (as noticed here), it probably took vastly longer to write than a simple loop and apparently it isn't even correct.
cyphar 11y ago
> Isn't arbitrarily-long nesting or matching any sort of palindrome, where you count up and down, the classic case of something you should never do with a regex, because they're finite automata? Yes, but features like backtracking and backreferences (or recursive referencing) make regex implementations non-regular regular expressions. That comes with a whole heap of issues (pathologically exponential complexities in any circumstance where backtracking is inolved).

fenollp 11y ago

> Having very little to go on, I began the search by searching the whole codebase for the word “newline”. Is this really how people troubleshoot JS issues? In 2016?

GFK_of_xmaspast 11y ago
I don't know javascript beyond the most basic aspects, what would you have suggested here.
- Merad 11y ago
  Start debugger, cause 'freeze' to happen, pause debugger, look at call stack. I touch JavaScript as little as possible, but that's what I would do in any decent language.

bproctor 11y ago

I really really want to love this editor, it's beautiful, but it burns me every time I use it and I end up going back to sublime text. Slow, buggy, hangs, 100% cpu usage, huge amounts of memory usage...

pmilot 11y ago

I'm not familiar with the way syntax highlighting and code indentation logic in text editors is implemented, but using regular expressions to try and parse a CFG like the Go programming language seems like a phenomenally bad idea. Maybe everyone does it and everybody is happy with it working only in 90% of cases, but it just seems like the wrong approach to me.

kcbanner 11y ago

I am still unsure why anyone uses Atom when it consumes so many resources. The emacs instance I have had open for some time now is using 92Mb. Atom instances have been reported in the hundreds of megabytes [1]. Of course, this is because it is actually a web application running in an entire browser, renderer and helper processes included. I understand the need for people to be able to "easily" hack on it (easily in quotes here because really that just means "people who know web languages"), but that goal could still be accomplished in a native application that embedded a JS runtime for plugins. [1]: https://discuss.atom.io/t/high-usage-cpu-and-memory/16165/3 https://discuss.atom.io/t/high-usage-cpu-and-memory/16165/3

anon4 11y ago
Because it has a friendly interface, while Vim and Emacs have all these "weird" keybinds and whatnot. These are people switching from e.g. SublimeText to Atom. I am not trying to harp on Vim or Emacs, I myself am a Vim user and use it for C++ code.
- zeveb 11y ago
  > Because it has a friendly interface, while Vim and Emacs have all these "weird" keybinds and whatnot. vim and emacs are friendly and welcoming to people who already know them; new users in 2016 have some weird expectations due to growing up using insufficiently-powerful UIs, which means that they have quite a learning curve when picking up a powerful UI.
  - dragonwriter 11y ago
    > vim and emacs are friendly and welcoming to people who already know them; new users in 2016 have some weird expectations due to growing up using insufficiently-powerful UIs, which means that they have quite a learning curve when picking up a powerful UI. Having first used both vi and emacs (and other text-mode - and even line-mode -- editors), though only casually then, I disagree; vi and emacs are, like almost anything, friendly and welcoming to people who have become deeply familiar with them, but they simply aren't as accessible as tools that benefit from the advances in the intervening years in making UIs easy to use for people that haven't invested huge amounts of time in familiarity. This is really a different issue than UI power (though if vim and emacs didn't have both powerful UIs and substantially bodies of users with long investments, they wouldn't stick around in the face of their disadvantages in terms of onramp.) Older users just didn't, when they were new, have alternatives with a simpler learning curve. So, there really was no trade off for the power vi and emacs offered (less powerful alternatives of the time were still just as inaccessible), where now there is.

pilif 11y ago

In general, when you are working on a problem and you think "let me use a regex for that" and then you come up with ^\s*[^\s()}]+(?<m>[^()]*$(?:\g<m>|[^()]*)$[^()]*)*[^()]*\)[,]?$ to solve your problem, then you have IMHO come across a problem which you should not be solving using regular expressions. Case in point is counting and balancing parentheses which is very easily done using a single loop over the string in question.

ngoldbaum 11y ago
The issue seems to be that Atom uses regular expressions as part of the internal API it uses to separate Atom core from add-on modules.
- pilif 11y ago
  Maybe the API needs to be changed to allow the module to return a function. After all, JS has first-class functions, so this would make total sense.
  - mohaine 11y ago
    This. With a default implementation that falls back to current behavior.
jandrese 11y ago
The worst part is that as soon as he mentioned regular expressions I knew exactly what the problem was. Regexes are powerful and useful but also dangerous. People who don't thoroughly understand them and try to get fancy often run into problems like this. In general you shouldn't be using them to parse a computer language anyway, it is something you should be using a tokenizer/parser for.
- emodendroket 11y ago
  Well, when the only tool you have is a hammer...
- 11y ago

alkonaut 11y ago

The lesson is: don't parse with regexes. At all. Even for a first draft of a plugin for editor support for some obscure language should regexes be used for this kind of job. Surprised to see that mistake in code that otherwise looks pretty high quality.

zeveb 11y ago

Yet more proof that anyone who uses regular expressions to attempt to parse a context-free grammar is in a state of sin. Balanced parentheses form a context-free grammar, and thus cannot be parsed by regular expressions. There are extensions to regexps which make them irregular — and hence capable of parsing CFGs — but they often lead to poor performance, as here.

ajmarsh 11y ago

Good work. With enough eyeballs all bugs are shallow.

cliffordfajardo 11y ago

For those who want a quick look at the state of Atom today & it's packages I made a video covering the top frontend and backend packages for Atom. https://www.youtube.com/watch?v=cFAzqvYoHJs https://www.youtube.com/watch?v=cFAzqvYoHJs Or here's a small blog post I made: http://cliffordfajardo.com/2016/atom-editor-review/ http://cliffordfajardo.com/2016/atom-editor-review/

mohsinr 11y ago

What a wonderful article! Yes what a nice trip you had to fix awesome editor (Yeah I am using ATOM and I love to code in it)!

limeyx 11y ago

...and thats why Regexes should usually be avoided :)