9 ms·

Interesting Bugs Caught by ESLint's no-constant-binary-expression (2022)

363 points by CharlesW 3y ago

yjftsjthsd-h 3y ago
> Eventually it clicked for me: developers don’t intend to write useless code, and code that does not match the developer’s intent is by definition a bug. Therefore, any useless code you can detect is a bug. I'm not completely sure I would take it all the way to calling it a bug, but I do appreciate a rigorous way to simplify code, because the worst case is that you've made it easier to reason about and that's a win all by itself. (EDIT: This post also makes me feel better about my personal coding style being paranoid and doing things like using parens to force order of operations and avoiding "advanced" constructs like ?? because I don't trust myself to not shoot myself in the foot. I'm not a professional dev, so I'm happy to write verbose, inelegant code in exchange for it being so simple that I'm less likely to screw it up)
- zeta0134 3y ago
  I am a professional developer, and I'll take the verbose, ugly, but readable code every time. Clever code that makes me work harder to understand what it does has a greater chance of being missed, blindly trusted, or just misunderstood, and that causes us untold numbers of headaches. Writing clear, concise code is *hard.* Writing just clear code, conciseness be damned, is a perfectly fine middle ground.
  - NewJazz 3y ago
    OK but if you have large amounts of unused code throughout the codebase, mistakes are more likely to be "missed, blindly trusted, or just misunderstood".
    - teaearlgraycold 3y ago
      > but if you have large amounts of unused code throughout the codebase Delete it? Version control is your backup
      - hyperman1 3y ago
        You have to find it, first. I have a PHP codebase where symfony and doctrine are using strings to connect distant parts of the code. Just trying to find what calls what is hard. I am in the process of replacing as much strings with real calls and adding as much typing as possible, and have thrown away 1/3, but the code base fights back.
    - jay_kyburz 3y ago
      I think when the OP says concise they mean using several lines to make some logic clear, rather than a tricky little one liners. (like many of the mistakes in the article)
      - newZWhoDis 3y ago
        One example of this I HATE is ruby’s obsession with return X unless [complex logic] I sometimes see this stacked inside other complex logic and it’s a nightmare sometimes to figure out the actual control flow.
        nayuki 3y ago
        Which desugars to: if (!complex logic) return X;
    - reportgunner 3y ago
      If you put OR between two ANDs without a paren I think you have bigger issues than unused code.
  - jay_kyburz 3y ago
    I agree. (!someBool == anotherBool) very bad code if you ask me.
    - amatecha 3y ago
      yeah, it's reassuring that for most (all?) of these I'm like "I would never write that"... though I also lean towards very "simple" code that is not only blatantly clear but totally non-"clever". I'd rather the early-in-career dev we bring in 6-12mo later can read it and immediately parse it. Plus it's easier for ME 6-12mo later! hahah :)
  - FranksTV 3y ago
    agreeWithPreviousCommentOnUselessCode()
  - _a_a_a_ 3y ago
    I'm also a professional dev and I 100% agree with you. Clever code is a trap. Clever coders… Well, pretty much a disaster IME. KISS
- Alex3917 3y ago
  I often add an extra line or two of no-op code in order to improve readability. But in these examples, the code clearly wasn't written to communicate anything, but rather by mistake.
  - captbaritone 3y ago
    Rule author here. Would love to see some examples! I think the closest I’ve (knowingly) done to this is to add an empty else clause that contains a comment.
    - Alex3917 3y ago
      So one thing I try to be very good about is never doing more than one thing in a line, which means always using named conditions and named returns. So in other words, if statements and return statements should always contain a single variable that describes the condition or what's being returned, and should not contain function calls, boolean logic, etc. This typically involves giving things descriptive names and then not using them more than once, but it makes the code more readable. So e.g. you have lines like: const isCar = (wheels === 4) && (steeringWheels === 1) && (canDrive === true); if (isCar) { ... } and not lines like: if ((wheels === 4) && (steeringWheels === 1) && canDrive === true) { ... } And similarly: const isCar = (wheels === 4) && (steeringWheels === 1) && (canDrive === true); return isCar; Rather than: return (wheels === 4) && (steeringWheels === 1) && (canDrive === true); I'd actually like linters for JS and Python to enforce named conditionals and named returns, but afaik no one has done this yet.
      - sapiogram 3y ago
        I don't think the author was looking for this kind of example. Everything in your code is useful, so this category of linter would not complain. Other linters might complain about `==`, though ;)
      - captbaritone 3y ago
        Interesting. I see what you're saying, and I don't know if I would characterize that as no-op code. All of that code will run. What I see there is redundant assignments, which I agree can act as a great form of code comment. Often a code comment can be avoided (and clarity improved) by giving a name to an intermediate value rather than letting it be a nameless expression. I like it! But I don't think of this a no-op code and would not imagine a lint rule objecting to it. That said, a code minifier (or compiler) would be well positioned to optimize that code, so you shouldn't even have to worry about even the thought of perf implications.
- deleted 3y ago
  [deleted]
- captbaritone 3y ago
  Author of the rule and post here. I reread the post this morning, and I agree that I should have been less definitive in that sentence. But I stand by the broader point: Useless code is generally not something developers intend to write. When we do, it's generally something exceptional, so a lint suppression is a reasonable way to clarify "I meant to do that". And in the common case where it was an error, the lint rule proves quite helpful. I hope the take away for the reader is: If you can think of other rules that will detect useless code, you should pursue them, because they are likely more valuable than just enabling dead code elimination. They have a high probability of being able to uncover interesting bugs/mistakes as well, which is much more valuable.
- benj111 3y ago
  I wouldn't call it a bug either. I quite often write 'useless' code, just to make the intent more obvious. Int Foo,bar = 1,5 Int baz = foo+bar I could just make baz 6, but then I don't know why it's 6.
  - wes-k 3y ago
    Your code executes and is not useless. > The rule checks for comparisons (==, !==, etc) where the outcome cannot vary at runtime, and logical expressions (&&, ??, ||) which will either always or never short-circuit. If you write if(baz == 6) then the if clause is useless and probably a bug.
    - benj111 3y ago
      A general rule was extracted that 'useless' code is buggy code I'd expect the compiler to optimise out If(somedebugflag) Not tell me off for writing buggy code.
