Analyzer should warn on a mistaken JavaScript-function-body

[lrhn]

People used to writing JavaScript sometimes end up writing Dart code like:

someFuture.then(... onError: (e, s) => { logError(e, s) })

The issue is using a => { expression } "JavaScript function literal body" and thinking it just evaluates expression.
The {...} is a set literal in Dart, so it also allocates a new set.

This is particularly easy to do where the function returns void, because of a combination of conveniences:

• You can assign any value to void, even a Set<void>.
• You usually gets a warning if you try to return a value from a void function, but a => function disables that warning, because people like to use => anyExpression as a shorthand for { \n anyExpression; \n }.
• You usually gets a warning if you use the value of a void-typed expression, but not if it occurs in a context with type void. The void expression inside the (inferred) <void>{...} set literal also does not warn about using the value of a void expression.

So it's the perfect storm of looking the other way when you do useless things, combined with the syntax meaning something else in a nearby language.

I'd recommend a warning/lint/hint/suggestion iff:

• If you write (args) => { singleExpression } as a function literal, and
• the return type of the function is inferred as void.

It could possibly be part of a more general warning, about using a literal (which is known to not have any side effects) in a void context (so the resulting value can also be assumed to not be used), which means the the allocation is unnecessary.
Could also be an unnecessary_literal or unnecessary_allocation lint, but this is such a fundamental mistake that I'd like a warning to be enabled by default.

[srawlins]

+1. We really should solve some of these void issues soon. They keep cropping up. I'll note this is a very similar request to dart-lang/linter#2780. I wrote:

So, on the one hand we could go extremely specific and report a set literal, with exactly one element (which is not a spread), whose parent is a function expression body (=>)

I think I intended it to be more like your suggestion though, with the void return type.

[srawlins]

I see this as a warning, enabled-by-default. I can't think of false positives...

[asashour]

https://dart-review.googlesource.com/c/sdk/+/279962

[bwilkerson]

That all begs the question: Do we want a very specific warning for only this case, or do we want a more general warning about the use of collection literals where the value can't be accessed? Are there enough cases of the more general warning to make it worth the extra effort?

[srawlins]

I discussed the downsides of a more general diagnostic at dart-lang/linter#2780 (comment).

I do think this specific warning would be great to have. Any time this issue comes up (the javascript-function-body thing), it is always for this specific case. I think a more general warning involving collection-literals-of-void at large would be correct (have none-to-few false positives), but I don't know of real-world cases that would need to be guarded by a more general warning. I think it would be very pragmatic to enable this specific rule.

[bwilkerson]

Ok, thanks.

[oprypin]

