[HN Gopher] Interesting Bugs Caught by ESLint's no-constant-bina...
___________________________________________________________________
Interesting Bugs Caught by ESLint's no-constant-binary-expression
(2022)
Author : CharlesW
Score : 330 points
Date : 2023-11-08 20:41 UTC (1 days ago)
(HTM) web link (eslint.org)
(TXT) w3m dump (eslint.org)
| cultureswitch wrote:
| 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 wrote:
| > 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 wrote:
| 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 wrote:
| 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_ wrote:
| 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.
| addaon wrote:
| Branch coverage is also known as decision coverage.
| Essentially, it means coverage of every machine code
| instruction in the generated binary. This includes every
| statement in every 'then' and 'else' (which would be
| statement coverage), but also includes decisions that are
| sub-expressions and not statements. Consider:
| if (foo() || bar()) { a(); } else {
| b(); }
|
| Statement coverage says that a test must cover the lines
| of code (or statements) `if (foo() || bar())`, `a()`, and
| `b()`. But if `foo()` is tautological then the || short
| circuits, and all of your tests can pass, with 100%
| statement coverage, even if `bar()` causes the sun to go
| nova. With 100% decision coverage, you must have a test
| case that causes `foo()` to return false, so that you can
| test the behavior when `bar()` both returns true and
| returns false.
|
| Edit: The above isn't a correct example, because
| statement coverage will not hit `b()` if `foo()` is
| tautological. Still, you can see the point -- there's a
| difference between decision coverage and statement
| coverage.
| yjftsjthsd-h wrote:
| > 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 wrote:
| 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 wrote:
| 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 wrote:
| > but if you have large amounts of unused code throughout
| the codebase
|
| Delete it? Version control is your backup
| jay_kyburz wrote:
| 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 wrote:
| 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.
| jay_kyburz wrote:
| I agree. (!someBool == anotherBool) very bad code if you ask
| me.
| amatecha wrote:
| 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 wrote:
| agreeWithPreviousCommentOnUselessCode()
| _a_a_a_ wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| captbaritone wrote:
| 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.
| darraghmckay wrote:
| This was really interesting, particularly seeing how a lot of
| these are probably mistakes stemming from familiarity with syntax
| of other languages
| fiddlerwoaroof wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| dragonwriter wrote:
| > Ideally this lint would ignore if (false) and if (true), even
| though these are still constant binary expressions,
|
| I don't think they are: binary expressions affected are
| expressions constructed with binary logical or comparison
| operator (==, ===, &&, ||, ??).
|
| true and false each aren't binary expressions, they are simple
| constant values, and the lint isn't for constant conditionals.
|
| (The rule for that is no-constant-condition.)
| captbaritone wrote:
| Exactly right. And no-constant-expression is configurable to
| allow trivial constant expressions for exactly the reason the
| parent commenter raised.
| nightpool wrote:
| Just remove the conditional? If you ever need it back, git is
| your friend. Or comment it out or something if you _really_
| want to keep your code messy, I 'm not your mom. But I don't
| see any reason why `if (true)` would be better then `// if
| (!isAdmin(user)) {`
| Dylan16807 wrote:
| > But I don't see any reason why `if (true)` would be better
| then `// if (!isAdmin(user)) {`
|
| // takes editing tools in many situations
|
| /* doesn't nest
|
| Setting an if to false is easy and fast.
| twisteriffic wrote:
| > Thinking || allows for set operations: states.includes('VALID'
| || 'IN_PROGRESS')
|
| Genuinely wondering what language that would have been valid in.
| malfist wrote:
| English.
|
| Have a brain fart one day and you'll write something like this
| and be confused why it doesn't work
| swatcoder wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| This is a little further away IMO, as it doesn't have the
| `v1 or v2` construction.
| dragonwriter wrote:
| > 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" ||.
| nodoodles wrote:
| One could abuse Kotlin to make almost this syntax valid.
| Something like: states.includes { +"VALID" || +"IN_PROGRESS" }
|
| Where statestype function 'includes' takes a lambda that's run
| in states context and overrides plus operator for string to
| evaluate to states.contains(that string). One could, but should
| they?
| SonOfLilit wrote:
| I was amazed to discover COBOL has something like this:
|
| IF X = 0 OR = 1 OR = 2
| barosl wrote:
| > states.intersection({'VALID'} | {'IN_PROGRESS'})
|
| Does this Python code count? Haha.
| masklinn wrote:
| No, that's the explicit conjunction of two explicit sets.
| 'VALID' or 'IN_PROGRESS' in states
|
| Does not work and I've seen it around or SO. Or more commonly
| it's variation a == b or c
| lizmat wrote:
| Almost in the Raku Programming Language, bit with a single |
| states.includes('VALID' | 'IN_PROGRESS')
|
| is effectively the same as:
| states.includes('VALID') || states.includes('IN_PROGRESS')
|
| See https://docs.raku.org/type/Junction
| baq wrote:
| datadog allows exactly this syntax modulo JS:
| attribute:(value OR another_value)
| armchairhacker wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| Yeah for sure, I think the linting rule is great.
| swatcoder wrote:
| 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 wrote:
| 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 wrote:
| 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/
|
| [1] https://github.com/jsx-eslint/eslint-plugin-
| react/blob/maste...
|
| [2] Specifically checkLoops of
| 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 wrote:
| 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 wrote:
| 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 wrote:
| > 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/
|
| [2]: https://github.com/eslint/eslint/issues/17596
| n2d4 wrote:
| Regarding the recommendations:
|
| 0 is to prevent mistakes where you declare a function, but
| forget to implement it. If that's really intended I like to put
| a comment "do nothing" in empty functions, even in projects
| without the ESLint rule, so future readers know what's going
| on.
|
| 1 is for consistency & readability. Imagine you read someone
| else's code like <div children="foo" />, and you see that it's
| self-closing, so you expect it's an empty div. Even more
| confusing when you have more than just one attribute. Or say
| you want to modify the code to add a new child, so you remove
| the self-closing and add the child, breaking the old children
| in the process. What is the reason for doing children="foo" in
| the first place anyways?
|
| 2 is to prevent infinite loops, it's good practice to keep an
| upper bound (in a single-threaded world like JS, infinite loops
| can be very deadly and hard to diagnose). I like NASA's ten
| coding commandments (this is #2):
| https://devm.io/careers/power-ten-nasas-coding-commandments-...
| FranksTV wrote:
| I find throwing an error that says "not implemented" to be a
| good solution. Or at least logging a warning.
| josephg wrote:
| Yeah I love rust's todo!() macro for this. Not only does it
| throw an error, but it also typechecks in any context.
| Normally a function needs to actually return whatever it
| says it will return. But throw a todo!() in there, and you
| don't need to return anything at all. (I assume there's
| some type magic going on behind the scenes. Whatever it is,
| I love it.)
|
| But generally I agree that it's overly persnickety to
| complain about empty functions. My use case for them is
| that sometimes you need to pass a function as an argument
| and the function may be null - to indicate you don't want
| to do any work. It's often cleaner to use an empty function
| as a default value rather than add null checks everywhere.
|
| I don't use eslint at all because of defaults like this.
| Having my coding style negged by a tool feels awful. I hate
| gofmt. I ran it once and it deleted a bunch of empty lines
| in my code that I had put in on purpose to aid readability.
| And wow, that pissed me right off! I just know I'll hate a
| lot of eslint's rules just as much. 30 years of coding
| experience gives you opinions. Kids these days don't even
| know how to use the ternary operator properly.
| pmezard wrote:
| > Kids these days don't even know how to use the ternary
| operator properly.
|
| You mean not using it at all? 30 years of coding
| experience gives you opinions.
| captbaritone wrote:
| Author of the rule/post here. It's planned to be included in
| the set of recommend rules in version 9.
| https://eslint.org/blog/2023/11/whats-coming-in-eslint-9.0.0...
| Izkata wrote:
| > [1] https://github.com/jsx-eslint/eslint-plugin-
| react/blob/maste...
|
| From what I remember, being able to pass children as a prop is
| considered a side-effect of an implementation detail, that
| breaks the expected abstraction. There really isn't any reason
| to use it, and I think there's a chance it may even limit what
| the virtual dom diffing can manage?
|
| Also this would prevent you from accidentally doing both at
| once: <MyComponent children={<div>Is it
| me?</div>}> <div>Or is it me?</div>
| </MyComponent>
|
| (I don't remember what React does in this situation)
| MrJohz wrote:
| I use it fairly extensively whenever I have code like
| <Cpt>{props.children}</Cpt> (i.e. whenever the only thing I'm
| doing with the children prop is passing it to another
| component. This comes up pretty often with context providers,
| where I'll write a provider wrapper that handles some stuff
| by itself, and then passes the value and children to a React
| context.
|
| I like using the "children" prop explicitly because it's a
| lot more concise (with prettier, I can often go from five
| lines to one line with this style), and because it's more
| explicit. I'm not creating my own children, I'm not adding
| anything to the markup myself, I'm just passing the input
| children directly to another component.
|
| This isn't really anything unusual, it's exactly what JSX
| compiles to under the hood. And passing JSX elements in props
| rather than as children is useful in other contexts as well -
| for example, a button that allows <Btn icon={<MyIcon />} ...>
|
| You're right that this can cause odd situations where
| children are specified in two ways, but I'd rather have a
| lint rule explicitly handle this case than disallow using the
| "children" prop altogether. (In fact, I think Typescript does
| validate this case already? But I might be wrong there.)
|
| I can see why this rule might be useful to have in general,
| but I agree with the previous poster that it's a semi-
| opinionated rule, and one of the sort of rules where I end up
| having to disable it and fiddle around with configs whenever
| it comes up, which in turn puts me off using ESLint in
| general because I know to get it to be useful I'm going to
| need to spend some time changing everything up.
| silverwind wrote:
| Generally, "recommended" will never suit everyone. Best to
| write your own shareable config.
| aidanlister wrote:
| Best is to use recommended despite a few niggles you might
| have. Second best is shareable.
| WalterBright wrote:
| > 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 wrote:
| 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 wrote:
| Having them in a linter is fine, but in the compiler itself,
| maybe not.
| nightpool wrote:
| 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 wrote:
| 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"))
| maxloh wrote:
| Despite being Turing complete[0], TypeScript itself is more
| like a linter with a special syntax than a complete
| programming language.
|
| Its compiler can only transpile TypeScript into JavaScript
| code, and the language itself does not affect runtime
| behavior (with the exception of enums).
|
| [0]: https://github.com/microsoft/TypeScript/issues/14833
| raincole wrote:
| Keyword: temporarily
| FranksTV wrote:
| So disable the lining rule with a comment? That way you don't
| accidentally commit it.
| WalterBright wrote:
| 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 wrote:
| Is the latter special-cased, or do you just always need a
| braces around a single-line loop?
| WalterBright wrote:
| > 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.
| kccqzy wrote:
| 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
| Quentincestino wrote:
| Well considering the danger an UB can represent in C/C++ code
| you always should run gcc/clang with at least -Wall
| Too wrote:
| 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 wrote:
| 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.
| James_K wrote:
| 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 wrote:
| 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 wrote:
| Maybe sqlite3: https://www.sqlite.org/testing.html.
| jcparkyn wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| captbaritone wrote:
| I think you're technically correct. But if you are someone who
| is writing 100% branch coverage code this rule could still be
| useful. It will quickly point out parts of your code base that
| are going be impossible to test with 100% coverage.
| d3w4s9 wrote:
| 100% coverage is very rare outside safety critical code. People
| are much more willing to invest time in adding features than
| writing (often boring, repetitive and tedious) tests just to
| get perfect coverage -- finding a bug later is a risk people
| are willing to take.
|
| And even 100% doesn't mean much -- you can easily come up with
| examples where you have 100% coverage but there are still bugs.
| SonOfLilit wrote:
| 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 wrote:
| natural language also has its wats
| captbaritone wrote:
| 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 wrote:
| 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.
| cratermoon wrote:
| Ah, Javascript. https://www.destroyallsoftware.com/talks/wat
| peter_l_downs wrote:
| 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
|
| - golangci-lint: revive's constant-logical-expression
| https://golangci-lint.run/usage/linters/#revive
| maxloh wrote:
| 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 wrote:
| There's a short thread by WalterBright in here that explains
| why: 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 wrote:
| TypeScript _has_ to support weird old code.
|
| Eslint-typescript works well for this. I also recommend eslint-
| plugin-sonarjs
| maxloh wrote:
| Does the TypeScript team avoid making breaking changes even
| in major versions?
| amatecha wrote:
| 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 wrote:
| 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
| maxloh wrote:
| Do you have an url of the Stack overflow article?
| d3w4s9 wrote:
| https://stackoverflow.com/questions/52101977/why-
| typescript-...
| kmoser wrote:
| > 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 wrote:
| 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 ...
| pests wrote:
| 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 wrote:
| 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 wrote:
| 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.
| pests wrote:
| I agree.
|
| I was just pointing out that this is a case prettier
| wouldn't do anything. The expression parses differently
| and adding or removing parentheses to any subexpression
| will change its meaning.
|
| That by manually doing the transform in your example that
| you would realize your cody was a no-op or that the
| intended version was what I posted.
| robinson7d wrote:
| In fact, I've seen an extension several times: senior
| developers performing code review, and requesting that
| unnecessary, but clarifying, parens be removed. Not excessive
| parentheses, a single pair in a line or statement.
| captbaritone wrote:
| Author of the rule and blog post here. I agree that, for me, I
| appreciate the extra clarity of explicit parens. This lead me
| to explore a VSCode plugin which visually show the implicit
| parens even if they are not present in the code:
| https://jordaneldredge.com/blog/a-vs-code-extension-to-comba...
|
| I like the idea that different readers of the same code base
| could opt for differing levels of explicitness when it comes to
| operator precedence. One thing that working on the project
| helped demonstrate for me is that adding parens around _every_
| subexpression is _way_ too noisy. So, you need to draw the line
| somewhere. But for me, I prefer drawing that line on the
| noisier side.
| kmoser wrote:
| > One thing that working on the project helped demonstrate
| for me is that adding parens around _every_ subexpression is
| _way_ too noisy. But for me, I prefer drawing that line on
| the noisier side.
|
| I prefer the noisier side as well. When adding parens to
| indicate/force precedence, I will sometimes split my
| expressions into multiple lines, with indentation, as an aid
| to legibility, e.g. instead of ( ( a + b ) / ( ( c - d ) / e
| ) ) I might write: ( ( a + b )
| / ( ( c - d ) / e
| ) )
|
| I've found this not only quite legible, but also fairly easy
| to edit because you can easily match parens visually.
| kawhah wrote:
| Thanks, I hate it.
| FlyingAvatar wrote:
| I appreciate the attempt, but don't the explicit parens
| make it pretty clear?
|
| I feel like if you just remove the spaces around the parens
| themselves, it's the most clear: ((a + b) / ((c - d) / e))
|
| Nowhere I have worked would pass a code review for what you
| have proposed.
| innocenat wrote:
| If it isn't just a b c d e but longer variable name or
| function call, then I imagine it wouldn't be as clear.
| kmoser wrote:
| This. An extreme example, where I think my solution works
| best, is SQL statements that are hundreds of lines long.
| No sane developer would put that all on one line.
| kmoser wrote:
| I'm curious, what is it about my multi-line expression is
| unclear? I find it easier to match parens when they line
| up vertically (similar to aligning braces in C functions)
| than when they're all on one line. True, most IDEs will
| happily show you a matching paren when you hover over
| one, but that still doesn't help you understand how a
| long, one-line expression really breaks down.
|
| Another example: do you write { foo: bar, baz: bat, bing:
| bang } or: { foo: bar,
| baz: bat, bing: bang }
|
| If you use the latter method, why wouldn't you write your
| expressions the same way? Obviously not the ultra simple
| ones like a + b, but the longer ones.
|
| Incidentally, I used to write code with no spaces between
| parens, which is what you suggested: ((a + b) / ((c - d)
| / e)), but eventually found it much easier to _always_
| put spaces around parens: ( ( a + b ) / ( ( c - d ) / e
| ) ). I would say this is similar to the indent-with-
| spaces-vs-tabs argument, which will rage forever.
| rmetzler wrote:
| I would rather argue to add temp variables and name parts
| of the expression, so you can actually understand and
| communicate what this does.
| canucker2016 wrote:
| The javascript operator precedence table is almost the same
| as the C/C++ operator precedence table. I've written a static
| code analyzer for C/C++ code, so I was pleasantly surprised
| when I could also run the script on javascript code and have
| reasonable errors reported as well.
|
| One bug that appeared is bitwise-AND followed by a comparison
| with no parentheses grouping the bitwise-AND operation.
| Obviously developers thought the bitwise-AND had higher
| precedence than the comparison. They were wrong. They
| obviously didn't test the code either.
|
| Javascript code doesn't twiddle bits as often as C/C++ code
| so this bug doesn't appear as often.
|
| For text editor support of this problem, what may work is to
| show that an expression with higher precedence IS evaluated
| first before the expression using an operator lower on the
| operator precedence table.
|
| There was a suggestion in the Vim mailing list years ago to
| use background color to show the nesting depth of a block.
|
| One could use background color or a font attribute, say
| underline or font-size, to indicate the evaluation order of a
| non-parenthesized multi-operator expression.
|
| HN posts only support italics for font formatting so I'll use
| that in my example - hmmm, looks like I can't use block
| formatting in addition - just imagine the code indented in a
| fixed-width font
|
| ====
|
| if (x & _3 == 1_ ) { // do something }
|
| ===
| viraptor wrote:
| Because there's a threshold of where you _know_ how things will
| be parsed. You wouldn 't write: a=(b+c)
|
| Because you know it's not necessary. But we're human and
| sometimes make mistakes when we're sure we didn't.
|
| That's also why we allow for some misunderstanding, (rather
| than (writing (using) (sentence trees))).
| kmoser wrote:
| Agreed, that would be one level too far, and thus
| unnecessary. I'm not arguing that _every_ operator should be
| parenthesized, but I think parens should be used more than
| most programmers tend to do.
|
| On a related note, I occasionally write expressions that both
| assign and test, e.g. if ( ( a = b ) == c ) and in those
| cases I parenthesize liberally and _always_ write a comment
| that indicates yes, I intend to make an assignment in the
| middle of a test for equality.
| demondemidi wrote:
| Oof really? That's a horrible pattern! In my professional
| opinion of course. Separation of causes, man! :)
| porridgeraisin wrote:
| It's very common in C while((buf =
| resultofstreamingoperation() != null) { }
|
| The ` != null ` is redundant here but you can imagine
| where it'd be required.
| sweetjuly wrote:
| A more common form (and slightly less evil) is for error
| checking: if ((error = do_a_thing())) {
| // some error happened, good thing we saved the error
| code }
|
| Given the standard "zero is success" paradigm, handling
| errors is very succinct.
| demondemidi wrote:
| You are arguing "common" == "good". That's not true.
| kmoser wrote:
| Yeah, but separation of causes then violates the DRY
| principle because you have to repeat the 'a' expression
| twice: a = b if ( a == c ) ...
|
| The repetition is even worse if you replace 'a' or 'b'
| with a more complex expression:
| a[foo/bar] = b[baz/bat] if (a[foo/bar] == c) ...
|
| Oof indeed! Just rewrite it as: if ( (
| a[foo/bar] = b[baz/bat] ) == c ) ... // Yes, assign and
| test!
| iamflimflam1 wrote:
| Wow - I've been coding for a long time and find your
| assign and test really hard to parse.
|
| In a code review I would definitely reject this.
| kmoser wrote:
| Just curious, do you also find the ternary operator hard
| to parse? This is a serious question: I've found some
| programmers tend to avoid it entirely, while I think
| there are some clear cases where it's advantageous to
| use.
|
| What I'm getting at is one person's "easy to parse" is
| another person's "difficult to parse", and there may be
| no objective answer which makes one any better than the
| other.
| nickthesick wrote:
| I hope this is satire, DRY is more about extraction of
| shared function not about individual expressions. You
| write code once, no need to save yourself typing time at
| the expense of you and other peoples ability to read and
| understand the code afterward.
| kmoser wrote:
| > DRY is more about extraction of shared function not
| about individual expressions
|
| I'm not sure where you get that idea. Repetition anywhere
| is usually a code smell.
|
| > You write code once
|
| And then you update it when you add new features or the
| requirements change or you fix bugs, etc. Having to
| change two symbols is more error prone than changing one.
| And having to parse more code is harder than parsing less
| code.
|
| The assign-and-test pattern is common in several
| languages (e.g. C), and adding a comment that explains
| the logic should remove all doubt as to what is happening
| and why, so I see it as a win/win.
|
| In any case, there is a trade-off between terseness and
| legibility, and while I usually favor more verbose code,
| I tend to draw the line at needless repetition. But
| that's my personal preference, and everybody draws that
| line in different places.
| rini17 wrote:
| > And having to parse more code is harder than parsing
| less code.
|
| I'd rather parse three straightforward copies with minor
| differences than one chunk of dry spaghetti. And in
| assign & test case, splitting them makes debugging much
| easier.
| kawhah wrote:
| Hmm, no. You rewrite it as key1 =
| foo/bar key2 = baz/bat a[key1] = b[key2]
| if (a[key1] == c)
|
| I'm sure a coder who understands the context can come up
| with better names than key1 and key2.
|
| edit: or better yet key2 = baz/bat
| normal_var = b[key2] if (normal_var == c)
| ... key1 = foo/bar a[key1] = normal_var
| kmoser wrote:
| While your solution splits the whole thing into smaller
| and arguably more easily digestible steps, you have
| introduced two new variables, key1 and key2, which need
| to be carefully scoped (if they aren't already) so they
| don't clash with existing variables. If your project
| consists of many instances of code like this, you end up
| with many variables which can become harder to manage.
|
| You're also splitting what is intended to be essentially
| an atomic operation into multiple steps, which can be
| good if you want to analyze and tweak them in the future,
| but it's now no longer clear where the process begins and
| ends in relation to the code that comes before and after:
| you have to add more comments, or split the whole thing
| out into its own function.
|
| I'm not saying your code is bad or wrong, just that there
| are downsides to any solution (including mine), and
| ultimately everybody has to pick whichever has the fewest
| negatives for their particular project.
| demondemidi wrote:
| Mission critical automotive code under ISO(ASIL) that
| usually follows MISRA, assign and compare in the same
| clause is banned. If you're writing C code that lives
| don't depend on, do whatever you want, but I pity people
| that have to own your code.
| jug wrote:
| Also it's easy to make assumptions and have them slip by when
| more experienced. Like how the ?? operator works in C# or
| even just the bitshift or logical operator order which is
| pretty much standardized across languages by now. But still,
| this can confuse readers or even yourself on a tired day.
| GeneralMaximus wrote:
| I personally make liberal use of parens to make the precedence
| of operators as clear as possible. Then I run formatting tools
| like Prettier or rustfmt on my code, which remove any parens
| that are not strictly required. This way, I end up with an
| expression that is clear as well as concise. Best of both
| worlds.
| captbaritone wrote:
| This is the approach I take as well. But it's not necessarily
| best of both worlds. Removing (or not inserting) technically
| useless parens can obscure bugs if you are not perfectly
| fluent in the precedence rules of the language. Their doubly
| easy to miss if there's a lot going on on one line.
|
| Here's one example that the rule caught: https://github.com/c
| aptbaritone/vscode/blob/ab86e0229d6b4d0c...
|
| I've been writing JS for over 10 years now, and I'm not sure
| I would have caught that in code review.
| amatecha wrote:
| Ahh... there's a strict equality operator comparing
| undefined (both a value and a type) to something that's
| being coerced to a boolean... it will always return false.
| The first thing I look at when I see a strict equality
| comparison is "are the operands actually the same type"
| because that's a very common mistake (or so I've noticed)!
|
| Huh, now I'm curious if my IDE would highlight this with a
| warning... will have to check!
| demondemidi wrote:
| I think one reason is that when I am refactoring I am thinking
| about other larger logic, and as expressions get reduced, I
| become blind to the details that I and previously internalized
| as correct. Linters really help because I can't always be
| thinking at both ends of the abstraction.
| runeks wrote:
| > I want to chalk this up to hubris, but it's probably just
| laziness.
|
| You may be able to chalk it up to some "senior" reviewer who
| asks a junior developer to remove the parens because "they
| aren't needed", and the junior developer doesn't know how to
| argue their case.
| matsemann wrote:
| I put them there, but some linter will then complain about
| "unnecessary parenthesis", so you can't win.
| billpg wrote:
| LISP has entered the chat.
| hoosieree wrote:
| Precedence was a mistake and new language designs should throw
| it in the garbage.
|
| Left to right is fine. So is right to left. Just pick one and
| stick with it.
|
| "But PEMDAS!" Ok, YOU can use parens.
| flashback2199 wrote:
| 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 wrote:
| In aggregate, near enough nobody knows the language that well.
| flashback2199 wrote:
| 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 wrote:
| As it turns out, people are fallible.
|
| Without rigorous systems in place to catch mistakes,
| mistakes will [more frequently] happen.
| prmph wrote:
| Isn't this a special case of the @typescript-eslint/no-
| unnecessary-condition [1] rule?
|
| [1] https://github.com/typescript-eslint/typescript-
| eslint/blob/...
| thaumasiotes wrote:
| > 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.
| _a_a_a_ wrote:
| 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.
| meitros wrote:
| 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.
___________________________________________________________________
(page generated 2023-11-09 23:01 UTC)