darraghmckay 3y ago
This was really interesting, particularly seeing how a lot of these are probably mistakes stemming from familiarity with syntax of other languages
cultureswitch 3y ago
Having any such binary expression is considered a severe violation of coding rules for certified software (avionics, medical...). There's a good reason, because it's my almost always hiding a bug. And in gray cases where the duplication is not obvious, forces to think hard about it.
- addaon 3y ago
  > There's a good reason, because it's my almost always hiding a bug There's another good reason -- by definition, it's impossible to write a test case with full decision coverage if some of the decisions are themselves impossible.
  - captbaritone 3y ago
    Post author here. That’s a great framing! That means that, in theory, an enforced 100% code coverage would catch these issues as well, which I’m inclined to believe!
    - thomasrockhu 3y ago
      Tom from Codecov here. I'm a huge proponent that branch coverage is way more important than just statement coverage, because as you mentioned, it's way easier to miss these types of bugs just by getting 100% statement coverage.
      - _a_a_a_ 3y ago
        I think I am unclear what you mean by branch coverage. I assumed it means ensuring that every 'then' and 'else' branch is taken? But if so, that means every statement must get covered, right? Put another way, you can only get 100% statement coverage if every 'then' and 'else' branch is taken, so 100% statement coverage and 100% branch coverage seems to be saying the same thing I think a misunderstanding you, an explanation would be helpful, thanks.
        3y ago
fiddlerwoaroof 3y ago
One thing that reading _The Little Prover_ did for me was correct a sort of lazy preconception I had that you cannot “reason about” code in dynamically typed languages. And it sort of crystallized my experience of large dynamically typed codebases not being as bad as one might expect. The patterns in OP are one case here where the boolean expressions are trivially analyzable and there are other classes of such analysis: e.g. it’s possible to use the condition of an id statement to detect useless expressions in both the then and else clauses.
- gwern 3y ago
  Benjamin Pierce has a famous quote: "Attempting to prove any nontrivial theorem about your program will expose lots of bugs: The particular choice of theorem makes little difference!" (This is also true of mathematical proofs - any attempt to formalize a nontrivial proof will usually expose several minor but fixable bugs in the proof.)
lalaithion 3y ago
Ideally this lint would ignore if (false) and if (true), even though these are still constant binary expressions, they aren't vectors for similar bugs like the constant expressions in the blog post. Although "real" production software would use feature flags of some kind instead of hardcoding the constant, sometimes you do just need to hide some code behind an if statement that is currently ineffectual, and linters that prevent that are extremely annoying and force me to write a confusing, convoluted expression that is too complicated for the linter to detect as constant true or constant false.
- AlexMoffat 3y ago
  Can't you, and shouldn't you, just add appropriate comment / annotation to tell the linter to ignore the next line? That way you make it clear to others reading the code that you know what's going on.
- MatthiasPortzel 3y ago
  It’s annoying that linters are responsible for enforcing code-style (indentation, line length, etc.) as well as code correctness (as discussed in the OP), and these sorts of intentional temporary code smells. There’s a huge amount of overlap, so I understand where it comes from. But it means that linters are too intrusive for most small or early-stage projects. (Obviously these lint rules can be turned on and off individually but that’s a hassle.)
  - SonOfLilit 3y ago
    In good setups, linters are only responsible for the latter. Autoformatters of the zero config "any color you want as long as it's black" variety - gofmt, black, etc' - take care of the former.
  - captbaritone 3y ago
    Author or the rule and post here. ESLint version 9, which is going to enable this rule as part of the set of "recommended" rules, is also removing all of the formatting rules. I'm pleased to see that. In the era of pretty printers (which ESLint predates) I think it makes sense to encourage linters to focus on correctness and other conventions aimed at improving code quality rather than just style.
twisteriffic 3y ago
> Thinking || allows for set operations: states.includes('VALID' || 'IN_PROGRESS') Genuinely wondering what language that would have been valid in.
- malfist 3y ago
  English. Have a brain fart one day and you'll write something like this and be confused why it doesn't work
- swatcoder 3y ago
  The type system in TypeScript can compose types from string literals using a similar syntax. Obviously, this is runtime code, not a type declaration, but you can see how a new or tired/busy developer’s brain might spit this out in the wrong place.
- recursive 3y ago
  You can do this in C#, which is similar if you squint. states.Any(s => s is "VALID" or "IN_PROGRESS") I'm sure there are languages where a match-able pattern is an expression, allowing the original code basically unchanged.
  - simlevesque 3y ago
    You can do it in JS too: states.find(state => state === 'VALID' || state === 'IN_PROGRESS') Or states.find(state => ['VALID', 'IN_PROGRESS'].includes(state))
    - recursive 3y ago
      This is a little further away IMO, as it doesn't have the `v1 or v2` construction.
- dragonwriter 3y ago
  > Genuinely wondering what language that would have been valid in. Raku, but using “junctive or” | (which creates an any junction of its arguments, which “autothreads” on method calls producing a Junction of the results) instead of “tight or” ||.
armchairhacker 3y ago
It's surprising how many bugs can be caught by lints like this, which check for code which is obviously flawed in some way (other examples are unused variables, dead code, statements which will always crash, statements which will always throw an exception that aren't `throw` or something else where that would be intended)
ZephyrBlu 3y ago
I'm fascinated by how many examples of this kind of issue there seem to be. Seems like the kind of thing that should be fairly obvious in code review, or should be caught by unit tests.
- ilumanty 3y ago
  Sure, but I‘d rather have a linter detect this kind of code smell automatically if it provides a reasonable explanation to the author. In my experience, a fairly strict non-aesthetic linter setup makes mentoring much more efficient.
  - captbaritone 3y ago
    I love that framing. I’m the author of the rule/post and I see writing rules like this as an opportunity to mentor at scale. Incredibly rewarding to think that, in a sense, I can be in so many engineer’s editors helpfully pointing out (and via documentation explaining) issues right at the moment that the developer needs it. Just in time mentorship!
  - ZephyrBlu 3y ago
    Yeah for sure, I think the linting rule is great.
- swatcoder 3y ago
  No contributor writes consistently (truly) thorough unit tests and no reviewer performs consistently thorough line-by-line analysis. It’s extremely valuable to have multiple layers of verification, and fast-cheap static analysis tools like linters have a tremendously high ROI as one of those layers, especially in languages with many subtle syntax surprises.
- captbaritone 3y ago
  Hey, author of the rule/post here. I'd encourage you to click through to the actual examples linked from the post. Seeing the issues in context, as opposed to the minimal example, can help show how quickly these issues can get lost. It might also be interesting to click "blame" on the line and look at it in the context of the PR that added it. Overall, my point with the examples was to highlight that these are mistakes that even make their way into high visibility projects built by highly competent engineering teams. That said, looking at the issues few were in really critical paths of these projects. Often they cropped up in auxiliary areas like test harnesses or more off-the-beaten-path features. One can assume the same bugs may have existed at some point in the development cycle in other areas of the code base, but they got caught by more rigorous testing/review of those areas, or bug reports. But it's surely a time saver to identify them _as the developer saves the file_ rather than later in the process. The sooner you catch the bug, the more engineering energy you save.