I'm surprised- why isn't this caught by the void_checks lint?

  [1, 2, 3].forEach((x) => {
        print('Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed $x')
      });```

We are a passing   a function that returns a Set   as a Function(void) argument

[srawlins]

It's hard to tell what void_checks is supposed to check for. The implementation does not have any comments, and the tests are in our old style of integration tests, so it's impossible to tell what is intended or asserted.

I believe the case for the example you give, @oprypin, is that we infer a static type of void Function(int) for the function literal. By the time the lint rule sees the function literal, it has that type, and it never has a static type of Set<void> Function(int).

Also, it looks like, from this line in the impl, the void_checks rule does not concern itself with arrow functions (checkedNode.body is! ExpressionFunctionBody), but I could be reading the impl wrong.

[oprypin]

https://dart-review.googlesource.com/c/sdk/+/279962

This works beautifully 🤩
Will be great to have this check

[jacob314]

Looking forward to having this added to the recommended set of lints!

[oprypin]

https://dart-review.googlesource.com/c/sdk/+/279962/comment/453f8d4b_d7b5b4af/

This check as implemented intentionally flags only Set literals of length 1 to prevent only code like

foo(() => {someAction()})

However I am seeing that people have written code as follows and weren't alarmed even by the presence of commas:

foo(() => {someAction(), anotherAction()})

So could we consider also flagging cases with any length?

[bwilkerson]

Does JavaScript allow commas in its function bodies?

It's simple to rewrite the first case by removing the curly braces, but what would you expect users to write in place of the second case?

[oprypin]

I have been applying the fix of removing =>. The lint kicks in only for void return values, the => is out of place there anyway.

foo(() => {someAction(), anotherAction()})

foo(() {
  someAction();
  anotherAction();
})

(and so yes, I also disagree with how the suggested fix was implemented in https://dart-review.googlesource.com/c/sdk/+/282601)

[oprypin]

Does JavaScript allow commas in its function bodies?

I suppose actually it does. JavaScript has this strange comma operator...

But I don't think people wrote the commas there because they knew it from JavaScript, maybe they did it because Dart "for some reason" was complaining about a semicolon there

[lrhn]

Does JavaScript allow commas in its function bodies?

Yup. The JavaScript , operator is a binary infix operator which evaluates to the value of the latter operand.

It has one reasonable use, in the increment section of a for(;; inc1, inc2)... loop.
Everything else is hackish, probably to circumvent a coding style that would require multiple lines otherwise.

If you recognize the usage as mistaken, the rewrite is to remove the => and insert semicolons between and after expressions. And reformat, obviously.

[asashour]

I think JavaScript doesn't allow commas in its function bodies, for example:

var f = () => {
   console.log('abc'),
   return 1;
}
console.log(f());

doesn't work.

Changing the comma to a semicolon, or even removing it, would make it compilable.

Regardless of the commas in JavaScript, the main point about reporting the hint is whether the user intended to have a set, or it was inadvertently written.

The reported hint was explicitly made to trigger with only a single expression, since we are almost sure that the user didn't want to create a set. This was also mentioned in the OP.

I think it should be expanded to cover multiple expressions, as even if the user meant to create a set, it doesn't have an impact, since the function expects a void, and we have a way to convert the commas to semicolon as mentioned.

... I also disagree with how the suggested fix was implemented

Regarding the fix for a single expression, I am not seeing the harm of retaining the =>. As the user has selected this way, and the language also supports it.

Changing it to a block {} would implicitly mean that it is preferred over the =>, but this is not the case as I humbly understand.

[oprypin]

I think JavaScript doesn't allow commas in its function bodies

It allows:

var f = () => {
   console.log('abc'),
   console.log('def')
}

An example with return is not applicable anyway because it also breaks in Dart.

---

the user has selected this way, and the language also supports it.

JavaScript has this syntax:

1. (param) => expression;

2. (param) => {
     statements;
   }

And Dart, correspondingly:

1. (param) => expression;

2. (param) {
     statements;
   }

The user wrote something that corresponds to option 2, why does the fix change it to option 1?

[asashour]

https://dart-review.googlesource.com/c/sdk/+/284021

[lrhn]

The reason

var f = () => {
   console.log('abc'),
   return 1;
}

doesn't work is that , is an operator requiring two expression operands, and return 1 is a statement, not an expression.

About using => or {...}, I read a JS-inspired => { ... } as intending a block body, and would also prefer to convert it to a block body without the (in Dart) unnecessary =>.

[lukehutch]

• , and
• the return type of the function is inferred as void.

I think it would be a mistake to impose this restriction -- I use () {...} all the time with non-void return type, and I frequently erroneously write () => {...}.

I pointed out in my dup #51698 that the effect of this problem can result in some pretty weird errors, which don't indicate what is going wrong -- such as when you use if (cond) {block1} else {block2}, since this occurs in the middle of the construction of a set when => is erroneously included, this if is assumed to be an expression, not a statement, so {block1} and {block2} are treated like nested sets, which then reports errors about semicolons not being expected inside those blocks.

The errors can get so weird that I really recommend this warning be turned on by default in the linter.

[lukehutch]

So could we consider also flagging cases with any length?

I think I have seen VS Code rewrite ("fix" but really break) code that the programmer thought they were writing as a block, but the compiler and linter took to be a set, due to () => { ... , ... }. However, I can't seem to trigger it right now. I do know that the errors you get in this context can be extremely bizarre, for example if statements are treated differently, and their then and else clauses also get treated as sets, in the set context. You can end up with a domino effect, of weirdness compounding on top of weirdness, without the source of the problem being obviously pointed out by the linter!

In addition to a lint that specifies that => and {} are generally mutually exclusive, I would like for the linter warnings and compiler errors to be more explicit when you're in a set-builder context. For example:

  a() {}
  b() {}
  x() => {
    a();         // Error here
    b();
  }

the error is (currently):

Expected to find '}'.dart(expected_token)
Type: Set<Null>

I would prefer something explicit like: ';' is not allowed within 'Set' notation -- expected to find '}'

I'm surprised- why isn't this caught by the void_checks lint?

I wanted to point out the similarity of this issue to a couple of others:

1. Warn about expressions with no effect in void context #53537 asks for a warning when trying to return a Function-typed value from a void lambda (because this almost certainly indicates that parentheses were accidentally omitted).
2. Linter needs to warn about single-item records linter#4761 asks for a warning when adding a comma inside parentheses consisting of a single expression, because this actually constructs a record with one positional field.

These two bugs, as well as this one, are all about detecting cases where the syntax allows programmers to make innocent mistakes with high likelihood, where the likelihood of intentional use of this syntax is either zero or close to zero, and where the warnings or errors that result from the mistake are mystifying, and can occur far from the problem.

Hopefully these issues can all get fixed with warnings, at the least!

[lukehutch]

I just noticed this bug was closed. However, The patch hasn't landed yet. The last update was May 24th by Samuel Rawlins:

I'm prepared to get this landed! Could you rebase once more and I'll try to land it, thanks!!

@asashour Any chance this could get merged?