CGamesPlay 3y ago
This is pretty compelling. Why isn't it in the "recommended" preset of lints? In general, I find it frustrating that the eslint recommended presets don't document why they are recommended. I disable several of the rules in all my projects because they seem to be arbitrary stylistic choices (e.g. [0], [1], [2]). [0] https://typescript-eslint.io/rules/no-empty-function/ https://typescript-eslint.io/rules/no-empty-function/ [1] https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-children-prop.md https://github.com/jsx-eslint/eslint-plugin-react/blob/maste... [2] Specifically checkLoops of https://eslint.org/docs/latest/rules/no-constant-condition https://eslint.org/docs/latest/rules/no-constant-condition [edit] In looking up no-empty-function, I saw a stack overflow post that provides a compelling alternative of using `() => undefined` instead of `() => {}`, which suppresses the error. That should be shown as an example on the eslint page!
- gwern 3y ago
  Presumably it's not yet default because it's so new, and that's why he ends the post by asking for tester to opt-in and report any issues.
  - captbaritone 3y ago
    This is correct. The post is from 2022. New default rules are a breaking change and version 9, the first major relates since it was added, is coming soon and will include it by default.
- btmills 3y ago
  > I find it frustrating that the eslint recommended presets don't document why they are recommended. That’s a good point. While this doesn’t address the root problem, I can share some context here as a former maintainer: browsing the core rules [1], you should see that recommended rules flag cases that are highly likely to be bugs or unnecessary constructs. Recommended rules should have false positives only in exceptional cases and be objective or near-universal consensus opinions. Plugins, of course, are free to choose their own threshold for their recommended configs. > Why isn't it in the "recommended" preset of lints? It will be added to recommended in v9! [2] The rule was written during one of the v8 minor versions, and adding to the recommended config is always a breaking change. [1]: https://eslint.org/docs/latest/rules/ https://eslint.org/docs/latest/rules/ [2]: https://github.com/eslint/eslint/issues/17596 https://github.com/eslint/eslint/issues/17596
WalterBright 3y ago
> Comparisons which will always evaluate to true or false and logical expressions (||, &&, ??) which either always short-circuit or never short-circuit are both likely indications of programmer error. I regularly do things like: if (0 && expression) ... and: if (1 || expression) ... to temporarily disable a conditional when I'm looking for a bug.
- nightpool 3y ago
  Well, then presumably it would be nice to have your linter point that out for you so that you don't commit it to the codebase on accident! I've definitely seen a few of those kinds of things accidentally left in in production, which is definitely a code smell
  - WalterBright 3y ago
    Having them in a linter is fine, but in the compiler itself, maybe not.
    - nightpool 3y ago
      Well, luckily this is a post about linters...? I'm not sure I understand your comment. How is it related to the article?
      - gus_massa 3y ago
        WalterBright is the mantainer of the D language, so I guess he sees everithing from the point of view of the compiler. I wrote a lot of code for the optimization step of the compiler of Racket, in particular steps to eliminate similar code. I saw those expressions and I inmediately think: Macro expansions create a lot of similar expressions, and also more complex expressions that the compiler can reduce to trivialy looking expressions [1]. It's nice that the compiler can detect them and eliminate them, but raisng an error would break a lot of code. My guess is that all three of us agree that the post is about linters, but it would be very bad to extend this rule to the compiler. [1] For example, after an expansion of a macro or inlining a function you may get: (define x (random 10)) (if (integer? x) (display x) (error "The number should be an integer"))
WalterBright 3y ago
D makes expressions like: if (a < b < c) if (a < b & c) if (a = b) and statements like: for (i = 0; i < 10; ++i); illegal.
- saagarjha 3y ago
  Is the latter special-cased, or do you just always need a braces around a single-line loop?
  - WalterBright 3y ago
    > you just always need a braces around a single-line loop? Yes. And ditto for `if (expression)` and a couple others. In developing D, I looked at what linters and coding standards did and incorporated into the language rules for troublesome things that there's just no excuse for. For another tidbit, the JSF coding standard specifies that using a lower case 'l' as an integer literal suffix is not allowed, because it is too easily confused with '1'. D just doesn't allow it; you gotta use 'L'. No need to write a coding standard about it.
- deleted 3y ago
  [deleted]
kccqzy 3y ago
Clang and GCC have something like this but restricted to just comparisons, called -Wtautological-compare. I recommend turning it on for the same reasons. Although in practice it may need to be suppressed in macro-heavy or template-heavy code. This has also caught many real world bugs in C such as https://github.com/aircrack-ng/rtl8812au/issues/308 https://github.com/aircrack-ng/rtl8812au/issues/308
- Quentincestino 3y ago
  Well considering the danger an UB can represent in C/C++ code you always should run gcc/clang with at least -Wall
- Too 3y ago
  C compilers are funny on this one, because instead of warning you about code that don’t do anything, they take advantage of it, in the form of UB optimizations. Like you say, macros often make this very hard to enforce in practice.
  - sapiogram 3y ago
    None of the examples in the article would cause UB in analogous C code. A C compiler could still optimize away the useless check itself, but that's true for every language.
deleted 3y ago
[deleted]
James_K 3y ago
This won't be a problem if you have 100% test coverage. Potentially useful to point something out as you type it, but I don't think it does anything profound.
- SonOfLilit 3y ago
  Neither would malloc(), and yet we still like Rust's borrow checker. I've never seen a project with 100% condition tests coverage. Sure, they exist, but they're as rare as unicorns.
  - heinrich5991 3y ago
    Maybe sqlite3: https://www.sqlite.org/testing.html https://www.sqlite.org/testing.html.
    - jcparkyn 3y ago
      Ironically, that's a fantastic example of just how unachievable full test coverage actually is in most projects. > By comparison, the project has 590 times as much test code [as program code].
    - SonOfLilit 3y ago
      I considered mentioning it, but to my previous understanding sqlite measures coverage at the assembly level and not at the C level. Skimming the link, it seems I was wrong, so thanks, TIL. My previous conception doesn't make much sense, now that I think of it.
- altano 3y ago
  You can't have 100% test coverage with these kinds of bugs, because these bugs create unreachable expressions. You can either find out you have these bugs from lint errors or you can find out by finding them in code coverage reports while chasing 100% code coverage. Most people would obviously prefer the former.
- l0b0 3y ago
  100% branch coverage, which is even more rare. I've done it for a couple of projects, and it's really useful (there were bugs hidden even in the last few percent), but it is a decent amount of work.
SonOfLilit 3y ago
On the one hand, now I really want this for python. On the other, I've made most of these bugs with JS, and very few with python, despite having used it more. So many of them are consequences of JS wats...
- colordrops 3y ago
  natural language also has its wats
- captbaritone 3y ago
  Author of the post/rule here. I'd be very curious to see this rule ported/translated to Python! While the gotcha's would probably be different, I suspect it would uncover real bugs. One interesting challenge with JavaScript is that its so ubiquitous that _everyone_ ends up needing to write it at some point. Even those for whom it is a second or third language. I suspect this makes its gotchas all the more common to encounter. Conversely, it makes the value of tools to guide around those gotchas that much more valuable.
  - SonOfLilit 3y ago
    My friends are trying to get me to port it, I'll ping you if I do. Being a common second language is relevant, but JS also has weird truthiness rules and very first-class-feeling lists and hashmaps but not native deep comparisons, and many of the bugs you described stem from this. Those categories of bugs don't exist in Python, that has almost perfect truthiness (PSA: an epoch datetime is falsy! Wat) and deep comparisons by default.
deleted 3y ago
[deleted]
cratermoon 3y ago
Ah, Javascript. https://www.destroyallsoftware.com/talks/wat https://www.destroyallsoftware.com/talks/wat
peter_l_downs 3y ago
Does an equivalent linter exist for Python, or for Golang, or for Scala? I'm sold, it would be great to have something like this. EDIT: - pylint: using-constant-test (W0125) https://docs.pylint.org/features.html#id13 https://docs.pylint.org/features.html#id13 - golangci-lint: revive's constant-logical-expression https://golangci-lint.run/usage/linters/#revive https://golangci-lint.run/usage/linters/#revive
maxloh 3y ago
Why can't TypeScript catch this? I thought TypeScript was able to analyze some static code, like below: const alwaysTrue = true if (alwaysTrue === false) { } // This comparison appears to be unintentional because the types 'true' and 'false' have no overlap.(2367)
- robinson7d 3y ago
  There’s a short thread by WalterBright in here that explains why: https://news.ycombinator.com/item?id=38199237 https://news.ycombinator.com/item?id=38199237 The back and forth on that thread seems to result in advocating for what we’ve got: compiler (TypeScript in your example) not caring, but linter caring. The reason not to in the transpiler is it can be very helpful during testing to block or force some paths.
- simlevesque 3y ago
  TypeScript _has_ to support weird old code. Eslint-typescript works well for this. I also recommend eslint-plugin-sonarjs
  - maxloh 3y ago
    Does the TypeScript team avoid making breaking changes even in major versions?
- amatecha 3y ago
  TypeScript is a superset of JavaScript, so they can't just like.. stop you from writing valid JS. Generally speaking, any valid JS is valid TypeScript. It's not invalid to write that, so TS won't do anything to stop you from doing so. However, fortunately linters exist for the exact reason of catching these sort of "gotchas" or "probably not what you were intending" kind of situations.
- d3w4s9 3y ago
  TypeScript seems to often make arbitrary decisions what they consider a type error and what is not. The argument is often "TypeScript supports everything in JavaScript" but that is done inconsistently. A typical example is string + number results in no warning. I understand that some people actually want to do it for "convenience", but often this turns out to be a mistake (e.g. passing the wrong variable). However, if you have a Map<string, number> and tries to do map.get( 3 ), that's an error even though you can absolutely do that in JavaScript -- it just always returns undefined, no error thrown. You can find a stackoverflow thread about this. To me the current behavior does not make any sense -- I use TypeScript for static typing and avoid any implicit type conversion, and I want that to be consistently applied without exception, that's why I am using TypeScript. I am a bit disappointed that there are a number of such holes in TypeScript. Other times
kmoser 3y ago
> When trying to define default values, people get confused with expressions like a === b ?? c and assume it will be parsed as a === (b ?? c). When in actuality it will be parsed as (a === b) ?? c. I'll never understand why programmers don't simply put parens where they want the expression to be evaluated, rather than relying on their (sometimes incorrect) assumption about operator precedence. I want to chalk this up to hubris, but it's probably just laziness.
- SenHeng 3y ago
  Prettier oftentimes removes a lot of parentheses that I had hoped it would leave. I’ve taken naming them as variables instead. Definitely more verbose but more readability. const aIsB = a === b; … aIsB ?? c …
  - deleted 3y ago
    [deleted]
  - pests 3y ago
    That is how it evaluates, not what the original author intended in the example in the OP. return a === b ?? c; The intent was const bOrDefault = b ?? c; return a === bOrDefault; You posted the actual (buggy) evaluation. I agree that its better if that is what you intend.
  - pests 3y ago
    Wait, your example doesn't even make sense to me on second thought (and was the ultimate point in the article) aIsB is guaranteed to not be null or undefined so the ?? is a no-op. You just did a comparison so it's either true or false. For general clarify for precedence I still agree but in this case it's a silly thing to do. It would be more clear without the syntax shorthand: const ret = a === b if(ret == null || ret == undefined) { ret = c } return ret
    - SenHeng 3y ago
      I was referring particularly to my parent's point about adding parentheses to remove ambiguity. That prettier oftentimes removes parentheses that I explicitly want. So rather than risk that, I remove the ambiguity by moving one of the comparisons off into a named variable.
deleted 3y ago
[deleted]
flashback2199 3y ago
It seems the hypothetical developer in each example didn't know the language that well, and didn't use parens when they were in doubt. Prettier will remove the parens automatically on save, if auto format with prettier is set up in your editor, when the parens are not needed.
- yakshaving_jgt 3y ago
  In aggregate, near enough nobody knows the language that well.
  - flashback2199 3y ago
    Right so what you do is add parens when you don't know, you don't just write code and go whatever lol
    - yakshaving_jgt 3y ago
      As it turns out, people are fallible. Without rigorous systems in place to catch mistakes, mistakes will [more frequently] happen.
deleted 3y ago
[deleted]
prmph 3y ago
Isn't this a special case of the @typescript-eslint/no-unnecessary-condition [1] rule? [1] https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md https://github.com/typescript-eslint/typescript-eslint/blob/...
thaumasiotes 3y ago
> When trying to define default values, people get confused with expressions like a === b ?? c and assume it will be parsed as a === (b ?? c). When in actuality it will be parsed as (a === b) ?? c. Note that this is pointing out a bug in the language spec. There are two ways to assign the precedence: a === b ?? c One says "Compare a to a value, b, which might be null. If it is null, compare against the guard value, c, instead." a === (b ?? c) The other way says "Compare a to b. Consider whether the result of the comparison is NULL, and if it is, do nothing. If it isn't, do nothing then too. In all cases, return the comparison between a and b." (a === b) ?? c There's a good reason people make the erroneous assumption that the language itself isn't trying to sabotage them. The left operand of ?? must be a nullable value; for === to bind tighter than ?? implies that it can return NULL, which in fact it can't.
deleted 3y ago
[deleted]
_a_a_a_ 3y ago
interesting. > where developers misunderstood the precedence of operators, particularly > unary operators like !, + and typeof. if (!whitelist.has(specifier.imported.name) == null) { return; } which is exactly why I always bracket stuff, almost to a fault. I've seen almost exactly this in a piece of shipped code which meant a crucial condition would never trigger. Cost of that? Possibly hundreds of millions or even higher. I'm not exaggerating. This is why I totally hate the tower of precedence in C-style languages.
deleted 3y ago
[deleted]
aaron695 3y ago
[dead]
meitros 3y ago
My past experience with (maybe more limited versions of) this is when you’re developing something and you throw a ‘true ||’ in there as part of that, in can get annoying to have to outsmart your environment to get your code to compile. But surprised at how many issues this can catch - seems worth the occasional extra pain.

363 points by CharlesW 3y ago

> Eventually it clicked for me: developers don’t intend to write useless code, and code that does not match the developer’s intent is by definition a bug. Therefore, any useless code you can detect is a bug. I'm not completely sure I would take it all the way to calling it a bug, but I do appreciate a rigorous way to simplify code, because the worst case is that you've made it easier to reason about and that's a win all by itself. (EDIT: This post also makes me feel better about my personal coding style being paranoid and doing things like using parens to force order of operations and avoiding "advanced" constructs like ?? because I don't trust myself to not shoot myself in the foot. I'm not a professional dev, so I'm happy to write verbose, inelegant code in exchange for it being so simple that I'm less likely to screw it up)

zeta0134 3y ago
I am a professional developer, and I'll take the verbose, ugly, but readable code every time. Clever code that makes me work harder to understand what it does has a greater chance of being missed, blindly trusted, or just misunderstood, and that causes us untold numbers of headaches. Writing clear, concise code is *hard.* Writing just clear code, conciseness be damned, is a perfectly fine middle ground.
- NewJazz 3y ago
  OK but if you have large amounts of unused code throughout the codebase, mistakes are more likely to be "missed, blindly trusted, or just misunderstood".
  - teaearlgraycold 3y ago
    > but if you have large amounts of unused code throughout the codebase Delete it? Version control is your backup
    - hyperman1 3y ago
      You have to find it, first. I have a PHP codebase where symfony and doctrine are using strings to connect distant parts of the code. Just trying to find what calls what is hard. I am in the process of replacing as much strings with real calls and adding as much typing as possible, and have thrown away 1/3, but the code base fights back.
  - jay_kyburz 3y ago
    I think when the OP says concise they mean using several lines to make some logic clear, rather than a tricky little one liners. (like many of the mistakes in the article)
    - newZWhoDis 3y ago
      One example of this I HATE is ruby’s obsession with return X unless [complex logic] I sometimes see this stacked inside other complex logic and it’s a nightmare sometimes to figure out the actual control flow.
      - nayuki 3y ago
        Which desugars to: if (!complex logic) return X;
  - reportgunner 3y ago
    If you put OR between two ANDs without a paren I think you have bigger issues than unused code.
- jay_kyburz 3y ago
  I agree. (!someBool == anotherBool) very bad code if you ask me.
  - amatecha 3y ago
    yeah, it's reassuring that for most (all?) of these I'm like "I would never write that"... though I also lean towards very "simple" code that is not only blatantly clear but totally non-"clever". I'd rather the early-in-career dev we bring in 6-12mo later can read it and immediately parse it. Plus it's easier for ME 6-12mo later! hahah :)
- FranksTV 3y ago
  agreeWithPreviousCommentOnUselessCode()
- _a_a_a_ 3y ago
  I'm also a professional dev and I 100% agree with you. Clever code is a trap. Clever coders… Well, pretty much a disaster IME. KISS
Alex3917 3y ago
I often add an extra line or two of no-op code in order to improve readability. But in these examples, the code clearly wasn't written to communicate anything, but rather by mistake.
- captbaritone 3y ago
  Rule author here. Would love to see some examples! I think the closest I’ve (knowingly) done to this is to add an empty else clause that contains a comment.
  - Alex3917 3y ago
    So one thing I try to be very good about is never doing more than one thing in a line, which means always using named conditions and named returns. So in other words, if statements and return statements should always contain a single variable that describes the condition or what's being returned, and should not contain function calls, boolean logic, etc. This typically involves giving things descriptive names and then not using them more than once, but it makes the code more readable. So e.g. you have lines like: const isCar = (wheels === 4) && (steeringWheels === 1) && (canDrive === true); if (isCar) { ... } and not lines like: if ((wheels === 4) && (steeringWheels === 1) && canDrive === true) { ... } And similarly: const isCar = (wheels === 4) && (steeringWheels === 1) && (canDrive === true); return isCar; Rather than: return (wheels === 4) && (steeringWheels === 1) && (canDrive === true); I'd actually like linters for JS and Python to enforce named conditionals and named returns, but afaik no one has done this yet.
    - sapiogram 3y ago
      I don't think the author was looking for this kind of example. Everything in your code is useful, so this category of linter would not complain. Other linters might complain about `==`, though ;)
    - captbaritone 3y ago
      Interesting. I see what you're saying, and I don't know if I would characterize that as no-op code. All of that code will run. What I see there is redundant assignments, which I agree can act as a great form of code comment. Often a code comment can be avoided (and clarity improved) by giving a name to an intermediate value rather than letting it be a nameless expression. I like it! But I don't think of this a no-op code and would not imagine a lint rule objecting to it. That said, a code minifier (or compiler) would be well positioned to optimize that code, so you shouldn't even have to worry about even the thought of perf implications.
deleted 3y ago
[deleted]
captbaritone 3y ago
Author of the rule and post here. I reread the post this morning, and I agree that I should have been less definitive in that sentence. But I stand by the broader point: Useless code is generally not something developers intend to write. When we do, it's generally something exceptional, so a lint suppression is a reasonable way to clarify "I meant to do that". And in the common case where it was an error, the lint rule proves quite helpful. I hope the take away for the reader is: If you can think of other rules that will detect useless code, you should pursue them, because they are likely more valuable than just enabling dead code elimination. They have a high probability of being able to uncover interesting bugs/mistakes as well, which is much more valuable.
benj111 3y ago
I wouldn't call it a bug either. I quite often write 'useless' code, just to make the intent more obvious. Int Foo,bar = 1,5 Int baz = foo+bar I could just make baz 6, but then I don't know why it's 6.
- wes-k 3y ago
  Your code executes and is not useless. > The rule checks for comparisons (==, !==, etc) where the outcome cannot vary at runtime, and logical expressions (&&, ??, ||) which will either always or never short-circuit. If you write if(baz == 6) then the if clause is useless and probably a bug.
  - benj111 3y ago
    A general rule was extracted that 'useless' code is buggy code I'd expect the compiler to optimise out If(somedebugflag) Not tell me off for writing buggy code.

darraghmckay 3y ago

This was really interesting, particularly seeing how a lot of these are probably mistakes stemming from familiarity with syntax of other languages

cultureswitch 3y ago

Having any such binary expression is considered a severe violation of coding rules for certified software (avionics, medical...). There's a good reason, because it's my almost always hiding a bug. And in gray cases where the duplication is not obvious, forces to think hard about it.

addaon 3y ago
> There's a good reason, because it's my almost always hiding a bug There's another good reason -- by definition, it's impossible to write a test case with full decision coverage if some of the decisions are themselves impossible.
- captbaritone 3y ago
  Post author here. That’s a great framing! That means that, in theory, an enforced 100% code coverage would catch these issues as well, which I’m inclined to believe!
  - thomasrockhu 3y ago
    Tom from Codecov here. I'm a huge proponent that branch coverage is way more important than just statement coverage, because as you mentioned, it's way easier to miss these types of bugs just by getting 100% statement coverage.
    - _a_a_a_ 3y ago
      I think I am unclear what you mean by branch coverage. I assumed it means ensuring that every 'then' and 'else' branch is taken? But if so, that means every statement must get covered, right? Put another way, you can only get 100% statement coverage if every 'then' and 'else' branch is taken, so 100% statement coverage and 100% branch coverage seems to be saying the same thing I think a misunderstanding you, an explanation would be helpful, thanks.
      - 3y ago

fiddlerwoaroof 3y ago

One thing that reading _The Little Prover_ did for me was correct a sort of lazy preconception I had that you cannot “reason about” code in dynamically typed languages. And it sort of crystallized my experience of large dynamically typed codebases not being as bad as one might expect. The patterns in OP are one case here where the boolean expressions are trivially analyzable and there are other classes of such analysis: e.g. it’s possible to use the condition of an id statement to detect useless expressions in both the then and else clauses.

gwern 3y ago
Benjamin Pierce has a famous quote: "Attempting to prove any nontrivial theorem about your program will expose lots of bugs: The particular choice of theorem makes little difference!" (This is also true of mathematical proofs - any attempt to formalize a nontrivial proof will usually expose several minor but fixable bugs in the proof.)

lalaithion 3y ago

Ideally this lint would ignore if (false) and if (true), even though these are still constant binary expressions, they aren't vectors for similar bugs like the constant expressions in the blog post. Although "real" production software would use feature flags of some kind instead of hardcoding the constant, sometimes you do just need to hide some code behind an if statement that is currently ineffectual, and linters that prevent that are extremely annoying and force me to write a confusing, convoluted expression that is too complicated for the linter to detect as constant true or constant false.

AlexMoffat 3y ago
Can't you, and shouldn't you, just add appropriate comment / annotation to tell the linter to ignore the next line? That way you make it clear to others reading the code that you know what's going on.
MatthiasPortzel 3y ago
It’s annoying that linters are responsible for enforcing code-style (indentation, line length, etc.) as well as code correctness (as discussed in the OP), and these sorts of intentional temporary code smells. There’s a huge amount of overlap, so I understand where it comes from. But it means that linters are too intrusive for most small or early-stage projects. (Obviously these lint rules can be turned on and off individually but that’s a hassle.)
- SonOfLilit 3y ago
  In good setups, linters are only responsible for the latter. Autoformatters of the zero config "any color you want as long as it's black" variety - gofmt, black, etc' - take care of the former.
- captbaritone 3y ago
  Author or the rule and post here. ESLint version 9, which is going to enable this rule as part of the set of "recommended" rules, is also removing all of the formatting rules. I'm pleased to see that. In the era of pretty printers (which ESLint predates) I think it makes sense to encourage linters to focus on correctness and other conventions aimed at improving code quality rather than just style.

twisteriffic 3y ago

> Thinking || allows for set operations: states.includes('VALID' || 'IN_PROGRESS') Genuinely wondering what language that would have been valid in.

malfist 3y ago
English. Have a brain fart one day and you'll write something like this and be confused why it doesn't work
swatcoder 3y ago
The type system in TypeScript can compose types from string literals using a similar syntax. Obviously, this is runtime code, not a type declaration, but you can see how a new or tired/busy developer’s brain might spit this out in the wrong place.
recursive 3y ago
You can do this in C#, which is similar if you squint. states.Any(s => s is "VALID" or "IN_PROGRESS") I'm sure there are languages where a match-able pattern is an expression, allowing the original code basically unchanged.
- simlevesque 3y ago
  You can do it in JS too: states.find(state => state === 'VALID' || state === 'IN_PROGRESS') Or states.find(state => ['VALID', 'IN_PROGRESS'].includes(state))
  - recursive 3y ago
    This is a little further away IMO, as it doesn't have the `v1 or v2` construction.
dragonwriter 3y ago
> Genuinely wondering what language that would have been valid in. Raku, but using “junctive or” | (which creates an any junction of its arguments, which “autothreads” on method calls producing a Junction of the results) instead of “tight or” ||.

armchairhacker 3y ago

It's surprising how many bugs can be caught by lints like this, which check for code which is obviously flawed in some way (other examples are unused variables, dead code, statements which will always crash, statements which will always throw an exception that aren't `throw` or something else where that would be intended)

ZephyrBlu 3y ago

I'm fascinated by how many examples of this kind of issue there seem to be. Seems like the kind of thing that should be fairly obvious in code review, or should be caught by unit tests.

ilumanty 3y ago
Sure, but I‘d rather have a linter detect this kind of code smell automatically if it provides a reasonable explanation to the author. In my experience, a fairly strict non-aesthetic linter setup makes mentoring much more efficient.
- captbaritone 3y ago
  I love that framing. I’m the author of the rule/post and I see writing rules like this as an opportunity to mentor at scale. Incredibly rewarding to think that, in a sense, I can be in so many engineer’s editors helpfully pointing out (and via documentation explaining) issues right at the moment that the developer needs it. Just in time mentorship!
- ZephyrBlu 3y ago
  Yeah for sure, I think the linting rule is great.
swatcoder 3y ago
No contributor writes consistently (truly) thorough unit tests and no reviewer performs consistently thorough line-by-line analysis. It’s extremely valuable to have multiple layers of verification, and fast-cheap static analysis tools like linters have a tremendously high ROI as one of those layers, especially in languages with many subtle syntax surprises.
captbaritone 3y ago
Hey, author of the rule/post here. I'd encourage you to click through to the actual examples linked from the post. Seeing the issues in context, as opposed to the minimal example, can help show how quickly these issues can get lost. It might also be interesting to click "blame" on the line and look at it in the context of the PR that added it. Overall, my point with the examples was to highlight that these are mistakes that even make their way into high visibility projects built by highly competent engineering teams. That said, looking at the issues few were in really critical paths of these projects. Often they cropped up in auxiliary areas like test harnesses or more off-the-beaten-path features. One can assume the same bugs may have existed at some point in the development cycle in other areas of the code base, but they got caught by more rigorous testing/review of those areas, or bug reports. But it's surely a time saver to identify them _as the developer saves the file_ rather than later in the process. The sooner you catch the bug, the more engineering energy you save.

CGamesPlay 3y ago

This is pretty compelling. Why isn't it in the "recommended" preset of lints? In general, I find it frustrating that the eslint recommended presets don't document why they are recommended. I disable several of the rules in all my projects because they seem to be arbitrary stylistic choices (e.g. [0], [1], [2]). [0] https://typescript-eslint.io/rules/no-empty-function/ https://typescript-eslint.io/rules/no-empty-function/ [1] https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-children-prop.md https://github.com/jsx-eslint/eslint-plugin-react/blob/maste... [2] Specifically checkLoops of https://eslint.org/docs/latest/rules/no-constant-condition https://eslint.org/docs/latest/rules/no-constant-condition [edit] In looking up no-empty-function, I saw a stack overflow post that provides a compelling alternative of using `() => undefined` instead of `() => {}`, which suppresses the error. That should be shown as an example on the eslint page!

gwern 3y ago
Presumably it's not yet default because it's so new, and that's why he ends the post by asking for tester to opt-in and report any issues.
- captbaritone 3y ago
  This is correct. The post is from 2022. New default rules are a breaking change and version 9, the first major relates since it was added, is coming soon and will include it by default.
btmills 3y ago
> I find it frustrating that the eslint recommended presets don't document why they are recommended. That’s a good point. While this doesn’t address the root problem, I can share some context here as a former maintainer: browsing the core rules [1], you should see that recommended rules flag cases that are highly likely to be bugs or unnecessary constructs. Recommended rules should have false positives only in exceptional cases and be objective or near-universal consensus opinions. Plugins, of course, are free to choose their own threshold for their recommended configs. > Why isn't it in the "recommended" preset of lints? It will be added to recommended in v9! [2] The rule was written during one of the v8 minor versions, and adding to the recommended config is always a breaking change. [1]: https://eslint.org/docs/latest/rules/ https://eslint.org/docs/latest/rules/ [2]: https://github.com/eslint/eslint/issues/17596 https://github.com/eslint/eslint/issues/17596

WalterBright 3y ago

> Comparisons which will always evaluate to true or false and logical expressions (||, &&, ??) which either always short-circuit or never short-circuit are both likely indications of programmer error. I regularly do things like: if (0 && expression) ... and: if (1 || expression) ... to temporarily disable a conditional when I'm looking for a bug.

nightpool 3y ago
Well, then presumably it would be nice to have your linter point that out for you so that you don't commit it to the codebase on accident! I've definitely seen a few of those kinds of things accidentally left in in production, which is definitely a code smell
- WalterBright 3y ago
  Having them in a linter is fine, but in the compiler itself, maybe not.
  - nightpool 3y ago
    Well, luckily this is a post about linters...? I'm not sure I understand your comment. How is it related to the article?
    - gus_massa 3y ago
      WalterBright is the mantainer of the D language, so I guess he sees everithing from the point of view of the compiler. I wrote a lot of code for the optimization step of the compiler of Racket, in particular steps to eliminate similar code. I saw those expressions and I inmediately think: Macro expansions create a lot of similar expressions, and also more complex expressions that the compiler can reduce to trivialy looking expressions [1]. It's nice that the compiler can detect them and eliminate them, but raisng an error would break a lot of code. My guess is that all three of us agree that the post is about linters, but it would be very bad to extend this rule to the compiler. [1] For example, after an expansion of a macro or inlining a function you may get: (define x (random 10)) (if (integer? x) (display x) (error "The number should be an integer"))

WalterBright 3y ago

D makes expressions like: if (a < b < c) if (a < b & c) if (a = b) and statements like: for (i = 0; i < 10; ++i); illegal.

saagarjha 3y ago
Is the latter special-cased, or do you just always need a braces around a single-line loop?
- WalterBright 3y ago
  > you just always need a braces around a single-line loop? Yes. And ditto for `if (expression)` and a couple others. In developing D, I looked at what linters and coding standards did and incorporated into the language rules for troublesome things that there's just no excuse for. For another tidbit, the JSF coding standard specifies that using a lower case 'l' as an integer literal suffix is not allowed, because it is too easily confused with '1'. D just doesn't allow it; you gotta use 'L'. No need to write a coding standard about it.
deleted 3y ago
[deleted]

kccqzy 3y ago

Clang and GCC have something like this but restricted to just comparisons, called -Wtautological-compare. I recommend turning it on for the same reasons. Although in practice it may need to be suppressed in macro-heavy or template-heavy code. This has also caught many real world bugs in C such as https://github.com/aircrack-ng/rtl8812au/issues/308 https://github.com/aircrack-ng/rtl8812au/issues/308

Quentincestino 3y ago
Well considering the danger an UB can represent in C/C++ code you always should run gcc/clang with at least -Wall
Too 3y ago
C compilers are funny on this one, because instead of warning you about code that don’t do anything, they take advantage of it, in the form of UB optimizations. Like you say, macros often make this very hard to enforce in practice.
- sapiogram 3y ago
  None of the examples in the article would cause UB in analogous C code. A C compiler could still optimize away the useless check itself, but that's true for every language.

deleted 3y ago

[deleted]

James_K 3y ago

This won't be a problem if you have 100% test coverage. Potentially useful to point something out as you type it, but I don't think it does anything profound.

SonOfLilit 3y ago
Neither would malloc(), and yet we still like Rust's borrow checker. I've never seen a project with 100% condition tests coverage. Sure, they exist, but they're as rare as unicorns.
- heinrich5991 3y ago
  Maybe sqlite3: https://www.sqlite.org/testing.html https://www.sqlite.org/testing.html.
  - jcparkyn 3y ago
    Ironically, that's a fantastic example of just how unachievable full test coverage actually is in most projects. > By comparison, the project has 590 times as much test code [as program code].
  - SonOfLilit 3y ago
    I considered mentioning it, but to my previous understanding sqlite measures coverage at the assembly level and not at the C level. Skimming the link, it seems I was wrong, so thanks, TIL. My previous conception doesn't make much sense, now that I think of it.
altano 3y ago
You can't have 100% test coverage with these kinds of bugs, because these bugs create unreachable expressions. You can either find out you have these bugs from lint errors or you can find out by finding them in code coverage reports while chasing 100% code coverage. Most people would obviously prefer the former.
l0b0 3y ago
100% branch coverage, which is even more rare. I've done it for a couple of projects, and it's really useful (there were bugs hidden even in the last few percent), but it is a decent amount of work.

SonOfLilit 3y ago

On the one hand, now I really want this for python. On the other, I've made most of these bugs with JS, and very few with python, despite having used it more. So many of them are consequences of JS wats...

colordrops 3y ago
natural language also has its wats
captbaritone 3y ago
Author of the post/rule here. I'd be very curious to see this rule ported/translated to Python! While the gotcha's would probably be different, I suspect it would uncover real bugs. One interesting challenge with JavaScript is that its so ubiquitous that _everyone_ ends up needing to write it at some point. Even those for whom it is a second or third language. I suspect this makes its gotchas all the more common to encounter. Conversely, it makes the value of tools to guide around those gotchas that much more valuable.
- SonOfLilit 3y ago
  My friends are trying to get me to port it, I'll ping you if I do. Being a common second language is relevant, but JS also has weird truthiness rules and very first-class-feeling lists and hashmaps but not native deep comparisons, and many of the bugs you described stem from this. Those categories of bugs don't exist in Python, that has almost perfect truthiness (PSA: an epoch datetime is falsy! Wat) and deep comparisons by default.

deleted 3y ago

[deleted]

cratermoon 3y ago

Ah, Javascript. https://www.destroyallsoftware.com/talks/wat https://www.destroyallsoftware.com/talks/wat

peter_l_downs 3y ago

Does an equivalent linter exist for Python, or for Golang, or for Scala? I'm sold, it would be great to have something like this. EDIT: - pylint: using-constant-test (W0125) https://docs.pylint.org/features.html#id13 https://docs.pylint.org/features.html#id13 - golangci-lint: revive's constant-logical-expression https://golangci-lint.run/usage/linters/#revive https://golangci-lint.run/usage/linters/#revive

maxloh 3y ago

Why can't TypeScript catch this? I thought TypeScript was able to analyze some static code, like below: const alwaysTrue = true if (alwaysTrue === false) { } // This comparison appears to be unintentional because the types 'true' and 'false' have no overlap.(2367)

robinson7d 3y ago
There’s a short thread by WalterBright in here that explains why: https://news.ycombinator.com/item?id=38199237 https://news.ycombinator.com/item?id=38199237 The back and forth on that thread seems to result in advocating for what we’ve got: compiler (TypeScript in your example) not caring, but linter caring. The reason not to in the transpiler is it can be very helpful during testing to block or force some paths.
simlevesque 3y ago
TypeScript _has_ to support weird old code. Eslint-typescript works well for this. I also recommend eslint-plugin-sonarjs
- maxloh 3y ago
  Does the TypeScript team avoid making breaking changes even in major versions?
amatecha 3y ago
TypeScript is a superset of JavaScript, so they can't just like.. stop you from writing valid JS. Generally speaking, any valid JS is valid TypeScript. It's not invalid to write that, so TS won't do anything to stop you from doing so. However, fortunately linters exist for the exact reason of catching these sort of "gotchas" or "probably not what you were intending" kind of situations.
d3w4s9 3y ago
TypeScript seems to often make arbitrary decisions what they consider a type error and what is not. The argument is often "TypeScript supports everything in JavaScript" but that is done inconsistently. A typical example is string + number results in no warning. I understand that some people actually want to do it for "convenience", but often this turns out to be a mistake (e.g. passing the wrong variable). However, if you have a Map<string, number> and tries to do map.get( 3 ), that's an error even though you can absolutely do that in JavaScript -- it just always returns undefined, no error thrown. You can find a stackoverflow thread about this. To me the current behavior does not make any sense -- I use TypeScript for static typing and avoid any implicit type conversion, and I want that to be consistently applied without exception, that's why I am using TypeScript. I am a bit disappointed that there are a number of such holes in TypeScript. Other times

kmoser 3y ago

> When trying to define default values, people get confused with expressions like a === b ?? c and assume it will be parsed as a === (b ?? c). When in actuality it will be parsed as (a === b) ?? c. I'll never understand why programmers don't simply put parens where they want the expression to be evaluated, rather than relying on their (sometimes incorrect) assumption about operator precedence. I want to chalk this up to hubris, but it's probably just laziness.

SenHeng 3y ago
Prettier oftentimes removes a lot of parentheses that I had hoped it would leave. I’ve taken naming them as variables instead. Definitely more verbose but more readability. const aIsB = a === b; … aIsB ?? c …
- deleted 3y ago
  [deleted]
- pests 3y ago
  That is how it evaluates, not what the original author intended in the example in the OP. return a === b ?? c; The intent was const bOrDefault = b ?? c; return a === bOrDefault; You posted the actual (buggy) evaluation. I agree that its better if that is what you intend.
- pests 3y ago
  Wait, your example doesn't even make sense to me on second thought (and was the ultimate point in the article) aIsB is guaranteed to not be null or undefined so the ?? is a no-op. You just did a comparison so it's either true or false. For general clarify for precedence I still agree but in this case it's a silly thing to do. It would be more clear without the syntax shorthand: const ret = a === b if(ret == null || ret == undefined) { ret = c } return ret
  - SenHeng 3y ago
    I was referring particularly to my parent's point about adding parentheses to remove ambiguity. That prettier oftentimes removes parentheses that I explicitly want. So rather than risk that, I remove the ambiguity by moving one of the comparisons off into a named variable.

deleted 3y ago

[deleted]

flashback2199 3y ago

It seems the hypothetical developer in each example didn't know the language that well, and didn't use parens when they were in doubt. Prettier will remove the parens automatically on save, if auto format with prettier is set up in your editor, when the parens are not needed.

yakshaving_jgt 3y ago
In aggregate, near enough nobody knows the language that well.
- flashback2199 3y ago
  Right so what you do is add parens when you don't know, you don't just write code and go whatever lol
  - yakshaving_jgt 3y ago
    As it turns out, people are fallible. Without rigorous systems in place to catch mistakes, mistakes will [more frequently] happen.

deleted 3y ago

[deleted]

prmph 3y ago

Isn't this a special case of the @typescript-eslint/no-unnecessary-condition [1] rule? [1] https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md https://github.com/typescript-eslint/typescript-eslint/blob/...

thaumasiotes 3y ago

> When trying to define default values, people get confused with expressions like a === b ?? c and assume it will be parsed as a === (b ?? c). When in actuality it will be parsed as (a === b) ?? c. Note that this is pointing out a bug in the language spec. There are two ways to assign the precedence: a === b ?? c One says "Compare a to a value, b, which might be null. If it is null, compare against the guard value, c, instead." a === (b ?? c) The other way says "Compare a to b. Consider whether the result of the comparison is NULL, and if it is, do nothing. If it isn't, do nothing then too. In all cases, return the comparison between a and b." (a === b) ?? c There's a good reason people make the erroneous assumption that the language itself isn't trying to sabotage them. The left operand of ?? must be a nullable value; for === to bind tighter than ?? implies that it can return NULL, which in fact it can't.

deleted 3y ago

[deleted]

_a_a_a_ 3y ago

interesting. > where developers misunderstood the precedence of operators, particularly > unary operators like !, + and typeof. if (!whitelist.has(specifier.imported.name) == null) { return; } which is exactly why I always bracket stuff, almost to a fault. I've seen almost exactly this in a piece of shipped code which meant a crucial condition would never trigger. Cost of that? Possibly hundreds of millions or even higher. I'm not exaggerating. This is why I totally hate the tower of precedence in C-style languages.

deleted 3y ago

[deleted]

aaron695 3y ago

[dead]

meitros 3y ago

My past experience with (maybe more limited versions of) this is when you’re developing something and you throw a ‘true ||’ in there as part of that, in can get annoying to have to outsmart your environment to get your code to compile. But surprised at how many issues this can catch - seems worth the occasional extra pain.